ptuser: guarantee that all names are valid C strings 96/7896/11
authorGarrett Wollman <wollman@csail.mit.edu>
Sat, 28 Jul 2012 22:35:13 +0000 (18:35 -0400)
committerBenjamin Kaduk <kaduk@mit.edu>
Sun, 17 Jul 2016 03:57:16 +0000 (23:57 -0400)
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>

src/libadmin/pts/afs_ptsAdmin.c
src/libafsauthent/afsauthent.def
src/libafsauthent/libafsauthent.la.sym
src/ptserver/liboafs_prot.la.sym
src/ptserver/pterror.et
src/ptserver/ptserver.h
src/ptserver/ptuser.c
src/ptserver/ptuser.h
src/viced/host.c

index 845bab8..80f7d4c 100644 (file)
@@ -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;
index 40186f0..2aa445e 100644 (file)
@@ -166,3 +166,4 @@ EXPORTS
         afsconf_typedKey_values                         @165
         afsconf_GetAllKeys                              @166
        afsconf_CheckRestrictedQuery                    @167
+        string_PR_IDToName                             @168
index 4354955..0291492 100644 (file)
@@ -62,6 +62,7 @@ pr_ListMembers
 pr_NameToId
 pr_SNameToId
 setpag
+string_PR_IDToName
 ubik_Call
 ubik_CallIter
 ubik_Call_New
index dc6c782..2efa03c 100644 (file)
@@ -29,6 +29,7 @@ pr_SetFieldsEntry
 pr_SetMaxGroupId
 pr_SetMaxUserId
 pruclient
+string_PR_IDToName
 ubik_PR_AddToGroup
 ubik_PR_Delete
 ubik_PR_GetCPS
index ab3f1e8..ac3ea83 100644 (file)
@@ -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
index 09251b8..3fd975c 100644 (file)
@@ -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
index 4296149..6296cb4 100644 (file)
@@ -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;
index 8a1ed8b..6e1ebff 100644 (file)
@@ -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);
index f53a415..2b21a85 100644 (file)
@@ -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;
 }