[Fastjar-dev] [PATCH] Directory traversal flaw in fastjar

[Jakub Jelinek]

Hi!

Dan Rosenberg reported a directory traversal flaw in fastjar that allows an
attacker, who is able to convince a victim to extract a malicious .jar file, to
overwrite arbitrary files on disk without prompting the victim.  The files to
be overwritten must be writable by the user extracting the .jar file.

For more info see:
https://launchpad.net/bugs/540575
CVE-2010-0831

Clearly this is mainly because the previous CVE-2006-3619 fix has been buggy
- tmp_buff isn't just the current directory component, but all already
processed directory components including the current one.

This patch should fix that and make the code tiny bit more efficient.
The first hunk in the patch is from Dan's patch above.

2010-06-08  Jakub Jelinek  <address@hidden>
            Dan Rosenberg  <address@hidden>

        * jartool.c (extract_jar): Fix up checks for traversal to parent
        directories, disallow absolute paths, make the code slightly more
        efficient.

--- fastjar-0.98/jartool.c.jj   2009-09-07 00:10:47.000000000 +0200
+++ fastjar-0.98/jartool.c      2010-06-08 20:00:29.000000000 +0200
@@ -1730,7 +1730,17 @@ int extract_jar(int fd, const char **fil
       struct stat sbuf;
       int depth = 0;
 
-      tmp_buff = malloc(sizeof(char) * strlen((const char *)filename));
+      if(*filename == '/'){
+       fprintf(stderr, "Absolute path names are not allowed.\n");
+       exit(EXIT_FAILURE);
+      }
+
+      tmp_buff = malloc(strlen((const char *)filename));
+
+      if(tmp_buff == NULL) {
+       fprintf(stderr, "Out of memory.\n");
+       exit(EXIT_FAILURE);
+      }
 
       for(;;){
         const ub1 *idx = (const unsigned char *)strchr((const char *)start, 
'/');
@@ -1738,25 +1748,28 @@ int extract_jar(int fd, const char **fil
         if(idx == NULL)
           break;
         else if(idx == start){
+         tmp_buff[idx - filename] = '/';
           start++;
           continue;
         }
-        start = idx + 1;
 
-        strncpy(tmp_buff, (const char *)filename, (idx - filename));
-        tmp_buff[(idx - filename)] = '\0';
+       memcpy(tmp_buff + (start - filename), (const char *)start, (idx - 
start));
+       tmp_buff[idx - filename] = '\0';
 
 #ifdef DEBUG    
         printf("checking the existance of %s\n", tmp_buff);
 #endif
-       if(strcmp(tmp_buff, "..") == 0){
+       if(idx - start == 2 && memcmp(start, "..", 2) == 0){
          --depth;
          if (depth < 0){
            fprintf(stderr, "Traversal to parent directories during 
unpacking!\n");
            exit(EXIT_FAILURE);
          }
-       } else if (strcmp(tmp_buff, ".") != 0)
+       } else if (idx - start != 1 || *start != '.')
          ++depth;
+
+        start = idx + 1;
+
         if(stat(tmp_buff, &sbuf) < 0){
           if(errno != ENOENT)
             exit_on_error("stat");
@@ -1765,6 +1778,7 @@ int extract_jar(int fd, const char **fil
 #ifdef DEBUG    
           printf("Directory exists\n");
 #endif
+         tmp_buff[idx - filename] = '/';
           continue;
         }else {
           fprintf(stderr, "Hmmm.. %s exists but isn't a directory!\n",
@@ -1781,10 +1795,11 @@ int extract_jar(int fd, const char **fil
         if(verbose && handle)
           printf("%10s: %s/\n", "created", tmp_buff);
 
+       tmp_buff[idx - filename] = '/';
       }
 
       /* only a directory */
-      if(strlen((const char *)start) == 0)
+      if(*start == '\0')
         dir = TRUE;
 
 #ifdef DEBUG    
@@ -1792,7 +1807,7 @@ int extract_jar(int fd, const char **fil
 #endif
 
       /* If the entry was just a directory, don't write to file, etc */
-      if(strlen((const char *)start) == 0)
+      if(*start == '\0')
         f_fd = -1;
 
       free(tmp_buff);
@@ -1876,7 +1891,8 @@ int extract_jar(int fd, const char **fil
       exit(EXIT_FAILURE);
     }
 
-    close(f_fd);
+    if (f_fd != -1)
+      close(f_fd);
 
     if(verbose && dir == FALSE && handle)
       printf("%10s: %s\n",


        Jakub