Hi all,
Here's a patch that fixes the problem with trash buffers being
overwritten during build_logline() execution.
Thanks.
Best regards,
Dragan Dosen
>From a5652cdbdbc71e4d303f28e6cacd7bdad263409f Mon Sep 17 00:00:00 2001
From: Dragan Dosen <ddo...@haproxy.com>
Date: Thu, 26 Oct 2017 11:25:10 +0200
Subject: [PATCH] BUG/MEDIUM: prevent buffers being overwritten during
build_logline() execution
Calls to build_logline() are audited in order to use dynamic trash buffers
allocated by alloc_trash_chunk() instead of global trash buffers.
This is similar to commits 07a0fec ("BUG/MEDIUM: http: Prevent
replace-header from overwriting a buffer") and 0d94576 ("BUG/MEDIUM: http:
prevent redirect from overwriting a buffer").
This patch should be backported in 1.7, 1.6 and 1.5. 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 | 218 ++++++++++++++++++++++++++++++++++---------------------
src/stream.c | 18 +++--
2 files changed, 149 insertions(+), 87 deletions(-)
diff --git a/src/proto_http.c b/src/proto_http.c
index 0662041..c81409f 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -2555,19 +2555,25 @@ resume_execution:
break;
case ACT_HTTP_SET_HDR:
- case ACT_HTTP_ADD_HDR:
+ case ACT_HTTP_ADD_HDR: {
/* The scope of the trash buffer must be limited to this function. The
* build_logline() function can execute a lot of other function which
* can use the trash buffer. So for limiting the scope of this global
* buffer, we build first the header value using build_logline, and
* after we store the header name.
*/
+ struct chunk *replace;
+
+ replace = alloc_trash_chunk();
+ if (!replace)
+ return HTTP_RULE_RES_BADREQ;
+
len = rule->arg.hdr_add.name_len + 2,
- len += build_logline(s, trash.str + len, trash.size - len, &rule->arg.hdr_add.fmt);
- memcpy(trash.str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
- trash.str[rule->arg.hdr_add.name_len] = ':';
- trash.str[rule->arg.hdr_add.name_len + 1] = ' ';
- trash.len = len;
+ len += build_logline(s, replace->str + len, replace->size - len, &rule->arg.hdr_add.fmt);
+ memcpy(replace->str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
+ replace->str[rule->arg.hdr_add.name_len] = ':';
+ replace->str[rule->arg.hdr_add.name_len + 1] = ' ';
+ replace->len = len;
if (rule->action == ACT_HTTP_SET_HDR) {
/* remove all occurrences of the header */
@@ -2578,90 +2584,105 @@ resume_execution:
}
}
- http_header_add_tail2(&txn->req, &txn->hdr_idx, trash.str, trash.len);
+ http_header_add_tail2(&txn->req, &txn->hdr_idx, replace->str, replace->len);
+
+ free_trash_chunk(replace);
break;
+ }
case ACT_HTTP_DEL_ACL:
case ACT_HTTP_DEL_MAP: {
struct pat_ref *ref;
- char *key;
- int len;
+ struct chunk *key;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
/* collect key */
- len = build_logline(s, trash.str, trash.size, &rule->arg.map.key);
- key = trash.str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* perform update */
/* returned code: 1=ok, 0=ko */
- pat_ref_delete(ref, key);
+ pat_ref_delete(ref, key->str);
+ free_trash_chunk(key);
break;
}
case ACT_HTTP_ADD_ACL: {
struct pat_ref *ref;
- char *key;
- struct chunk *trash_key;
- int len;
-
- trash_key = get_trash_chunk();
+ struct chunk *key;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
/* collect key */
- len = build_logline(s, trash_key->str, trash_key->size, &rule->arg.map.key);
- key = trash_key->str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* perform update */
/* add entry only if it does not already exist */
- if (pat_ref_find_elt(ref, key) == NULL)
- pat_ref_add(ref, key, NULL, NULL);
+ if (pat_ref_find_elt(ref, key->str) == NULL)
+ pat_ref_add(ref, key->str, NULL, NULL);
+ free_trash_chunk(key);
break;
}
case ACT_HTTP_SET_MAP: {
struct pat_ref *ref;
- char *key, *value;
- struct chunk *trash_key, *trash_value;
- int len;
-
- trash_key = get_trash_chunk();
- trash_value = get_trash_chunk();
+ struct chunk *key, *value;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
+ /* allocate value */
+ value = alloc_trash_chunk();
+ if (!value) {
+ free_trash_chunk(key);
+ return HTTP_RULE_RES_BADREQ;
+ }
+
/* collect key */
- len = build_logline(s, trash_key->str, trash_key->size, &rule->arg.map.key);
- key = trash_key->str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* collect value */
- len = build_logline(s, trash_value->str, trash_value->size, &rule->arg.map.value);
- value = trash_value->str;
- value[len] = '\0';
+ value->len = build_logline(s, value->str, value->size, &rule->arg.map.value);
+ value->str[value->len] = '\0';
/* perform update */
- if (pat_ref_find_elt(ref, key) != NULL)
+ if (pat_ref_find_elt(ref, key->str) != NULL)
/* update entry if it exists */
- pat_ref_set(ref, key, value, NULL);
+ pat_ref_set(ref, key->str, value->str, NULL);
else
/* insert a new entry */
- pat_ref_add(ref, key, value, NULL);
+ pat_ref_add(ref, key->str, value->str, NULL);
+ free_trash_chunk(key);
+ free_trash_chunk(value);
break;
}
@@ -2824,13 +2845,20 @@ resume_execution:
break;
case ACT_HTTP_SET_HDR:
- case ACT_HTTP_ADD_HDR:
- chunk_printf(&trash, "%s: ", rule->arg.hdr_add.name);
- memcpy(trash.str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
- trash.len = rule->arg.hdr_add.name_len;
- trash.str[trash.len++] = ':';
- trash.str[trash.len++] = ' ';
- trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->arg.hdr_add.fmt);
+ case ACT_HTTP_ADD_HDR: {
+ struct chunk *replace;
+
+ replace = alloc_trash_chunk();
+ if (!replace)
+ return HTTP_RULE_RES_BADREQ;
+
+ chunk_printf(replace, "%s: ", rule->arg.hdr_add.name);
+ memcpy(replace->str, rule->arg.hdr_add.name, rule->arg.hdr_add.name_len);
+ replace->len = rule->arg.hdr_add.name_len;
+ replace->str[replace->len++] = ':';
+ replace->str[replace->len++] = ' ';
+ replace->len += build_logline(s, replace->str + replace->len, replace->size - replace->len,
+ &rule->arg.hdr_add.fmt);
if (rule->action == ACT_HTTP_SET_HDR) {
/* remove all occurrences of the header */
@@ -2840,90 +2868,105 @@ resume_execution:
http_remove_header2(&txn->rsp, &txn->hdr_idx, &ctx);
}
}
- http_header_add_tail2(&txn->rsp, &txn->hdr_idx, trash.str, trash.len);
+ http_header_add_tail2(&txn->rsp, &txn->hdr_idx, replace->str, replace->len);
+
+ free_trash_chunk(replace);
break;
+ }
case ACT_HTTP_DEL_ACL:
case ACT_HTTP_DEL_MAP: {
struct pat_ref *ref;
- char *key;
- int len;
+ struct chunk *key;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
/* collect key */
- len = build_logline(s, trash.str, trash.size, &rule->arg.map.key);
- key = trash.str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* perform update */
/* returned code: 1=ok, 0=ko */
- pat_ref_delete(ref, key);
+ pat_ref_delete(ref, key->str);
+ free_trash_chunk(key);
break;
}
case ACT_HTTP_ADD_ACL: {
struct pat_ref *ref;
- char *key;
- struct chunk *trash_key;
- int len;
-
- trash_key = get_trash_chunk();
+ struct chunk *key;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
/* collect key */
- len = build_logline(s, trash_key->str, trash_key->size, &rule->arg.map.key);
- key = trash_key->str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* perform update */
/* check if the entry already exists */
- if (pat_ref_find_elt(ref, key) == NULL)
- pat_ref_add(ref, key, NULL, NULL);
+ if (pat_ref_find_elt(ref, key->str) == NULL)
+ pat_ref_add(ref, key->str, NULL, NULL);
+ free_trash_chunk(key);
break;
}
case ACT_HTTP_SET_MAP: {
struct pat_ref *ref;
- char *key, *value;
- struct chunk *trash_key, *trash_value;
- int len;
-
- trash_key = get_trash_chunk();
- trash_value = get_trash_chunk();
+ struct chunk *key, *value;
/* collect reference */
ref = pat_ref_lookup(rule->arg.map.ref);
if (!ref)
continue;
+ /* allocate key */
+ key = alloc_trash_chunk();
+ if (!key)
+ return HTTP_RULE_RES_BADREQ;
+
+ /* allocate value */
+ value = alloc_trash_chunk();
+ if (!value) {
+ free_trash_chunk(key);
+ return HTTP_RULE_RES_BADREQ;
+ }
+
/* collect key */
- len = build_logline(s, trash_key->str, trash_key->size, &rule->arg.map.key);
- key = trash_key->str;
- key[len] = '\0';
+ key->len = build_logline(s, key->str, key->size, &rule->arg.map.key);
+ key->str[key->len] = '\0';
/* collect value */
- len = build_logline(s, trash_value->str, trash_value->size, &rule->arg.map.value);
- value = trash_value->str;
- value[len] = '\0';
+ value->len = build_logline(s, value->str, value->size, &rule->arg.map.value);
+ value->str[value->len] = '\0';
/* perform update */
- if (pat_ref_find_elt(ref, key) != NULL)
+ if (pat_ref_find_elt(ref, key->str) != NULL)
/* update entry if it exists */
- pat_ref_set(ref, key, value, NULL);
+ pat_ref_set(ref, key->str, value->str, NULL);
else
/* insert a new entry */
- pat_ref_add(ref, key, value, NULL);
+ pat_ref_add(ref, key->str, value->str, NULL);
+ free_trash_chunk(key);
+ free_trash_chunk(value);
break;
}
@@ -11678,15 +11721,26 @@ void http_set_status(unsigned int status, const char *reason, struct stream *s)
enum act_return http_action_set_req_line(struct act_rule *rule, struct proxy *px,
struct session *sess, struct stream *s, int flags)
{
- chunk_reset(&trash);
+ struct chunk *replace;
+ enum act_return ret = ACT_RET_ERR;
+
+ replace = alloc_trash_chunk();
+ if (!replace)
+ goto leave;
/* If we have to create a query string, prepare a '?'. */
if (rule->arg.http.action == 2)
- trash.str[trash.len++] = '?';
- trash.len += build_logline(s, trash.str + trash.len, trash.size - trash.len, &rule->arg.http.logfmt);
+ replace->str[replace->len++] = '?';
+ replace->len += build_logline(s, replace->str + replace->len, replace->size - replace->len,
+ &rule->arg.http.logfmt);
- http_replace_req_line(rule->arg.http.action, trash.str, trash.len, px, s);
- return ACT_RET_CONT;
+ http_replace_req_line(rule->arg.http.action, replace->str, replace->len, px, s);
+
+ ret = ACT_RET_CONT;
+
+leave:
+ free_trash_chunk(replace);
+ return ret;
}
/* This function is just a compliant action wrapper for "set-status". */
diff --git a/src/stream.c b/src/stream.c
index 2e9b95c..a78ee96 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -1186,13 +1186,21 @@ static int process_switching_rules(struct stream *s, struct channel *req, int an
* If we can't resolve the name, or if any error occurs, break
* the loop and fallback to the default backend.
*/
- struct proxy *backend;
+ struct proxy *backend = NULL;
if (rule->dynamic) {
- struct chunk *tmp = get_trash_chunk();
- if (!build_logline(s, tmp->str, tmp->size, &rule->be.expr))
- break;
- backend = proxy_be_by_name(tmp->str);
+ struct chunk *tmp;
+
+ tmp = alloc_trash_chunk();
+ if (!tmp)
+ goto sw_failed;
+
+ if (build_logline(s, tmp->str, tmp->size, &rule->be.expr))
+ backend = proxy_be_by_name(tmp->str);
+
+ free_trash_chunk(tmp);
+ tmp = NULL;
+
if (!backend)
break;
}
--
2.7.4
