httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boya Sun <bxs...@case.edu>
Subject four potential bugs discovered from CWRU research group
Date Sat, 22 Aug 2009 16:04:40 GMT
Dear Apache-httpd programmers:

This is Boya Sun from Case Western Reserve University.  I have sent you some
potential bugs we discovered in our recent research a few days ago, but I
haven’t got any response yet, so I tried to organize the bugs and
resubmitted these 4 potential bugs.  I rewrite the potential bugs and the
potential fixes in the form of patches.  Most of the patches are against the
trunk (revision 806655), except for the second one, which is against the
branch of 2.2.x at revision 806782. The patch for BUG2 is not a real bug
fix, but just some comments indicating where the missing code should be
added, since I am not exactly sure how to fix the bug.

I STRONGLY RECOMMEND you to go over these potential bugs, since these
potential bugs are very similar to some previous bugs in your issue DB or
some revisions that looks like bug-fix, which provide strong evidence that
these potential bugs are real ones.

In order to make it easier to understand, for each bug we discovered, I also
show the original bug-fix which we used to discover the new bugs.

I would REALLY appreciate that you could help us confirm whether these
bug-fixes are valid or not, since this is the ONLY way for us to know
whether our approach of discovering new bugs works.

Thanks very much in advance!  And enjoy viewing the bugs……:-)

BUG1:
Description: This bug was found by analyzing bug 31440  (
https://issues.apache.org/bugzilla/show_bug.cgi?id=31440);  this fix
replaced “srand((int) time((time_t *) NULL))” function with “seed_rand()” in
order to improve rand seed generation under the cases “ALG_APMD5” and
“ALG_CRYPT”.
We have found that in the file “htdbm.c”, there are code segments that are
very similar to the bug being fixed in 31440.  We believe that in this file,
“srand((int) time((time_t *) NULL))” should also be replaced with
“seed_rand()” for cases “ALG_APMD5” and “ALG_CRYPT”

**********************************original
bug-fix*************************************
--- htpasswd.c (revision 629163)
+++ htpasswd.c (revision 629164)
@@ -126,6 +126,18 @@
     }
 }

+static apr_status_t seed_rand()
+{
+    int seed = 0;
+    apr_status_t rv;
+    rv = apr_generate_random_bytes((unsigned char*) &seed, sizeof(seed));
+    if (rv) {
+        apr_file_printf(errfile, "Unable to generate random bytes: %pm" NL,
rv);
+        return rv;
+    }
+    srand(seed);
+    return rv;
+}

 static void putline(apr_file_t *f, const char *l)
 {
@@ -174,7 +186,9 @@
         break;

     case ALG_APMD5:
-        (void) srand((int) time((time_t *) NULL));
+        if (seed_rand()) {
+            break;
+        }
         generate_salt(&salt[0], 8);
         salt[8] = '\0';

@@ -190,7 +204,9 @@
 #if (!(defined(WIN32) || defined(TPF) || defined(NETWARE)))
     case ALG_CRYPT:
     default:
-        (void) srand((int) time((time_t *) NULL));
+        if (seed_rand()) {
+            break;
+        }
         to64(&salt[0], rand(), 8);
         salt[8] = '\0';

*****************************discovered potential bug and possible
fix*********************
--- support/htdbm.c (revision 806655)
+++ support/htdbm.c (working copy)
@@ -298,7 +298,9 @@
         break;

         case ALG_APMD5:
-            (void) srand((int) time((time_t *) NULL));
+            if (seed_rand()) {
+                break;
+   }
             to64(&salt[0], rand(), 8);
             salt[8] = '\0';
             apr_md5_encode((const char *)htdbm->userpass, (const char
*)salt,
@@ -314,7 +316,9 @@
         break;
 #if (!(defined(WIN32) || defined(NETWARE)))
         case ALG_CRYPT:
-            (void) srand((int) time((time_t *) NULL));
+            if (seed_rand()) {
+                break;
+   }
             to64(&salt[0], rand(), 8);
             salt[8] = '\0';
             apr_cpystrn(cpw, crypt(htdbm->userpass, salt), sizeof(cpw) -
1);

BUG2:
Description: This bug was found by analyzing revision 602467 (
http://svn.apache.org/viewvc?view=rev&revision=602467)
The log of this revision is as follows:

" * core log.c: Work around possible solutions rejected by apr for the old
implementation of apr_proc_create(), and explicitly pass the output and
error channels to all log processes created. This goes all the way back to
piped logs failing to run on win32. Not in or needed at trunk/, as apr 1.3.0
has the proper fix."

Note that this bug seems to be particular to the 2.2.x branch, so the ewe
searched for similar bugs in <
https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x> at revision
806782 , instead ot the trunk.

>From the above bug-fix, it seems that before calling "apr_proc_create",
"apr_procattr_child_out_set" and "apr_procattr_child_err_set" should be
invoked on "procattr"; however, we have found a bug that these two functions
are missing where it seems to be appropriate.  In this bug, we think that
before calling "ap_os_create_privileged_process", the two functions
"apr_procattr_child_*_set" should be invoked.  Note that
"ap_os_create_privileged_process" called "apr_proc_create" inside the
function.
We have also found another evidence that shows that this is likely to be a
real bug,  that is, in another file modules/generators/mod_cgid.c, in
function "cgid_server", the two "apr_procattr_child_*_set" functions did
invoked before calling "ap_os_create_privileged_process".

The patch I attached is not a real bug-fix, but just some comments
indicating where the two missing functions should be added, since I am not
*exactly* sure how to fix the bug.

**********************************original
bug-fix*************************************
--- log.c (revision 602466)
+++ log.c (revision 602467)
@@ -263,7 +263,7 @@
     apr_status_t rc;
     apr_procattr_t *procattr;
     apr_proc_t *procnew;
-    apr_file_t *errfile;
+    apr_file_t *outfile, *errfile;

     if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
         && ((rc = apr_procattr_cmdtype_set(procattr,
@@ -282,8 +282,11 @@
         pname = apr_pstrdup(p, args[0]);
         procnew = (apr_proc_t *)apr_pcalloc(p, sizeof(*procnew));

-        if (dummy_stderr) {
-            if ((rc = apr_file_open_stdout(&errfile, p)) == APR_SUCCESS)
+        if ((rc = apr_file_open_stdout(&outfile, p)) == APR_SUCCESS) {
+            rc = apr_procattr_child_out_set(procattr, outfile, NULL);
+            if (dummy_stderr)
+                rc = apr_procattr_child_err_set(procattr, outfile, NULL);
+            else if ((rc = apr_file_open_stderr(&errfile, p)) ==
APR_SUCCESS)
                 rc = apr_procattr_child_err_set(procattr, errfile, NULL);
         }

@@ -887,7 +890,13 @@
     else {
         char **args;
         const char *pname;
+        apr_file_t *outfile, *errfile;

+        if ((status = apr_file_open_stdout(&outfile, pl->p)) ==
APR_SUCCESS)
+            status = apr_procattr_child_out_set(procattr, outfile, NULL);
+        if ((status = apr_file_open_stderr(&errfile, pl->p)) ==
APR_SUCCESS)
+            status = apr_procattr_child_err_set(procattr, errfile, NULL);
+
         apr_tokenize_to_argv(pl->program, &args, pl->p);
         pname = apr_pstrdup(pl->p, args[0]);
         procnew = apr_pcalloc(pl->p, sizeof(apr_proc_t));

*****************************discovered potential bug and possible
fix*********************
--- modules/generators/mod_cgi.c (revision 806782)
+++ modules/generators/mod_cgi.c (working copy)
@@ -446,12 +446,16 @@
         ((rc = apr_procattr_addrspace_set(procattr,
                                         e_info->addrspace)) != APR_SUCCESS)
||
         ((rc = apr_procattr_child_errfn_set(procattr, cgi_child_errfn)) !=
APR_SUCCESS)) {
+     /*apr_procattr_child_err_set(procattr,...) and
+    apr_procattr_child_out_set(procattr,...) should be invoked here*/
+
         /* Something bad happened, tell the world. */
         ap_log_rerror(APLOG_MARK, APLOG_ERR, rc, r,
                       "couldn't set child process attributes: %s",
r->filename);
     }
     else {
         procnew = apr_pcalloc(p, sizeof(*procnew));
+  /*ap_proc_create() is invoked inside ap_os_create_privileged_process*/
         rc = ap_os_create_privileged_process(r, procnew, command, argv,
env,
                                              procattr, p);

BUG3:
Description: This bug was found by analyzing bug 39722 (
https://issues.apache.org/bugzilla/show_bug.cgi?id=39722); this bug added a
check for the return of ap_server_root_relative; we found a bug where an
additional check for the return value is missing.

We have observed that almost in all the places where ap_server_root_relative
is invoked, its return value is checked.  In the bug we discovered, however,
the return value is not checked.  I don’t know whether it is because that
ap_scoreboard_fname is assigned a constant value DEFAULT_SCOREBOARD.  But I
think it is safe to add a check to the return value anyway…

**********************************original
bug-fix*************************************
--- core.c revision 589176
+++ core.c revision 589177
@@ -1164,6 +1164,9 @@

     /* Make it absolute, relative to ServerRoot */
     arg = ap_server_root_relative(cmd->pool, arg);
+    if (arg == NULL) {
+        return "DocumentRoot must be a directory";
+    }

     /* TODO: ap_configtestonly && ap_docrootcheck && */
     if (apr_filepath_merge((char**)&conf->ap_document_root, NULL, arg,


*****************************discovered potential bug and possible
fix*********************
--- server/scoreboard.c (revision 806655)
+++ server/scoreboard.c (working copy)
@@ -221,7 +221,9 @@
             /* Make sure it's an absolute pathname */
             ap_scoreboard_fname = DEFAULT_SCOREBOARD;
             fname = ap_server_root_relative(pconf, ap_scoreboard_fname);
-
+   if(!fname){
+    //Add error handling messsage and return
+   }
             return create_namebased_scoreboard(global_pool, fname);
         }
     }

BUG4:
Description: This bug was found by analyzing bug 39518 (
https://issues.apache.org/bugzilla/show_bug.cgi?id=39518); this bug change
some "apr_palloc / memcpy" construction into a single apr_pmemdup.

It is actually not a bug-fix, but the change of programming style.  However,
I think it’s still worth mentioning some of the code segments we discovered
which need to be refactored the same way as the bug-fix.

**********************************original
bug-fix*************************************
--- mod_include.c (revision 557836)
+++ mod_include.c (revision 557837)
@@ -3225,9 +3225,8 @@

             /* check if we mismatched earlier and have to release some
chars */
             if (release && (ctx->flags & SSI_FLAG_PRINTING)) {
-                char *to_release = apr_palloc(ctx->pool, release);
+                char *to_release = apr_pmemdup(ctx->pool,
intern->start_seq, release);

-                memcpy(to_release, intern->start_seq, release);
                 newb = apr_bucket_pool_create(to_release, release,
ctx->pool,
                                               f->c->bucket_alloc);
                 APR_BRIGADE_INSERT_TAIL(pass_bb, newb);
*****************************discovered potential bug and possible
fix*********************
--- server/util_filter.c (revision 806655)
+++ server/util_filter.c (working copy)
@@ -74,9 +74,8 @@
     if (parent->nchildren == parent->size) {
         filter_trie_child_ptr *new;
         parent->size *= 2;
-        new = (filter_trie_child_ptr *)apr_palloc(p, parent->size *
-
sizeof(filter_trie_child_ptr));
-        memcpy(new, parent->children, parent->nchildren *
+
+  new = (filter_trie_child_ptr *)apr_pmemdup(p, parent->children,
parent->nchildren *
                sizeof(filter_trie_child_ptr));
         parent->children = new;
     }
--- modules/http/mod_mime.c (revision 806655)
+++ modules/http/mod_mime.c (working copy)
@@ -182,10 +182,10 @@
                                               APR_HASH_KEY_STRING);
         if (exinfo && *(const char**)((char *)exinfo + suffix[i].offset)) {
             extension_info *copyinfo = exinfo;
-            exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
+           exinfo = (extension_info*)apr_pmemdup(p,copyinfo,
sizeof(*exinfo));
             apr_hash_set(mappings, suffix[i].name,
                          APR_HASH_KEY_STRING, exinfo);
-            memcpy(exinfo, copyinfo, sizeof(*exinfo));
+
             *(const char**)((char *)exinfo + suffix[i].offset) = NULL;
         }
     }




-- 
BOYA SUN
Computer Science Division
Electrical Engineering & Computer Science Department
513 Olin Building
Case Western Reserve University
10900 Euclid Avenue
Cleveland, OH 44106
boya.sun@case.edu

Mime
View raw message