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 e7c987d..076cc32 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 704d4ec..9c1d3b7 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 8c90943..b7c9abc 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 9a381ca..3b24ef4 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