httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: svn commit: r1772678 [2/2] - in /httpd/httpd/branches/2.4.x: ./ docs/manual/mod/ include/ modules/http/ server/
Date Wed, 07 Dec 2016 22:01:57 GMT
On Wed, Dec 7, 2016 at 2:11 PM, Ruediger Pluem <rpluem@apache.org> wrote:

> Sorry for chiming in so late :-(.


Again, no worries...

On 12/05/2016 03:34 PM, jim@apache.org wrote:
> > Modified: httpd/httpd/branches/2.4.x/server/protocol.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/
> server/protocol.c?rev=1772678&r1=1772677&r2=1772678&view=diff
> > ============================================================
> ==================
> > --- httpd/httpd/branches/2.4.x/server/protocol.c (original)
> > +++ httpd/httpd/branches/2.4.x/server/protocol.c Mon Dec  5 14:34:29
> 2016
>
> > @@ -617,58 +646,269 @@ static int read_request_line(request_rec
> > [...]
> > +
> > +    /* Advance protocol pointer over leading whitespace, NUL terminate
> the uri
> > +     * If non-SP whitespace is encountered, mark as specific error
> > +     */
> > +    for (r->protocol = ll; apr_isspace(*r->protocol); ++r->protocol)
> > +        if (*r->protocol != ' ' && deferred_error == rrl_none)
> > +            deferred_error = rrl_badwhitespace;
> > +    *ll = '\0';
> > +
> > +    /* Scan the protocol up to the next whitespace, validation comes
> later */
> > +    if (!(ll = (char*) ap_scan_vchar_obstext(r->protocol))) {
> > +        len = strlen(r->protocol);
> > +        goto rrl_done;
> > +    }
> > +    len = ll - r->protocol;
> > +
> > +    /* Advance over trailing whitespace, if found mark in error,
> > +     * determine if trailing text is found, unconditionally mark in
> error,
> > +     * finally NUL terminate the protocol string
> > +     */
> > +    if (*ll && !apr_isspace(*ll)) {
> > +        deferred_error = rrl_badprotocol;
> > +    }
> > +    else if (strict && *ll) {
> > +        deferred_error = rrl_excesswhitespace;
> >      }
> > +    else {
> > +        for ( ; apr_isspace(*ll); ++ll)
> > +            if (*ll != ' ' && deferred_error == rrl_none)
> > +                deferred_error = rrl_badwhitespace;
> > +        if (*ll && deferred_error == rrl_none)
> > +            deferred_error = rrl_trailingtext;
> > +    }
> > +    *((char *)r->protocol + len) = '\0';
> > +
> > +rrl_done:
> > +    /* For internal integrety and palloc efficiency, reconstruct
> the_request
> > +     * in one palloc, using only single SP characters, per spec.
> > +     */
> > +    r->the_request = apr_pstrcat(r->pool, r->method, *uri ? " " : NULL,
> uri,
> > +                                 *r->protocol ? " " : NULL,
> r->protocol, NULL);
> >
> > -    /* Provide quick information about the request method as soon as
> known */
> > +    if (len == 8
> > +            && r->protocol[0] == 'H' && r->protocol[1] ==
'T'
> > +            && r->protocol[2] == 'T' && r->protocol[3] ==
'P'
> > +            && r->protocol[4] == '/' && apr_isdigit(r->protocol[5])
> > +            && r->protocol[6] == '.' && apr_isdigit(r->protocol[7])
> > +            && r->protocol[5] != '0') {
> > +        r->assbackwards = 0;
> > +        r->proto_num = HTTP_VERSION(r->protocol[5] - '0',
> r->protocol[7] - '0');
> > +    }
> > +    else if (len == 8
> > +                 && (r->protocol[0] == 'H' || r->protocol[0] == 'h')
> > +                 && (r->protocol[1] == 'T' || r->protocol[1] == 't')
> > +                 && (r->protocol[2] == 'T' || r->protocol[2] == 't')
> > +                 && (r->protocol[3] == 'P' || r->protocol[3] == 'p')
> > +                 && r->protocol[4] == '/' && apr_isdigit(r->protocol[5])
> > +                 && r->protocol[6] == '.' && apr_isdigit(r->protocol[7])
> > +                 && r->protocol[5] != '0') {
> > +        r->assbackwards = 0;
> > +        r->proto_num = HTTP_VERSION(r->protocol[5] - '0',
> r->protocol[7] - '0');
> > +        if (strict && deferred_error == rrl_none)
> > +            deferred_error = rrl_badprotocol;
> > +        else
> > +            memcpy((char*)r->protocol, "HTTP", 4);
> > +    }
> > +    else if (r->protocol[0]) {
> > +        r->assbackwards = 0;
> > +        r->proto_num = HTTP_VERSION(1,0);
> > +        /* Defer setting the r->protocol string till error msg is
> composed */
> > +        if (strict && deferred_error == rrl_none)
> > +            deferred_error = rrl_badprotocol;
> > +        else
> > +            r->protocol  = "HTTP/1.0";
>
> Hm. r->protocol is not const char*. Shouldn't we use
>
> apr_pstrdup(r->pool, "HTTP/1.0");
>
> > +    }
>
> +    else {
> > +        r->assbackwards = 1;
> > +        r->protocol = "HTTP/0.9";
> > +        r->proto_num = HTTP_VERSION(0, 9);
> > +    }
>

Does it make more sense to to apply the apr_pstrdup() against
r->protocol just here, after all but one assignment is made?

The same question occurs here with 0.9, and down at rrl_failed:
below. We can apr_pstrdup each of these three, letting the caller
manipulate the original we scanned in the successful case, or
make a copy in all cases right here, once again below for 1.0.

I had expected to see some caution and mentioned it in the
initial commit log, but my maintainer-mode build did not trip on this.


> > +        else if (deferred_error == rrl_badprotocol)
> > +            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
> > +                          "HTTP Request Line; Unrecognized protocol
> '%.*s' "
> > +                          "(perhaps whitespace was injected?)",
> > +                          field_name_len(r->protocol), r->protocol);
> Don't we miss a
>
> r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
>
> in this case?


No, we do not, it was assigned to HTTP/0.9 above in the case of no
protocol, and rrl_failed will whack it later into HTTP/1.0 to ensure we
see an error (we wouldn't see a response line in HTTP/0.9 mode).


> > +        r->status = HTTP_BAD_REQUEST;
> > +        goto rrl_failed;
> > +    }
>

Mime
View raw message