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....@case.edu
