[PATCH] Serf crash on malformed status line

<pierre.viret_at_postfinance.ch> writes:

>> I noticed that your proxy is changing the status line by dropping the
>> reason-phrase:
>>
>> HTTP/1.1 207 Multi-Status
>>
>> to
>>
>> HTTP/1.1 207

> Good news! I could create a new implementation of the JDK HttpServer
> which fixes the 207 Status line problem, and I can't reproduce the
> segmentation fault anymore when the HttpProxy uses this fixed version.

Looking at serf's response_buckets.c it doesn't handle a malformed
status line like

    HTTP/1.1 207

That passes the apr_date_checkmask check:

    /* ctx->linebuf.line should be of form: HTTP/1.1 200 OK */
    res = apr_date_checkmask(ctx->linebuf.line, "HTTP/#.# ###*");
    if (!res) {
        /* Not an HTTP response? Well, at least we won't understand it. */
        return SERF_ERROR_BAD_HTTP_RESPONSE;
    }

    ctx->sl.version = SERF_HTTP_VERSION(ctx->linebuf.line[5] - '0',
                                        ctx->linebuf.line[7] - '0');
    ctx->sl.code = apr_strtoi64(ctx->linebuf.line + 8, &reason, 10);

    /* Skip leading spaces for the reason string. */
    if (apr_isspace(*reason)) {
        reason++;
    }

    /* Copy the reason value out of the line buffer. */
    ctx->sl.reason = serf_bstrmemdup(allocator, reason,
                                     ctx->linebuf.used
                                     - (reason - ctx->linebuf.line));

After the apr_strtoi64 reason will point just beyond ctx->linebuf.used.
That's still within the buffer but *reason is arbitrary. If it happens
to be whitespace then reason will be incremented and the value passed to
serf_bstrmemdup will be negative. That converts to a huge unsigned
value which causes a SEGV.

Two things need to change:

  - the mask needs to require at least one space after the status code

  - ctx->linebuf.used needs to be at least 13 before calling
    apr_date_checkmask.

Index: buckets/response_buckets.c
===================================================================
--- buckets/response_buckets.c (revision 2445)
+++ buckets/response_buckets.c (working copy)
@@ -129,7 +129,10 @@ static apr_status_t parse_status_line(response_con
     char *reason; /* ### stupid APR interface makes this non-const */
 
     /* ctx->linebuf.line should be of form: HTTP/1.1 200 OK */
- res = apr_date_checkmask(ctx->linebuf.line, "HTTP/#.# ###*");
+ if (ctx->linebuf.used < 13) {
+ return SERF_ERROR_BAD_HTTP_RESPONSE;
+ }
+ res = apr_date_checkmask(ctx->linebuf.line, "HTTP/#.# ### *");
     if (!res) {
         /* Not an HTTP response? Well, at least we won't understand it. */
         return SERF_ERROR_BAD_HTTP_RESPONSE;

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*