From e6d7e165886122dc4cee850632ee2c1f55165258 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Wed, 19 Mar 2008 13:22:02 +0000 Subject: [PATCH] windows-parseacl-20080319 LICENSE MIT Protect against invalid data being passed into ParseAcl and corrupting the stack. This affects both fs.exe and the explorer shell extension. Windows Error Reporting in recent weeks has begun to report several instances of stack corruption in the explorer shell extension from Denmark and Germany. --- src/WINNT/afsd/fs.c | 100 +++++++++++++++--- src/WINNT/client_exp/gui2fs.cpp | 98 ++++++++++++++--- .../client_exp/lang/en_US/afs_shl_ext.rc | 1 + src/WINNT/client_exp/resource.h | 1 + 4 files changed, 170 insertions(+), 30 deletions(-) diff --git a/src/WINNT/afsd/fs.c b/src/WINNT/afsd/fs.c index e7c987d7c..076cc32f9 100644 --- a/src/WINNT/afsd/fs.c +++ b/src/WINNT/afsd/fs.c @@ -535,24 +535,36 @@ EmptyAcl(char *astr) tp->nplus = tp->nminus = 0; tp->pluslist = tp->minuslist = 0; tp->dfs = 0; - sscanf(astr, "%d dfs:%d %s", &junk, &tp->dfs, tp->cell); + if (astr == NULL || sscanf(astr, "%d dfs:%d %s", &junk, &tp->dfs, tp->cell) <= 0) { + tp->dfs = 0; + tp->cell[0] = '\0'; + } return tp; } static struct Acl * ParseAcl (char *astr) { - int nplus, nminus, i, trights; + int nplus, nminus, i, trights, ret; char tname[MAXNAME]; - struct AclEntry *first, *last, *tl; + struct AclEntry *first, *next, *last, *tl; struct Acl *ta; - ta = (struct Acl *) malloc (sizeof (struct Acl)); - assert(ta); - ta->dfs = 0; - sscanf(astr, "%d dfs:%d %s", &ta->nplus, &ta->dfs, ta->cell); + ta = EmptyAcl(NULL); + if (astr == NULL || strlen(astr) == 0) + return ta; + + ret = sscanf(astr, "%d dfs:%d %s", &ta->nplus, &ta->dfs, ta->cell); + if (ret <= 0) { + free(ta); + return NULL; + } astr = SkipLine(astr); - sscanf(astr, "%d", &ta->nminus); + ret = sscanf(astr, "%d", &ta->nminus); + if (ret <= 0) { + free(ta); + return NULL; + } astr = SkipLine(astr); nplus = ta->nplus; @@ -561,10 +573,13 @@ ParseAcl (char *astr) last = 0; first = 0; for(i=0;iname, tname); @@ -579,10 +594,13 @@ ParseAcl (char *astr) last = 0; first = 0; for(i=0;iname, tname); @@ -594,7 +612,23 @@ ParseAcl (char *astr) } ta->minuslist = first; + exit: return ta; + + nminus_err: + for (;first; first = next) { + next = first->next; + free(first); + } + first = ta->pluslist; + + nplus_err: + for (;first; first = next) { + next = first->next; + free(first); + } + free(ta); + return NULL; } static int @@ -904,6 +938,13 @@ SetACLCmd(struct cmd_syndesc *as, void *arock) if (ta) ZapAcl(ta); ta = ParseAcl(space); + if (!ta) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + ti->data); + error = 1; + continue; + } if (!plusp && ta->dfs) { fprintf(stderr, "fs: %s: you may not use the -negative switch with DFS acl's.\n%s", @@ -918,6 +959,13 @@ SetACLCmd(struct cmd_syndesc *as, void *arock) ta = EmptyAcl(space); else ta = ParseAcl(space); + if (!ta) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + ti->data); + error = 1; + continue; + } CleanAcl(ta, ti->data); for(ui=as->parms[1].items; ui; ui=ui->next->next) { enum rtype rtype; @@ -1027,6 +1075,12 @@ CopyACLCmd(struct cmd_syndesc *as, void *arock) return 1; } fa = ParseAcl(space); + if (!fa) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + as->parms[0].items->data); + return 1; + } CleanAcl(fa, as->parms[0].items->data); for (ti=as->parms[1].items; ti;ti=ti->next) { blob.out_size = MAXSIZE; @@ -1044,6 +1098,13 @@ CopyACLCmd(struct cmd_syndesc *as, void *arock) ta = EmptyAcl(space); else ta = ParseAcl(space); + if (!ta) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + ti->data); + error = 1; + continue; + } CleanAcl(ta, ti->data); if (ta->dfs != fa->dfs) { fprintf(stderr, @@ -1206,7 +1267,13 @@ CleanACLCmd(struct cmd_syndesc *as, void *arock) if (ta) ZapAcl(ta); ta = ParseAcl(space); - + if (!ta) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + ti->data); + error = 1; + continue; + } if (ta->dfs) { fprintf(stderr, "%s: cleanacl is not supported for DFS access lists.\n", @@ -1292,6 +1359,13 @@ ListACLCmd(struct cmd_syndesc *as, void *arock) continue; } ta = ParseAcl(space); + if (!ta) { + fprintf(stderr, + "fs: %s: invalid acl data returned from VIOCGETAL\n", + ti->data); + error = 1; + continue; + } switch (ta->dfs) { case 0: printf("Access list for %s is\n", ti->data); diff --git a/src/WINNT/client_exp/gui2fs.cpp b/src/WINNT/client_exp/gui2fs.cpp index 704d4ece6..9c1d3b7e9 100644 --- a/src/WINNT/client_exp/gui2fs.cpp +++ b/src/WINNT/client_exp/gui2fs.cpp @@ -557,18 +557,45 @@ struct Acl *EmptyAcl(const CString& strCellName) return tp; } -struct Acl *ParseAcl(char *astr) +struct Acl *EmptyAcl(char *astr) { - int nplus, nminus, i, trights; + register struct Acl *tp; + int junk; + + tp = (struct Acl *)malloc(sizeof (struct Acl)); + tp->nplus = tp->nminus = 0; + tp->pluslist = tp->minuslist = 0; + tp->dfs = 0; + if (astr == NULL || sscanf(astr, "%d dfs:%d %s", &junk, &tp->dfs, tp->cell) <= 0) { + tp->dfs = 0; + tp->cell[0] = '\0'; + } + return tp; +} + +struct Acl * +ParseAcl (char *astr) +{ + int nplus, nminus, i, trights, ret; char tname[MAXNAME]; - struct AclEntry *first, *last, *tl; + struct AclEntry *first, *next, *last, *tl; struct Acl *ta; - ta = (struct Acl *) malloc (sizeof (struct Acl)); - ta->dfs = 0; - sscanf(astr, "%d dfs:%d %s", &ta->nplus, &ta->dfs, ta->cell); + ta = EmptyAcl(NULL); + if (astr == NULL || strlen(astr) == 0) + return ta; + + ret = sscanf(astr, "%d dfs:%d %s", &ta->nplus, &ta->dfs, ta->cell); + if (ret <= 0) { + free(ta); + return NULL; + } astr = SkipLine(astr); - sscanf(astr, "%d", &ta->nminus); + ret = sscanf(astr, "%d", &ta->nminus); + if (ret <= 0) { + free(ta); + return NULL; + } astr = SkipLine(astr); nplus = ta->nplus; @@ -576,39 +603,63 @@ struct Acl *ParseAcl(char *astr) last = 0; first = 0; - for(i = 0; i < nplus; i++) { - sscanf(astr, "%100s %d", tname, &trights); + for(i=0;iname, tname); tl->rights = trights; tl->next = 0; - if (last) - last->next = tl; + if (last) + last->next = tl; last = tl; } ta->pluslist = first; last = 0; first = 0; - for(i=0; i < nminus; i++) { - sscanf(astr, "%100s %d", tname, &trights); + for(i=0;iname, tname); tl->rights = trights; tl->next = 0; if (last) - last->next = tl; + last->next = tl; last = tl; } ta->minuslist = first; + exit: return ta; + + nminus_err: + for (;first; first = next) { + next = first->next; + free(first); + } + first = ta->pluslist; + + nplus_err: + for (;first; first = next) { + next = first->next; + free(first); + } + free(ta); + return NULL; } /* clean up an access control list of its bad entries; return 1 if we made @@ -681,6 +732,10 @@ void CleanACL(CStringArray& names) } ta = ParseAcl(space); + if (ta == NULL) { + ShowMessageBox(IDS_INVALID_ACL_DATA, MB_ICONERROR, IDS_INVALID_ACL_DATA); + continue; + } if (ta->dfs) { ShowMessageBox(IDS_CLEANACL_NOT_SUPPORTED, MB_ICONERROR, IDS_CLEANACL_NOT_SUPPORTED, names[i]); continue; @@ -731,6 +786,10 @@ BOOL GetRights(const CString& strDir, CStringArray& strNormal, CStringArray& str } ta = ParseAcl(space); + if (ta == NULL) { + ShowMessageBox(IDS_INVALID_ACL_DATA, MB_ICONERROR, IDS_INVALID_ACL_DATA); + return FALSE; + } if (ta->dfs) { ShowMessageBox(IDS_DFSACL_ERROR, MB_ICONERROR, IDS_DFSACL_ERROR); return FALSE; @@ -927,6 +986,11 @@ BOOL CopyACL(const CString& strToDir, const CStringArray& normal, const CStringA else pToAcl = ParseAcl(space); + if (pToAcl == NULL) { + ShowMessageBox(IDS_INVALID_ACL_DATA, MB_ICONERROR, IDS_INVALID_ACL_DATA); + return FALSE; + } + CleanAcl(pToAcl); if (pToAcl->dfs) { diff --git a/src/WINNT/client_exp/lang/en_US/afs_shl_ext.rc b/src/WINNT/client_exp/lang/en_US/afs_shl_ext.rc index 8c909435b..b7c9abcba 100644 --- a/src/WINNT/client_exp/lang/en_US/afs_shl_ext.rc +++ b/src/WINNT/client_exp/lang/en_US/afs_shl_ext.rc @@ -602,6 +602,7 @@ BEGIN IDS_ALL_SERVERS_RUNNING "All servers are running." IDS_CHECK_VOLUMES_OK "All volumeID/name mappings checked." IDS_CHECK_VOLUMES_ERROR "Error checking volumeID/name mappings: %o" + IDS_INVALID_ACL_DATA "Internal Error: Invalid ACL data was received." END STRINGTABLE DISCARDABLE diff --git a/src/WINNT/client_exp/resource.h b/src/WINNT/client_exp/resource.h index 9a381ca96..3b24ef45f 100644 --- a/src/WINNT/client_exp/resource.h +++ b/src/WINNT/client_exp/resource.h @@ -74,6 +74,7 @@ #define IDS_ALL_SERVERS_RUNNING 65 #define IDS_CHECK_VOLUMES_OK 66 #define IDS_CHECK_VOLUMES_ERROR 67 +#define IDS_INVALID_ACL_DATA 68 #define IDS_ACL_ENTRY_NAME_IN_USE 80 #define IDS_REALLY_DEL_MOUNT_POINTS 81 -- 2.39.5