Bug handling Upgrade: TLS/1.0 header in ssl_engine_io.c

[Joel J Smith <js...@novell.com>]

Hi httpd folks,
It seems that Joe Orton introduced a bug while updating ssl_engine_io.c
between version 109499 and version 111159.  The same bug was introduced
into NetWare's mod_nw_ssl.c version 111327. (Please forgive me if that's
not the correct way to reference svn version numbers... I'm new to svn.)
The code in question is part of the TLS Upgrade feature described in
RFC 2817 and was originally written by Ryan Bloom and committed by
Bill Rowe if I'm not mistaken.

Compare the new code:

    upgrade = apr_table_get(r->headers_in, "Upgrade");
    if (upgrade == NULL
        || strcmp(ap_getword(r->pool, &upgrade, ','), "TLS/1.0")) {
        /* "Upgrade: TLS/1.0, ..." header not found, don't do Upgrade */
        return ap_pass_brigade(f->next, bb);
    }

with the (bug-fixed, slightly cleaned-up) old code:

    if (upgrade == NULL) {
       /* "Upgrade: " header not found, don't do Upgrade */
        return ap_pass_brigade(f->next, bb);
    }
    token_string = apr_pstrdup(r->pool,upgrade);
    token = apr_strtok(token_string,", ",&token_state);
    while (token && strcasecmp(token,"TLS/1.0")) {
        token = apr_strtok(NULL,", ",&token_state);
    }
    if (token == NULL) {
       /* "TLS/1.0" token not in Upgrade header, * don't do Upgrade */
       return ap_pass_brigade(f->next, bb);
    }

While the new code is much shorter, it introduces a bug.  Consider
the header "Upgrade: SSL/3.0, TLS/1.0" or better yet, a derivation
of the example Upgrade header in the RFC 2616 snippet below with
TLS/1.0 somewhere besides the first spot.  The new code fails the
upgrade on any Upgrade header whose first token is not "TLS/1.0".
Also, I should mention that version 109499 had a bug where if
there was an Upgrade header that didn't have TLS/1.0 as the first
token, it got into an infinite loop.  That's why I said bug-fixed
above.

Maybe someone who is more familiar with the apr_ and ap_ APIs can
generate some working code that is shorter and cleaner than the
strtok-based code above.  Any correct implemenation I could think
of using ap_getword seemed like it would be messier than the
apr_strtok() version, especially since the tokens are in this case
"each separated by one or more commas (",") and OPTIONAL linear
white space (LWS)" (RFC 2616 2.1).

If there isn't an ap_ or apr_ function that seperates these tokens
more easily, I'm satisfied with the strtok based code above. Any
objections to me asking Brad Nicholes to commit it and revert that
part of 111159? (The other changes in that commit were very good)

Thanks,
Joel

----SNIP----

Here's the applicable verbage from RFC 2817 for your convenience:

3. Client Requested Upgrade to HTTP over TLS

   When the client sends an HTTP/1.1 request with an Upgrade header
   field containing the token "TLS/1.0", it is requesting the server to
   complete the current HTTP/1.1 request after switching to TLS/1.0.

and from RFC 2616:

14.42 Upgrade

   The Upgrade general-header allows the client to specify what
   additional communication protocols it supports and would like to use
   if the server finds it appropriate to switch protocols. The server
   MUST use the Upgrade header field within a 101 (Switching Protocols)
   response to indicate which protocol(s) are being switched.

       Upgrade        = "Upgrade" ":" 1#product

   For example,

       Upgrade: HTTP/2.0, SHTTP/1.3, IRC/6.9, RTA/x11

Re: Bug handling Upgrade: TLS/1.0 header in ssl_engine_io.c

["Joel J. Smith" <js...@novell.com>]

Joe Orton wrote:
> On Tue, Mar 08, 2005 at 08:54:52PM -0700, Joel J Smith wrote:
> 
>>Hi httpd folks,
>>It seems that Joe Orton introduced a bug while updating ssl_engine_io.c
>>between version 109499 and version 111159.  The same bug was introduced
>>into NetWare's mod_nw_ssl.c version 111327. (Please forgive me if that's
>>not the correct way to reference svn version numbers... I'm new to svn.)
>>The code in question is part of the TLS Upgrade feature described in
>>RFC 2817 and was originally written by Ryan Bloom and committed by
>>Bill Rowe if I'm not mistaken.
> 
> 
> Ah, right, thanks.  For some reason I thought the ordering in the header
> did matter but I can't see why now.  Can you send a patch to correct the
> issue?

I figured that was probably the case.  Patch attached.  Would it be 
helpful for me to create a bug report with the details from my first 
email that you can reference in the commit, or would that be more of a 
nuisance?  Looking back, I probably should have gone through bugzilla to 
begin with instead of bothering the dev list.
Thanks again,
Joel

Re: Bug handling Upgrade: TLS/1.0 header in ssl_engine_io.c

[Joe Orton <jo...@redhat.com>]

On Tue, Mar 08, 2005 at 08:54:52PM -0700, Joel J Smith wrote:
> Hi httpd folks,
> It seems that Joe Orton introduced a bug while updating ssl_engine_io.c
> between version 109499 and version 111159.  The same bug was introduced
> into NetWare's mod_nw_ssl.c version 111327. (Please forgive me if that's
> not the correct way to reference svn version numbers... I'm new to svn.)
> The code in question is part of the TLS Upgrade feature described in
> RFC 2817 and was originally written by Ryan Bloom and committed by
> Bill Rowe if I'm not mistaken.

Ah, right, thanks.  For some reason I thought the ordering in the header
did matter but I can't see why now.  Can you send a patch to correct the
issue?

joe