From a0417565a3ab7e6a49d7c48efd72d62bdeb4436c Mon Sep 17 00:00:00 2001 From: Garrett Wollman Date: Sat, 28 Jul 2012 18:35:13 -0400 Subject: [PATCH] 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 Reviewed-by: Benjamin Kaduk --- src/libadmin/pts/afs_ptsAdmin.c | 2 +- src/libafsauthent/afsauthent.def | 1 + src/libafsauthent/libafsauthent.la.sym | 1 + src/ptserver/liboafs_prot.la.sym | 1 + src/ptserver/pterror.et | 1 + src/ptserver/ptserver.h | 5 + src/ptserver/ptuser.c | 140 +++++++++++++++++++++---- src/ptserver/ptuser.h | 22 ++-- src/viced/host.c | 2 +- 9 files changed, 139 insertions(+), 36 deletions(-) diff --git a/src/libadmin/pts/afs_ptsAdmin.c b/src/libadmin/pts/afs_ptsAdmin.c index 845bab812..80f7d4ccf 100644 --- 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 40186f02a..2aa445ea2 100644 --- 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 435495559..02914924b 100644 --- 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 dc6c78265..2efa03c16 100644 --- 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 ab3f1e846..ac3ea8380 100644 --- 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 09251b844..3fd975cbc 100644 --- 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 4296149c9..6296cb4e7 100644 --- 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 8a1ed8b05..6e1ebff34 100644 --- 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 f53a415f4..2b21a85c7 100644 --- 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; } -- 2.39.5