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.

diff --git a/src/WINNT/afsd/fs.c b/src/WINNT/afsd/fs.c
index e7c987d7c03d87968d2f6c41fd798711a2d2b1e7..076cc32f94009408a0bebc278be5f9a50b484f05 100644 (file)
--- 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;i<nplus;i++) {
-        sscanf(astr, "%100s %d", tname, &trights);
+        ret = sscanf(astr, "%100s %d", tname, &trights); 
+        if (ret <= 0)
+            goto nplus_err;
         astr = SkipLine(astr);
         tl = (struct AclEntry *) malloc(sizeof (struct AclEntry));
-        assert(tl);
+        if (tl == NULL)
+            goto nplus_err;
         if (!first) 
             first = tl;
         strcpy(tl->name, tname);
@@ -579,10 +594,13 @@ ParseAcl (char *astr)
     last = 0;
     first = 0;
     for(i=0;i<nminus;i++) {
-        sscanf(astr, "%100s %d", tname, &trights);
+        ret = sscanf(astr, "%100s %d", tname, &trights);
+        if (ret <= 0)
+            goto nminus_err;
         astr = SkipLine(astr);
         tl = (struct AclEntry *) malloc(sizeof (struct AclEntry));
-        assert(tl);
+        if (tl == NULL)
+            goto nminus_err;
         if (!first) 
             first = tl;
         strcpy(tl->name, 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 704d4ece6e1ef885056752556058fcd4c14dfd9c..9c1d3b7e9e9894f3650f48b8b6af2eb978729bb3 100644 (file)
--- 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;i<nplus;i++) {
+        ret = sscanf(astr, "%100s %d", tname, &trights); 
+        if (ret <= 0)
+            goto nplus_err;
         astr = SkipLine(astr);
         tl = (struct AclEntry *) malloc(sizeof (struct AclEntry));
-        if (!first)
-                       first = tl;
+        if (tl == NULL)
+            goto nplus_err;
+        if (!first) 
+            first = tl;
         strcpy(tl->name, 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;i<nminus;i++) {
+        ret = sscanf(astr, "%100s %d", tname, &trights);
+        if (ret <= 0)
+            goto nminus_err;
         astr = SkipLine(astr);
         tl = (struct AclEntry *) malloc(sizeof (struct AclEntry));
+        if (tl == NULL)
+            goto nminus_err;
         if (!first) 
-                       first = tl;
+            first = tl;
         strcpy(tl->name, 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 8c909435bf726e80e4a2ce8e7e5f5186a8382870..b7c9abcba18d6b767f31d0e073b9a3cfa234fa89 100644 (file)
--- 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 9a381ca96d09dd33674425131a4dc04f7411f956..3b24ef45fdcdcb47307c7cfa742b73748c00fe5a 100644 (file)
--- 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