mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-09-21 13:51:26 +02:00
Code Issues Projects Releases Wiki Activity

BUG/MAJOR: http: fix breakage of "reqdeny" causing random crashes

Browse Source 
Commit 108b1dd ("MEDIUM: http: configurable http result codes for
http-request deny") introduced in 1.6-dev2 was incomplete. It introduced
a new field "rule_deny_status" into struct http_txn, which is filled only
by actions "http-request deny" and "http-request tarpit". It's then used
in the deny code path to emit the proper error message, but is used
uninitialized when the deny comes from a "reqdeny" rule, causing random
behaviours ranging from returning a 200, an empty response, or crashing
the process. Often upon startup only 200 was returned but after the fields
are used the crash happens. This can be sped up using -dM.

There's no need at all for storing this status in the http_txn struct
anyway since it's used immediately after being set. Let's store it in
a temporary variable instead which is passed as an argument to function
http_req_get_intercept_rule().

As an extra benefit, removing it from struct http_txn reduced the size
of this struct by 8 bytes.

This fix must be backported to 1.6 where the bug was detected. Special
thanks to Falco Schmutz for his detailed report including an exploitable
core and a reproducer.
This commit is contained in:
Willy Tarreau 2016-05-25 16:23:59 +02:00
parent 1516fe31dd
commit 58727ec088
2 changed files with 13 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
include/types/proto_http.h
View File

@ -366,7 +366,6 @@ struct http_txn {
unsigned int flags; /* transaction flags */ unsigned int flags; /* transaction flags */
enum http_meth_t meth; /* HTTP method */ enum http_meth_t meth; /* HTTP method */
/* 1 unused byte here */ /* 1 unused byte here */
short rule_deny_status; /* HTTP status from rule when denying */
short status; /* HTTP status from the server, negative if from proxy */ short status; /* HTTP status from the server, negative if from proxy */
char *uri; /* first line if log needed, NULL otherwise */ char *uri; /* first line if log needed, NULL otherwise */

21
src/proto_http.c
View File

@ -3276,10 +3276,12 @@ static int http_transform_header(struct stream* s, struct http_msg *msg,
* further processing of the request (auth, deny, ...), and defaults to * further processing of the request (auth, deny, ...), and defaults to
* HTTP_RULE_RES_STOP if it executed all rules or stopped on an allow, or * HTTP_RULE_RES_STOP if it executed all rules or stopped on an allow, or
* HTTP_RULE_RES_CONT if the last rule was reached. It may set the TX_CLTARPIT * HTTP_RULE_RES_CONT if the last rule was reached. It may set the TX_CLTARPIT
* on txn->flags if it encounters a tarpit rule. * on txn->flags if it encounters a tarpit rule. If <deny_status> is not NULL
* and a deny/tarpit rule is matched, it will be filled with this rule's deny
* status.
*/ */
enum rule_result enum rule_result
http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct stream *s) http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct stream *s, int *deny_status)
{ {
struct session *sess = strm_sess(s); struct session *sess = strm_sess(s);
struct http_txn *txn = s->txn; struct http_txn *txn = s->txn;
@ -3325,12 +3327,14 @@ resume_execution:
return HTTP_RULE_RES_STOP; return HTTP_RULE_RES_STOP;
case ACT_ACTION_DENY: case ACT_ACTION_DENY:
txn->rule_deny_status = rule->deny_status; if (deny_status)
*deny_status = rule->deny_status;
return HTTP_RULE_RES_DENY; return HTTP_RULE_RES_DENY;
case ACT_HTTP_REQ_TARPIT: case ACT_HTTP_REQ_TARPIT:
txn->flags |= TX_CLTARPIT; txn->flags |= TX_CLTARPIT;
txn->rule_deny_status = rule->deny_status; if (deny_status)
*deny_status = rule->deny_status;
return HTTP_RULE_RES_DENY; return HTTP_RULE_RES_DENY;
case ACT_HTTP_REQ_AUTH: case ACT_HTTP_REQ_AUTH:
@ -4090,6 +4094,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
struct redirect_rule *rule; struct redirect_rule *rule;
struct cond_wordlist *wl; struct cond_wordlist *wl;
enum rule_result verdict; enum rule_result verdict;
int deny_status = HTTP_ERR_403;
if (unlikely(msg->msg_state < HTTP_MSG_BODY)) { if (unlikely(msg->msg_state < HTTP_MSG_BODY)) {
/* we need more data */ /* we need more data */
@ -4110,7 +4115,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
/* evaluate http-request rules */ /* evaluate http-request rules */
if (!LIST_ISEMPTY(&px->http_req_rules)) { if (!LIST_ISEMPTY(&px->http_req_rules)) {
verdict = http_req_get_intercept_rule(px, &px->http_req_rules, s); verdict = http_req_get_intercept_rule(px, &px->http_req_rules, s, &deny_status);
switch (verdict) { switch (verdict) {
case HTTP_RULE_RES_YIELD: /* some data miss, call the function later. */ case HTTP_RULE_RES_YIELD: /* some data miss, call the function later. */
@ -4156,7 +4161,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
/* parse the whole stats request and extract the relevant information */ /* parse the whole stats request and extract the relevant information */
http_handle_stats(s, req); http_handle_stats(s, req);
verdict = http_req_get_intercept_rule(px, &px->uri_auth->http_req_rules, s); verdict = http_req_get_intercept_rule(px, &px->uri_auth->http_req_rules, s, &deny_status);
/* not all actions implemented: deny, allow, auth */ /* not all actions implemented: deny, allow, auth */
if (verdict == HTTP_RULE_RES_DENY) /* stats http-request deny */ if (verdict == HTTP_RULE_RES_DENY) /* stats http-request deny */
@ -4285,9 +4290,9 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
manage_client_side_cookies(s, req); manage_client_side_cookies(s, req);
txn->flags |= TX_CLDENY; txn->flags |= TX_CLDENY;
txn->status = http_err_codes[txn->rule_deny_status]; txn->status = http_err_codes[deny_status];
s->logs.tv_request = now; s->logs.tv_request = now;
http_reply_and_close(s, txn->status, http_error_message(s, txn->rule_deny_status)); http_reply_and_close(s, txn->status, http_error_message(s, deny_status));
stream_inc_http_err_ctr(s); stream_inc_http_err_ctr(s);
sess->fe->fe_counters.denied_req++; sess->fe->fe_counters.denied_req++;
if (sess->fe != s->be) if (sess->fe != s->be)