httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boya Sun <bxs...@case.edu>
Subject Re: four potential bugs discovered from CWRU research group
Date Wed, 26 Aug 2009 14:44:07 GMT
Has anyone looked at these bugs yet?  I would really appreciate it if
someone can comment on the bugs.

On Sat, Aug 22, 2009 at 12:04 PM, Boya Sun <bxs187@case.edu> wrote:

> 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
>



-- 
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