Hi Jesse, On Tue, Feb 07, 2017 at 06:37:09PM +0000, Jesse Schulman wrote: > Thank you for the update, we are running the patch Thierry provided with > success, but we only do a lua call within the %[] almost identically to the > simple reproducer I provided. I *think* we are safe considering we don't > do any redirect in the way that your (Willy's) reproducer is doing it. > > We will definitely look to upgrade to the next available stable version > that includes the proper fix.
Here come the two patches I intend to merge. The first one implements a simple dedicated trash allocator and the second one is Thierry's patch rebased to use it. It fixes the issue for me in both cases (your Lua test and my redirect involving the string conversion). Please test and let me know. I'm confident but I prefer to get your confirmation before merging them. Thanks! Willy
>From b686afd56817316a42529330a6b59c07708c2a37 Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Wed, 8 Feb 2017 11:06:11 +0100 Subject: MINOR: chunks: implement a simple dynamic allocator for trash buffers X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 The trash buffers are becoming increasingly complex to deal with due to the code's modularity allowing some functions to be chained and causing the same chunk buffers to be used multiple times along the chain, possibly corrupting each other. In fact the trash were designed from scratch for explicitly not surviving a function call but string manipulation makes this impossible most of the time while not fullfilling the need for reliable temporary chunks. Here we introduce the ability to allocate a temporary trash chunk which is reserved, so that it will not conflict with the trash chunks other functions use, and will even support reentrant calls (eg: build_logline). For this, we create a new pool which is exactly the size of a usual chunk buffer plus the size of the chunk struct so that these chunks when allocated are exactly the same size as the ones returned by get_trash_buffer(). These chunks may fail so the caller must check them, and the caller is also responsible for freeing them. The code focuses on minimal changes and ease of reliable backporting because it will be needed in stable versions in order to support next patch. --- include/common/chunk.h | 13 +++++++++++++ src/chunk.c | 25 ++++++++++++++++++++++++- src/haproxy.c | 1 + 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/include/common/chunk.h b/include/common/chunk.h index 205523c..8c067f7 100644 --- a/include/common/chunk.h +++ b/include/common/chunk.h @@ -26,6 +26,7 @@ #include <string.h> #include <common/config.h> +#include <common/memory.h> /* describes a chunk of string */ @@ -35,6 +36,8 @@ struct chunk { int len; /* current size of the string from first to last char. <0 = uninit. */ }; +struct pool_head *pool2_trash; + /* function prototypes */ int chunk_printf(struct chunk *chk, const char *fmt, ...) @@ -50,6 +53,16 @@ int chunk_strcasecmp(const struct chunk *chk, const char *str); int alloc_trash_buffers(int bufsize); void free_trash_buffers(void); struct chunk *get_trash_chunk(void); +struct chunk *alloc_trash_chunk(void); + +/* + * free a trash chunk allocated by alloc_trash_chunk(). NOP on NULL. + */ +static inline void free_trash_chunk(struct chunk *chunk) +{ + pool_free2(pool2_trash, chunk); +} + static inline void chunk_reset(struct chunk *chk) { diff --git a/src/chunk.c b/src/chunk.c index 7134fea..6090212 100644 --- a/src/chunk.c +++ b/src/chunk.c @@ -29,6 +29,9 @@ static int trash_size; static char *trash_buf1; static char *trash_buf2; +/* the trash pool for reentrant allocations */ +struct pool_head *pool2_trash = NULL; + /* * Returns a pre-allocated and initialized trash chunk that can be used for any * type of conversion. Two chunks and their respective buffers are alternatively @@ -63,7 +66,8 @@ int alloc_trash_buffers(int bufsize) trash_size = bufsize; trash_buf1 = (char *)my_realloc2(trash_buf1, bufsize); trash_buf2 = (char *)my_realloc2(trash_buf2, bufsize); - return trash_buf1 && trash_buf2; + pool2_trash = create_pool("trash", sizeof(struct chunk) + bufsize, MEM_F_EXACT); + return trash_buf1 && trash_buf2 && pool2_trash; } /* @@ -78,6 +82,25 @@ void free_trash_buffers(void) } /* + * Allocate a trash chunk from the reentrant pool. The buffer starts at the + * end of the chunk. This chunk must be freed using free_trash_chunk(). This + * call may fail and the caller is responsible for checking that the returned + * pointer is not NULL. + */ +struct chunk *alloc_trash_chunk(void) +{ + struct chunk *chunk; + + chunk = pool_alloc2(pool2_trash); + if (chunk) { + char *buf = (char *)chunk + sizeof(struct chunk); + *buf = 0; + chunk_init(chunk, buf, pool2_trash->size - sizeof(struct chunk)); + } + return chunk; +} + +/* * Does an snprintf() at the beginning of chunk <chk>, respecting the limit of * at most chk->size chars. If the chk->len is over, nothing is added. Returns * the new chunk size, or < 0 in case of failure. diff --git a/src/haproxy.c b/src/haproxy.c index 41be9cd..559b481 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -1545,6 +1545,7 @@ static void deinit(void) pool_destroy2(pool2_stream); pool_destroy2(pool2_session); pool_destroy2(pool2_connection); + pool_destroy2(pool2_trash); pool_destroy2(pool2_buffer); pool_destroy2(pool2_requri); pool_destroy2(pool2_task); -- 1.7.12.1
>From 0d94576c74a4e6cef050b6cddb513fd9a363cf6c Mon Sep 17 00:00:00 2001 From: Thierry FOURNIER <[email protected]> Date: Sat, 28 Jan 2017 07:39:53 +0100 Subject: BUG/MEDIUM: http: prevent redirect from overwriting a buffer X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 See 4b788f7d349ddde3f70f063b7394529eac6ab678 If we use the action "http-request redirect" with a Lua sample-fetch or converter, and the Lua function calls one of the Lua log function, the header name is corrupted, it contains an extract of the last loggued data. This is due to an overwrite of the trash buffer, because his scope is not respected in the "add-header" function. The scope of the trash buffer must be limited to the function using it. The build_logline() function can execute a lot of other function which can use the trash buffer. This patch fix the usage of the trash buffer. It limits the scope of this global buffer to the local function, we build first the header value using build_logline, and after we store the header name. Thanks Jesse Schulman for the bug repport. This patch must be backported in 1.7, 1.6 and 1.5 version, and it relies on commit b686afd ("MINOR: chunks: implement a simple dynamic allocator for trash buffers") for the trash allocator, which has to be backported as well. --- src/proto_http.c | 129 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 69 insertions(+), 60 deletions(-) diff --git a/src/proto_http.c b/src/proto_http.c index 3490aa7..23a7dc4 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -4024,6 +4024,12 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s struct http_msg *res = &txn->rsp; const char *msg_fmt; const char *location; + struct chunk *chunk; + int ret = 0; + + chunk = alloc_trash_chunk(); + if (!chunk) + goto leave; /* build redirect message */ switch(rule->code) { @@ -4045,10 +4051,10 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s break; } - if (unlikely(!chunk_strcpy(&trash, msg_fmt))) - return 0; + if (unlikely(!chunk_strcpy(chunk, msg_fmt))) + goto leave; - location = trash.str + trash.len; + location = chunk->str + chunk->len; switch(rule->type) { case REDIRECT_TYPE_SCHEME: { @@ -4087,40 +4093,40 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s if (rule->rdr_str) { /* this is an old "redirect" rule */ /* check if we can add scheme + "://" + host + path */ - if (trash.len + rule->rdr_len + 3 + hostlen + pathlen > trash.size - 4) - return 0; + if (chunk->len + rule->rdr_len + 3 + hostlen + pathlen > chunk->size - 4) + goto leave; /* add scheme */ - memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len); - trash.len += rule->rdr_len; + memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len); + chunk->len += rule->rdr_len; } else { /* add scheme with executing log format */ - trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt); + chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt); /* check if we can add scheme + "://" + host + path */ - if (trash.len + 3 + hostlen + pathlen > trash.size - 4) - return 0; + if (chunk->len + 3 + hostlen + pathlen > chunk->size - 4) + goto leave; } /* add "://" */ - memcpy(trash.str + trash.len, "://", 3); - trash.len += 3; + memcpy(chunk->str + chunk->len, "://", 3); + chunk->len += 3; /* add host */ - memcpy(trash.str + trash.len, host, hostlen); - trash.len += hostlen; + memcpy(chunk->str + chunk->len, host, hostlen); + chunk->len += hostlen; /* add path */ - memcpy(trash.str + trash.len, path, pathlen); - trash.len += pathlen; + memcpy(chunk->str + chunk->len, path, pathlen); + chunk->len += pathlen; /* append a slash at the end of the location if needed and missing */ - if (trash.len && trash.str[trash.len - 1] != '/' && + if (chunk->len && chunk->str[chunk->len - 1] != '/' && (rule->flags & REDIRECT_FLAG_APPEND_SLASH)) { - if (trash.len > trash.size - 5) - return 0; - trash.str[trash.len] = '/'; - trash.len++; + if (chunk->len > chunk->size - 5) + goto leave; + chunk->str[chunk->len] = '/'; + chunk->len++; } break; @@ -4149,38 +4155,38 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s } if (rule->rdr_str) { /* this is an old "redirect" rule */ - if (trash.len + rule->rdr_len + pathlen > trash.size - 4) - return 0; + if (chunk->len + rule->rdr_len + pathlen > chunk->size - 4) + goto leave; /* add prefix. Note that if prefix == "/", we don't want to * add anything, otherwise it makes it hard for the user to * configure a self-redirection. */ if (rule->rdr_len != 1 || *rule->rdr_str != '/') { - memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len); - trash.len += rule->rdr_len; + memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len); + chunk->len += rule->rdr_len; } } else { /* add prefix with executing log format */ - trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt); + chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt); /* Check length */ - if (trash.len + pathlen > trash.size - 4) - return 0; + if (chunk->len + pathlen > chunk->size - 4) + goto leave; } /* add path */ - memcpy(trash.str + trash.len, path, pathlen); - trash.len += pathlen; + memcpy(chunk->str + chunk->len, path, pathlen); + chunk->len += pathlen; /* append a slash at the end of the location if needed and missing */ - if (trash.len && trash.str[trash.len - 1] != '/' && + if (chunk->len && chunk->str[chunk->len - 1] != '/' && (rule->flags & REDIRECT_FLAG_APPEND_SLASH)) { - if (trash.len > trash.size - 5) - return 0; - trash.str[trash.len] = '/'; - trash.len++; + if (chunk->len > chunk->size - 5) + goto leave; + chunk->str[chunk->len] = '/'; + chunk->len++; } break; @@ -4188,29 +4194,29 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s case REDIRECT_TYPE_LOCATION: default: if (rule->rdr_str) { /* this is an old "redirect" rule */ - if (trash.len + rule->rdr_len > trash.size - 4) - return 0; + if (chunk->len + rule->rdr_len > chunk->size - 4) + goto leave; /* add location */ - memcpy(trash.str + trash.len, rule->rdr_str, rule->rdr_len); - trash.len += rule->rdr_len; + memcpy(chunk->str + chunk->len, rule->rdr_str, rule->rdr_len); + chunk->len += rule->rdr_len; } else { /* add location with executing log format */ - trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->rdr_fmt); + chunk->len += build_logline(s, chunk->str + chunk->len, chunk->size - chunk->len, &rule->rdr_fmt); /* Check left length */ - if (trash.len > trash.size - 4) - return 0; + if (chunk->len > chunk->size - 4) + goto leave; } break; } if (rule->cookie_len) { - memcpy(trash.str + trash.len, "\r\nSet-Cookie: ", 14); - trash.len += 14; - memcpy(trash.str + trash.len, rule->cookie_str, rule->cookie_len); - trash.len += rule->cookie_len; + memcpy(chunk->str + chunk->len, "\r\nSet-Cookie: ", 14); + chunk->len += 14; + memcpy(chunk->str + chunk->len, rule->cookie_str, rule->cookie_len); + chunk->len += rule->cookie_len; } /* add end of headers and the keep-alive/close status. @@ -4230,17 +4236,17 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s /* keep-alive possible */ if (!(req->flags & HTTP_MSGF_VER_11)) { if (unlikely(txn->flags & TX_USE_PX_CONN)) { - memcpy(trash.str + trash.len, "\r\nProxy-Connection: keep-alive", 30); - trash.len += 30; + memcpy(chunk->str + chunk->len, "\r\nProxy-Connection: keep-alive", 30); + chunk->len += 30; } else { - memcpy(trash.str + trash.len, "\r\nConnection: keep-alive", 24); - trash.len += 24; + memcpy(chunk->str + chunk->len, "\r\nConnection: keep-alive", 24); + chunk->len += 24; } } - memcpy(trash.str + trash.len, "\r\n\r\n", 4); - trash.len += 4; - FLT_STRM_CB(s, flt_http_reply(s, txn->status, &trash)); - bo_inject(res->chn, trash.str, trash.len); + memcpy(chunk->str + chunk->len, "\r\n\r\n", 4); + chunk->len += 4; + FLT_STRM_CB(s, flt_http_reply(s, txn->status, chunk)); + bo_inject(res->chn, chunk->str, chunk->len); /* "eat" the request */ bi_fast_delete(req->chn->buf, req->sov); req->next -= req->sov; @@ -4255,13 +4261,13 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s } else { /* keep-alive not possible */ if (unlikely(txn->flags & TX_USE_PX_CONN)) { - memcpy(trash.str + trash.len, "\r\nProxy-Connection: close\r\n\r\n", 29); - trash.len += 29; + memcpy(chunk->str + chunk->len, "\r\nProxy-Connection: close\r\n\r\n", 29); + chunk->len += 29; } else { - memcpy(trash.str + trash.len, "\r\nConnection: close\r\n\r\n", 23); - trash.len += 23; + memcpy(chunk->str + chunk->len, "\r\nConnection: close\r\n\r\n", 23); + chunk->len += 23; } - http_reply_and_close(s, txn->status, &trash); + http_reply_and_close(s, txn->status, chunk); req->chn->analysers &= AN_REQ_FLT_END; } @@ -4270,7 +4276,10 @@ static int http_apply_redirect_rule(struct redirect_rule *rule, struct stream *s if (!(s->flags & SF_FINST_MASK)) s->flags |= SF_FINST_R; - return 1; + ret = 1; + leave: + free_trash_chunk(chunk); + return ret; } /* This stream analyser runs all HTTP request processing which is common to -- 1.7.12.1
