ptuser: guarantee that all names are valid C strings

The prname type is represented in XDR as a vector[PR_MAXNAMELEN]
of char, not as a string, which means that the XDR (de)serializer
will not guarantee null-termination.  Guarantee that all buffers
used in the public protection server API are in fact valid strings
by disallowing any names that are exactly PR_MAXNAMELEN (64)
characters long.  DO NOT silently truncate names that are even
longer than this.  Consistently use the prname typedef in
declarations to reinforce the length limitation to those reading
the header file.  Introduces a new protection error code,
PRNAMETOOLONG, which will be returned if either IN or OUT parameters
would exceed the limit.

[kaduk@mit.edu convert macro to static_inline function and expand
at call sites; add string_ wrapper to add checking to viced and libadmin;
export the string_ wrapper from libafsauthent for the windows build]

Change-Id: I65f850afcfea2fd2bc0110ca7b7f6ecca247dd58
Reviewed-on: https://gerrit.openafs.org/7896
Reviewed-by: Chas Williams <3chas3@gmail.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

diff --git a/src/libadmin/pts/afs_ptsAdmin.c b/src/libadmin/pts/afs_ptsAdmin.c
index 845bab812c520108243c98aa41e73232ffd414b2..80f7d4ccff2fc86edaba46bc6edb1be4e5997615 100644 (file)
--- a/src/libadmin/pts/afs_ptsAdmin.c
+++ b/src/libadmin/pts/afs_ptsAdmin.c
@@ -342,7 +342,7 @@ TranslatePTSIds(const afs_cell_handle_p cellHandle, namelist * names,
     int rc = 0;
     afs_status_t tst = 0;
 
-    tst = ubik_PR_IDToName(cellHandle->pts, 0, ids, names);
+    tst = string_PR_IDToName(cellHandle->pts, 0, ids, names);
 
     if (tst) {
        goto fail_TranslatePTSIds;

diff --git a/src/libafsauthent/afsauthent.def b/src/libafsauthent/afsauthent.def
index 40186f02a1590acf0e093d1720568f7fecf416f7..2aa445ea2da15d57275cad91fd4d818367658d61 100644 (file)
--- a/src/libafsauthent/afsauthent.def
+++ b/src/libafsauthent/afsauthent.def
@@ -166,3 +166,4 @@ EXPORTS
         afsconf_typedKey_values                         @165
         afsconf_GetAllKeys                              @166
        afsconf_CheckRestrictedQuery                    @167
+        string_PR_IDToName                             @168

diff --git a/src/libafsauthent/libafsauthent.la.sym b/src/libafsauthent/libafsauthent.la.sym
index 435495559a3f601d2834602fc1437e3ef8ea6659..02914924b3a5c73ad5f44d6bcae0b736030ce565 100644 (file)
--- a/src/libafsauthent/libafsauthent.la.sym
+++ b/src/libafsauthent/libafsauthent.la.sym
@@ -62,6 +62,7 @@ pr_ListMembers
 pr_NameToId
 pr_SNameToId
 setpag
+string_PR_IDToName
 ubik_Call
 ubik_CallIter
 ubik_Call_New

diff --git a/src/ptserver/liboafs_prot.la.sym b/src/ptserver/liboafs_prot.la.sym
index dc6c78265472b054b7154feb71c3ef8f071e3568..2efa03c16e5859cc1b4b508780b60176ff3a038b 100644 (file)
--- a/src/ptserver/liboafs_prot.la.sym
+++ b/src/ptserver/liboafs_prot.la.sym
@@ -29,6 +29,7 @@ pr_SetFieldsEntry
 pr_SetMaxGroupId
 pr_SetMaxUserId
 pruclient
+string_PR_IDToName
 ubik_PR_AddToGroup
 ubik_PR_Delete
 ubik_PR_GetCPS

diff --git a/src/ptserver/pterror.et b/src/ptserver/pterror.et
index ab3f1e846648776f0d8a670254aaa55789a3fd4f..ac3ea8380628fc6406127aec307e47ae37dde077 100644 (file)
--- a/src/ptserver/pterror.et
+++ b/src/ptserver/pterror.et
@@ -31,4 +31,5 @@ error_table PT
        ec PRTOOMANY, "too many elements in group"
        ec PRNOMEM, "malloc failed to alloc enough memory"
        ec PRINTERNAL, "Protection library internal error"
+       ec PRNAMETOOLONG, "name is too long (maximum 63 characters)"
 end

diff --git a/src/ptserver/ptserver.h b/src/ptserver/ptserver.h
index 09251b84422014714936d86e8b3bcf016c303275..3fd975cbc530f3053bbc2e1ff2ae02fa1f09e504 100644 (file)
--- a/src/ptserver/ptserver.h
+++ b/src/ptserver/ptserver.h
@@ -9,6 +9,11 @@
 
 #include "ptint.h"
 
+/* A sanitized version of a routine prototyped in ptint.h.  The implementation
+ * is in ptuser.c, but putting the declaration in ptuser.h breaks things. */
+extern int string_PR_IDToName(struct ubik_client *client, afs_int32 flags,
+                             idlist *ids, namelist *names) AFS_NONNULL();
+
 #define        PRSRV           73
 
 #define        ENTRYSIZE       192

diff --git a/src/ptserver/ptuser.c b/src/ptserver/ptuser.c
index 4296149c9bcab5644d24ee3893b3ca325b46670a..6296cb4e7e6d1432b03b185e17104d1580f6bcf8 100644 (file)
--- a/src/ptserver/ptuser.c
+++ b/src/ptserver/ptuser.c
@@ -332,22 +332,40 @@ pr_End(void)
     return code;
 }
 
-
+/*
+ * Make sure that arg is a proper C string that fits in a prname.
+ * If strnlen(arg, PR_MAXNAMELEN) == PR_MAXNAMELEN, then arg either
+ * doesn't have a terminating NUL or is too long, and we can't tell
+ * which one in the current API.  This code has always assumed that
+ * the names presented to it are valid C strings, but for robustness
+ * we can't depend on the server side guaranteeing that.  Unfortunately,
+ * the wire protocol uses a vector[PR_MAXNAMELEN] of char, so XDR will
+ * not automatically fix up strings generated by the server.
+ *
+ * The inequality is just belt-and-suspenders and should be impossible.
+ */
+static_inline int check_length(prname arg)
+{
+    if (strnlen(arg, PR_MAXNAMELEN) >= PR_MAXNAMELEN)
+       return PRNAMETOOLONG;
+    return 0;
+}
 
 int
 pr_CreateUser(prname name, afs_int32 *id)
 {
     afs_int32 code;
 
+    code = check_length(name);
+    if (code)
+       return code;
     stolower(name);
     if (*id) {
        code = ubik_PR_INewEntry(pruclient, 0, name, *id, 0);
-       return code;
     } else {
        code = ubik_PR_NewEntry(pruclient, 0, name, 0, 0, id);
-       return code;
     }
-
+    return code;
 }
 
 int
@@ -357,6 +375,10 @@ pr_CreateGroup(prname name, prname owner, afs_int32 *id)
     afs_int32 oid = 0;
     afs_int32 flags = 0;
 
+    code = check_length(name);
+    if (code)
+       return code;
+    /* pr_SNameToId will check owner's length. */
     stolower(name);
     if (owner) {
        code = pr_SNameToId(owner, &oid);
@@ -368,20 +390,19 @@ pr_CreateGroup(prname name, prname owner, afs_int32 *id)
     flags |= PRGRP;
     if (*id) {
        code = ubik_PR_INewEntry(pruclient, 0, name, *id, oid);
-       return code;
     } else {
        code = ubik_PR_NewEntry(pruclient, 0, name, flags, oid, id);
-       return code;
     }
+    return code;
 }
 
 int
-pr_Delete(char *name)
+pr_Delete(prname name)
 {
     afs_int32 code;
     afs_int32 id;
 
-    stolower(name);
+    /* pr_SNameToId both checks the length of name and lowercases it. */
     code = pr_SNameToId(name, &id);
     if (code)
        return code;
@@ -401,12 +422,18 @@ pr_DeleteByID(afs_int32 id)
 }
 
 int
-pr_AddToGroup(char *user, char *group)
+pr_AddToGroup(prname user, prname group)
 {
     afs_int32 code;
     namelist lnames;
     idlist lids;
 
+    code = check_length(user);
+    if (code)
+       return code;
+    code = check_length(group);
+    if (code)
+       return code;
     lnames.namelist_len = 2;
     lnames.namelist_val = malloc(2 * PR_MAXNAMELEN);
     strncpy(lnames.namelist_val[0], user, PR_MAXNAMELEN);
@@ -434,12 +461,18 @@ pr_AddToGroup(char *user, char *group)
 }
 
 int
-pr_RemoveUserFromGroup(char *user, char *group)
+pr_RemoveUserFromGroup(prname user, prname group)
 {
     afs_int32 code;
     namelist lnames;
     idlist lids;
 
+    code = check_length(user);
+    if (code)
+       return code;
+    code = check_length(group);
+    if (code)
+       return code;
     lnames.namelist_len = 2;
     lnames.namelist_val = malloc(2 * PR_MAXNAMELEN);
     strncpy(lnames.namelist_val[0], user, PR_MAXNAMELEN);
@@ -473,8 +506,12 @@ pr_NameToId(namelist *names, idlist *ids)
     afs_int32 code;
     afs_int32 i;
 
-    for (i = 0; i < names->namelist_len; i++)
+    for (i = 0; i < names->namelist_len; i++) {
+       code = check_length(names->namelist_val[i]);
+       if (code)
+           return code;
        stolower(names->namelist_val[i]);
+    }
     code = ubik_PR_NameToID(pruclient, 0, names, ids);
     return code;
 }
@@ -486,6 +523,9 @@ pr_SNameToId(prname name, afs_int32 *id)
     idlist lids;
     afs_int32 code;
 
+    code = check_length(name);
+    if (code)
+       return code;
     lids.idlist_len = 0;
     lids.idlist_val = 0;
     lnames.namelist_len = 1;
@@ -504,15 +544,35 @@ pr_SNameToId(prname name, afs_int32 *id)
     return code;
 }
 
+/*
+ * Like ubik_PR_IDToName, but enforces that the output prnames are
+ * interpretable as C strings (i.e., NUL-terminated).
+ */
 int
-pr_IdToName(idlist *ids, namelist *names)
+string_PR_IDToName(struct ubik_client *client, afs_int32 flags,
+                  idlist *ids, namelist *names)
 {
     afs_int32 code;
+    int i;
 
-    code = ubik_PR_IDToName(pruclient, 0, ids, names);
+    code = ubik_PR_IDToName(client, flags, ids, names);
+    if (code)
+       return code;
+    for (i = 0; i < names->namelist_len; i++) {
+       code = check_length(names->namelist_val[i]);
+       if (code)
+           return code;
+    }
     return code;
 }
 
+
+int
+pr_IdToName(idlist *ids, namelist *names)
+{
+    return string_PR_IDToName(pruclient, 0, ids, names);
+}
+
 int
 pr_SIdToName(afs_int32 id, prname name)
 {
@@ -525,8 +585,7 @@ pr_SIdToName(afs_int32 id, prname name)
     *lids.idlist_val = id;
     lnames.namelist_len = 0;
     lnames.namelist_val = 0;
-    code = ubik_PR_IDToName(pruclient, 0, &lids, &lnames);
-
+    code = pr_IdToName(&lids, &lnames);
     if (lnames.namelist_val)
        strncpy(name, lnames.namelist_val[0], PR_MAXNAMELEN);
     else if (code == 0)
@@ -599,19 +658,28 @@ pr_GetHostCPS(afs_uint32 host, prlist *CPS)
 }
 
 int
-pr_ListMembers(char *group, namelist *lnames)
+pr_ListMembers(prname group, namelist *lnames)
 {
     afs_int32 code;
     afs_int32 gid;
+    int i;
 
     memset(lnames, 0, sizeof(namelist));
 
+    /* pr_SNameToId checks the length of group. */
     code = pr_SNameToId(group, &gid);
     if (code)
        return code;
     if (gid == ANONYMOUSID)
        return PRNOENT;
     code = pr_IDListMembers(gid, lnames);
+    if (code)
+       return code;
+    for (i = 0; i < lnames->namelist_len; i++) {
+       code = check_length(lnames->namelist_val[i]);
+       if (code)
+           return code;
+    }
     return code;
 }
 
@@ -773,13 +841,16 @@ pr_ListEntry(afs_int32 id, struct prcheckentry *aentry)
     afs_int32 code;
 
     code = ubik_PR_ListEntry(pruclient, 0, id, aentry);
-    return code;
+    if (code)
+       return code;
+    return check_length(aentry->name);
 }
 
 afs_int32
 pr_ListEntries(int flag, afs_int32 startindex, afs_int32 *nentries, struct prlistentries **entries, afs_int32 *nextstartindex)
 {
     afs_int32 code;
+    int i;
     prentries bulkentries;
 
     *nentries = 0;
@@ -791,18 +862,33 @@ pr_ListEntries(int flag, afs_int32 startindex, afs_int32 *nentries, struct prlis
     code =
        ubik_PR_ListEntries(pruclient, 0, flag, startindex,
                  &bulkentries, nextstartindex);
-    *nentries = bulkentries.prentries_len;
-    *entries = bulkentries.prentries_val;
+    if (code)
+       return code;
+    for (i = 0; i < bulkentries.prentries_len; i++) {
+       /* XXX should we try to return all the other entries? */
+       code = check_length(bulkentries.prentries_val[i].name);
+       if (code)
+           goto out;
+    }
+
+out:
+    if (code != 0) {
+       xdr_free((xdrproc_t)xdr_prentries, &bulkentries);
+    } else {
+       *nentries = bulkentries.prentries_len;
+       *entries = bulkentries.prentries_val;
+    }
     return code;
 }
 
 int
-pr_CheckEntryByName(char *name, afs_int32 *id, char *owner, char *creator)
+pr_CheckEntryByName(prname name, afs_int32 *id, prname owner, prname creator)
 {
     /* struct prcheckentry returns other things, which aren't useful to show at this time. */
     afs_int32 code;
     struct prcheckentry aentry;
 
+    /* pr_SNameToId will check name's length. */
     code = pr_SNameToId(name, id);
     if (code)
        return code;
@@ -822,12 +908,13 @@ pr_CheckEntryByName(char *name, afs_int32 *id, char *owner, char *creator)
 }
 
 int
-pr_CheckEntryById(char *name, afs_int32 id, char *owner, char *creator)
+pr_CheckEntryById(prname name, afs_int32 id, prname owner, prname creator)
 {
     /* struct prcheckentry returns other things, which aren't useful to show at this time. */
     afs_int32 code;
     struct prcheckentry aentry;
 
+    /* XXX ListEntry RPC gives us the name back so should avoid extra RPC */
     code = pr_SIdToName(id, name);
     if (code)
        return code;
@@ -847,12 +934,13 @@ pr_CheckEntryById(char *name, afs_int32 id, char *owner, char *creator)
 }
 
 int
-pr_ChangeEntry(char *oldname, char *newname, afs_int32 *newid, char *newowner)
+pr_ChangeEntry(prname oldname, prname newname, afs_int32 *newid, prname newowner)
 {
     afs_int32 code;
     afs_int32 id;
     afs_int32 oid = 0;
 
+    /* pr_SNameToId takes care of length checks for us. */
     code = pr_SNameToId(oldname, &id);
     if (code)
        return code;
@@ -873,12 +961,18 @@ pr_ChangeEntry(char *oldname, char *newname, afs_int32 *newid, char *newowner)
 }
 
 int
-pr_IsAMemberOf(char *uname, char *gname, afs_int32 *flag)
+pr_IsAMemberOf(prname uname, prname gname, afs_int32 *flag)
 {
     afs_int32 code;
     namelist lnames;
     idlist lids;
 
+    code = check_length(uname);
+    if (code)
+       return code;
+    code = check_length(gname);
+    if (code)
+       return code;
     stolower(uname);
     stolower(gname);
     lnames.namelist_len = 2;

diff --git a/src/ptserver/ptuser.h b/src/ptserver/ptuser.h
index 8a1ed8b05db79e3c81c3748d8b16402bb6d1d2cb..6e1ebff341a79b5f3878ee5cb0105fa68199f3ae 100644 (file)
--- a/src/ptserver/ptuser.h
+++ b/src/ptserver/ptuser.h
@@ -19,10 +19,10 @@ extern int pr_End(void);
 extern int pr_CreateUser(prname name, afs_int32 *id) AFS_NONNULL();
 extern int pr_CreateGroup(prname name, prname owner,
                          afs_int32 *id) AFS_NONNULL((1,3));
-extern int pr_Delete(char *name) AFS_NONNULL();
+extern int pr_Delete(prname name) AFS_NONNULL();
 extern int pr_DeleteByID(afs_int32 id);
-extern int pr_AddToGroup(char *user, char *group) AFS_NONNULL();
-extern int pr_RemoveUserFromGroup(char *user, char *group) AFS_NONNULL();
+extern int pr_AddToGroup(prname user, prname group) AFS_NONNULL();
+extern int pr_RemoveUserFromGroup(prname user, prname group) AFS_NONNULL();
 extern int pr_NameToId(namelist *names, idlist *ids) AFS_NONNULL();
 extern int pr_SNameToId(prname name, afs_int32 *id) AFS_NONNULL();
 extern int pr_IdToName(idlist *ids, namelist *names) AFS_NONNULL();
@@ -30,7 +30,7 @@ extern int pr_SIdToName(afs_int32 id, prname name) AFS_NONNULL();
 extern int pr_GetCPS(afs_int32 id, prlist *CPS) AFS_NONNULL();
 extern int pr_GetCPS2(afs_int32 id, afs_uint32 host, prlist *CPS) AFS_NONNULL();
 extern int pr_GetHostCPS(afs_uint32 host, prlist *CPS) AFS_NONNULL();
-extern int pr_ListMembers(char *group, namelist *lnames) AFS_NONNULL();
+extern int pr_ListMembers(prname group, namelist *lnames) AFS_NONNULL();
 extern int pr_ListOwned(afs_int32 oid, namelist *lnames, afs_int32 *moreP)
                       AFS_NONNULL();
 extern int pr_IDListMembers(afs_int32 gid, namelist *lnames) AFS_NONNULL();
@@ -40,13 +40,13 @@ extern afs_int32 pr_ListEntries(int flag, afs_int32 startindex,
                                afs_int32 *nentries,
                                struct prlistentries **entries,
                                afs_int32 *nextstartindex) AFS_NONNULL();
-extern int pr_CheckEntryByName(char *name, afs_int32 *id, char *owner,
-                              char *creator) AFS_NONNULL();
-extern int pr_CheckEntryById(char *name, afs_int32 id, char *owner,
-                            char *creator) AFS_NONNULL();
-extern int pr_ChangeEntry(char *oldname, char *newname, afs_int32 *newid,
-                         char *newowner) AFS_NONNULL((1));
-extern int pr_IsAMemberOf(char *uname, char *gname, afs_int32 *flag)
+extern int pr_CheckEntryByName(prname name, afs_int32 *id, prname owner,
+                              prname creator) AFS_NONNULL();
+extern int pr_CheckEntryById(prname name, afs_int32 id, prname owner,
+                            prname creator) AFS_NONNULL();
+extern int pr_ChangeEntry(prname oldname, prname newname, afs_int32 *newid,
+                         prname newowner) AFS_NONNULL((1));
+extern int pr_IsAMemberOf(prname uname, prname gname, afs_int32 *flag)
                         AFS_NONNULL();
 extern int pr_ListMaxUserId(afs_int32 *mid) AFS_NONNULL();
 extern int pr_SetMaxUserId(afs_int32 mid);

diff --git a/src/viced/host.c b/src/viced/host.c
index f53a415f491a448222bfd57cb59cd73212814236..2b21a85c775397401d75bccc972a7818d86bfc80 100644 (file)
--- a/src/viced/host.c
+++ b/src/viced/host.c
@@ -411,7 +411,7 @@ hpr_IdToName(idlist *ids, namelist *names)
     if (code)
        return code;
 
-    code = ubik_PR_IDToName(uclient, 0, ids, names);
+    code = string_PR_IDToName(uclient, 0, ids, names);
     return code;
 }