httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From d...@ast.cam.ac.uk (David Robinson)
Subject Re: REPOST: URL decoding bugs in apache 0.8.3
Date Thu, 27 Jul 1995 15:53:00 GMT
I think that apache showing problems for URL paths containing (decoded) '%'
characters should be considered signficicant.

>   1. PATH_INFO gets URL decoded _again_ before PATH_TRANSLATED is found;
>      e.g.
>      http://server.com/cgi-bin/test-cgi/hello%2524
>
>      produces
>      PATH_INFO = /hello%24
>      PATH_TRANSLATED = /opt/httpd/htdocs/hello$
>
>      fix: new routine sub_req_lookup_path() for this case.
>
>From a maintenance point of view, it would be much better to just call
>escape_uri on the argument to sub_req_lookup_uri() --- this is *very*
>slightly more expensive, but the extra cost is trivial in the context
>of a CGI fork/exec.

Just as good. (Of course, the CGI spec should never have defined PATH_INFO
to URL decoded.)

>(As a matter of style, there's way too much duplication of code in the
>sub_req stuff, and it has already been a maintenance headache.
>Creating another complete duplicate is, in my view, something to be
>avoided if there is any plausible alternative --- and in this case,
>there is).

>Oh yes. I was simply being consistend with the existing code.
>
>   3. for virtual path redirects, the URL might not be decoded, or parts of
>      it might be decoded twice.
>
>      The call to unescape_url() in sub_req_lookup_uri() is in the wrong place;
>      the new path should be decoded straight away.
>
>Disagree -- the current code there parallels the treatment of URIs as
>they come in from the client; in particular, parse_uri must be called
>before unescape_uri.  Your "fix" does it the other way, which would
>introduce at least two clear bugs --- a sub_req_uri lookup for
>/foo/bar%3fmoo would be treated the same as /foo/bar?moo, (it would
>*be* the same when parse_uri was called), which is clearly incorrect,
>and any args (anything following a real '?') in the URI would be
>decoded, which is very wrong (scripts need to be able to distinguish
>'%2b' from '+').

Well spotted.
But the current code has similar bugs;
a lookup of bar%41?moo would be inherit the args of the original request,
instead of setting it to 'moo', and the path would not be URL decoded
to barA. And it would fail if the original path contained a (decoded)
'%' character.

Revised patch for this bug below.

>   4. (May not be a bug) sub_req_lookup_file wasn't setting the args of the
>       new request.
>
>Not a bug, in my view --- URIs may have arguments, by the '...?foo'
>convention, but filenames do not.

Only a suggestion; but it seemed easier to set args rather than try and
guarantee that no existing or future code would ever try and access the args
for a request returned by sub_req_look_file.


 David.

------------------ begin file decode.patch -------------------------
*** http_request.c.orig	Tue Jul 25 02:12:45 1995
--- http_request.c	Thu Jul 27 15:42:29 1995
***************
*** 326,341 ****
  request_rec *sub_req_lookup_uri (char *new_file, request_rec *r)
  {
      request_rec *rnew;
!     int res;
      char *udir;
      
!     /* Check for a special case... if there are no '/' characters in new_file
!      * at all, then we are looking at a relative lookup in the same directory.
       * That means we don't have to redo any access checks.
       */
  
!     if (strchr (new_file, '/') == NULL) 
          return sub_req_lookup_simple (new_file, r);
      
      /* The nasty general case.  Need to start from nearly scratch. */
  
--- 326,346 ----
  request_rec *sub_req_lookup_uri (char *new_file, request_rec *r)
  {
      request_rec *rnew;
!     int res, relpath;
      char *udir;
      
!     /* Check for a special case... if there are no '/' or '?' characters in
!      * new_file at all, then we are looking at a relative lookup in the same
!      * directory without any query_args.
       * That means we don't have to redo any access checks.
       */
  
!     if (strchr (new_file, '/') == NULL && strchr(new_file, '?') == NULL) 
!     {
! 	new_file = pstrdup(r->pool, new_file);
! 	unescape_url(new_file);
          return sub_req_lookup_simple (new_file, r);
+     }
      
      /* The nasty general case.  Need to start from nearly scratch. */
  
***************
*** 346,357 ****
      rnew->server = r->server;
      rnew->request_config = create_request_config (rnew->pool);
      set_sub_req_protocol (rnew, r);
! 	
!     parse_uri (rnew, ((new_file[0] == '/') ?
! 		      new_file :
! 		      make_full_path (rnew->pool, udir, new_file)));
! 	
      unescape_url (rnew->uri);
      getparents (rnew->uri);
  	
      if ((res = translate_name (rnew))
--- 351,363 ----
      rnew->server = r->server;
      rnew->request_config = create_request_config (rnew->pool);
      set_sub_req_protocol (rnew, r);
! 
!     relpath = new_file[0] != '/';
!     parse_uri (rnew, new_file);
      unescape_url (rnew->uri);
+ 
+     if (relpath) rnew->uri = make_full_path(rnew->pool, udir, rnew->uri);
+ 	
      getparents (rnew->uri);
  	
      if ((res = translate_name (rnew))
------------------ end file decode.patch -------------------------------


Mime
View raw message