[PATCH] Serf crash on malformed status line

[Philip Martin <ph...@wandisco.com>]

<pi...@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*

Re: [PATCH] Serf crash on malformed status line

[Philip Martin <ph...@wandisco.com>]

Philip Martin <ph...@wandisco.com> writes:

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

That's not enough, we also need to stop the apr_isspace loop from
running beyond the used bytes:

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;
@@ -140,7 +143,8 @@ static apr_status_t parse_status_line(response_con
     ctx->sl.code = apr_strtoi64(ctx->linebuf.line + 8, &reason, 10);
 
     /* Skip leading spaces for the reason string. */
-    if (apr_isspace(*reason)) {
+    if (apr_isspace(*reason)
+        && reason < ctx->linebuf.line + ctx->linebuf.used) {
         reason++;
     }
 

With that loop fix we could drop the requirement for a space after the
status code and change the length check to 12.  It depends whether we
want the minimum status line to be

   "HTTP/1.1 207 "

or

   "HTTP/1.1 207"

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