From d678ea3cd2b29022457ba6664ab71688c010c17f Mon Sep 17 00:00:00 2001 From: Andy Green Date: Sat, 12 Jan 2013 23:09:36 +0800 Subject: [PATCH] renable deflate frame buffer expansion fixing DoS This reverts the removal of the deflate_frame code that was crashing after porting David Galeano's code: he pointed out there's a typo in the merged version causing the crash which is fixed here. However the fixed code has a problem, there's no limit (other than int size) to the amount of memory it will try to malloc, which can allow a DoS of the server by the client sending malicious compression states that inflate to a large amount. I have added checking for OOM already that will avert the segfault that would otherwise follow but the server will be unusuable if malicious connections were made repeatedly each forcing it to allocate large buffers and cause small allocations on other connections to fail. The patch changes the code to use realloc(), and introduces a configurable limit on the amount of memory one connection may need for zlib before the server hangs up the connection. It defaults to 64KBytes but can be set from ./configure as described now in the README. Signed-off-by: Andy Green --- README-test-server | 15 ++++++ lib/extension-deflate-frame.c | 116 +++++++++++++++++++++++++++--------------- 2 files changed, 91 insertions(+), 40 deletions(-) diff --git a/README-test-server b/README-test-server index a2ce210..f580672 100644 --- a/README-test-server +++ b/README-test-server @@ -70,6 +70,21 @@ There are a couple of other possible configure options one is used. +Externally configurable important constants +------------------------------------------- + +You can control these from configure by just setting them as commandline +args throgh CFLAGS, eg + +./configure CFLAGS="-DLWS_MAX_ZLIB_CONN_BUFFER=8192" + +They all have defaults so you only need to take care about them if you want +to tune them to the amount of memory available. + + - LWS_MAX_ZLIB_CONN_BUFFER maximum size a compression buffer is allowed to +grow to before closing the connection. Default is 64KBytes. + + Testing server with a browser ----------------------------- diff --git a/lib/extension-deflate-frame.c b/lib/extension-deflate-frame.c index c8ba423..a96d996 100644 --- a/lib/extension-deflate-frame.c +++ b/lib/extension-deflate-frame.c @@ -9,6 +9,9 @@ #define MIN_SIZE_TO_DEFLATE 4 +#ifndef LWS_MAX_ZLIB_CONN_BUFFER +#define LWS_MAX_ZLIB_CONN_BUFFER (64 * 1024) +#endif int lws_extension_callback_deflate_frame( struct libwebsocket_context *context, @@ -22,6 +25,7 @@ int lws_extension_callback_deflate_frame( struct lws_tokens *eff_buf = (struct lws_tokens *)in; size_t current_payload, remaining_payload, total_payload; int n; + size_t len_so_far; switch (reason) { @@ -149,23 +153,54 @@ bail: conn->zs_in.avail_in = total_payload + 4; - conn->zs_in.next_out = conn->buf_in + LWS_SEND_BUFFER_PRE_PADDING; - conn->zs_in.avail_out = conn->buf_in_length; - - n = inflate(&conn->zs_in, Z_SYNC_FLUSH); - switch (n) { - case Z_NEED_DICT: - case Z_STREAM_ERROR: - case Z_DATA_ERROR: - case Z_MEM_ERROR: - /* - * screwed.. close the connection... we will get a - * destroy callback to take care of closing nicely - */ - lwsl_ext("zlib error inflate %d: %s", - n, conn->zs_in.msg); - return -1; - } + conn->zs_in.next_out = conn->buf_in + LWS_SEND_BUFFER_PRE_PADDING; + conn->zs_in.avail_out = conn->buf_in_length; + + while (1) { + n = inflate(&conn->zs_in, Z_SYNC_FLUSH); + switch (n) { + case Z_NEED_DICT: + case Z_STREAM_ERROR: + case Z_DATA_ERROR: + case Z_MEM_ERROR: + /* + * screwed.. close the connection... we will get a + * destroy callback to take care of closing nicely + */ + fprintf(stderr, "zlib error inflate %d: %s", + n, conn->zs_in.msg); + return -1; + } + + if (conn->zs_in.avail_out) + break; + + len_so_far = conn->zs_in.next_out - + (conn->buf_in + LWS_SEND_BUFFER_PRE_PADDING); + + conn->buf_in_length *= 2; + if (conn->buf_in_length > LWS_MAX_ZLIB_CONN_BUFFER) { + lwsl_ext("zlib in buffer tried to exceed " + "max allowed of %u\n", + LWS_MAX_ZLIB_CONN_BUFFER); + return -1; + } + conn->buf_in = (unsigned char *)realloc(conn->buf_in, + LWS_SEND_BUFFER_PRE_PADDING + + conn->buf_in_length + + LWS_SEND_BUFFER_POST_PADDING); + if (!conn->buf_in) { + lwsl_err("Out of memory\n"); + return -1; + } + lwsl_debug( + "deflate-frame ext RX did realloc to %ld\n", + conn->buf_in_length); + conn->zs_in.next_out = conn->buf_in + + LWS_SEND_BUFFER_PRE_PADDING + len_so_far; + conn->zs_in.avail_out = + conn->buf_in_length - len_so_far; + } /* rewrite the buffer pointers and length */ eff_buf->token = (char *)(conn->buf_in + LWS_SEND_BUFFER_PRE_PADDING); @@ -201,35 +236,36 @@ bail: return -1; } - /* - * AG: uncertain about this log buffer expansion approach... - * same approach in Rx led to memory runaway OOM - */ - if (!conn->zs_out.avail_out) { - size_t len_so_far = (conn->zs_out.next_out - + if (conn->zs_out.avail_out) + break; + + len_so_far = (conn->zs_out.next_out - (conn->buf_out + LWS_SEND_BUFFER_PRE_PADDING)); - unsigned char *new_buf; - conn->buf_out_length *= 2; - new_buf = (unsigned char *) - malloc(LWS_SEND_BUFFER_PRE_PADDING + + conn->buf_out_length *= 2; + if (conn->buf_out_length > LWS_MAX_ZLIB_CONN_BUFFER) { + lwsl_ext("zlib out buffer tried to exceed " + "max allowed of %u\n", + LWS_MAX_ZLIB_CONN_BUFFER); + return -1; + } + conn->buf_out = (unsigned char *)realloc( + conn->buf_out, + LWS_SEND_BUFFER_PRE_PADDING + conn->buf_out_length + LWS_SEND_BUFFER_POST_PADDING); - if (!new_buf) { - lwsl_err("Out of memory\n"); - return -1; - } - memcpy(new_buf + LWS_SEND_BUFFER_PRE_PADDING, - conn->buf_out + LWS_SEND_BUFFER_PRE_PADDING, - len_so_far); - free(conn->buf_out); - conn->buf_out = new_buf; - conn->zs_out.next_out = (new_buf + + if (!conn->buf_out) { + lwsl_err("Out of memory\n"); + return -1; + } + lwsl_debug( + "deflate-frame ext TX did realloc to %ld\n", + conn->buf_in_length); + + conn->zs_out.next_out = (conn->buf_out + LWS_SEND_BUFFER_PRE_PADDING + len_so_far); - conn->zs_out.avail_out = + conn->zs_out.avail_out = (conn->buf_out_length - len_so_far); - } else - break; } conn->compressed_out = 1; -- 2.7.4