Re: [PATCH] Serf crash on malformed status line

Philip Martin <philip.martin_at_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*