Fix reqdeny causing random crashes (CVE-2016-5360, #1346672)
This commit is contained in:
Ryan O'Hara
parent
9f2d883fc0
commit
dcad1c9699
122
fix-reqdeny-crash.patch
Normal file
122
fix-reqdeny-crash.patch
Normal file
@ -0,0 +1,122 @@
|
||||
From 60f01f8c89e4fb2723d5a9f2046286e699567e0b Mon Sep 17 00:00:00 2001
|
||||
From: Willy Tarreau <w@1wt.eu>
|
||||
Date: Wed, 25 May 2016 16:23:59 +0200
|
||||
Subject: [PATCH] BUG/MAJOR: http: fix breakage of "reqdeny" causing random
|
||||
crashes
|
||||
|
||||
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.
|
||||
(cherry picked from commit 58727ec088e55f739b146cff3baa955f8d1b2a3e)
|
||||
---
|
||||
include/types/proto_http.h | 1 -
|
||||
src/proto_http.c | 21 +++++++++++++--------
|
||||
2 files changed, 13 insertions(+), 9 deletions(-)
|
||||
|
||||
diff --git a/include/types/proto_http.h b/include/types/proto_http.h
|
||||
index e5e9667..c3a73ef 100644
|
||||
--- a/include/types/proto_http.h
|
||||
+++ b/include/types/proto_http.h
|
||||
@@ -362,7 +362,6 @@ struct http_txn {
|
||||
unsigned int flags; /* transaction flags */
|
||||
enum http_meth_t meth; /* HTTP method */
|
||||
/* 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 */
|
||||
|
||||
char *uri; /* first line if log needed, NULL otherwise */
|
||||
diff --git a/src/proto_http.c b/src/proto_http.c
|
||||
index 59cd5d2..6eac62b 100644
|
||||
--- a/src/proto_http.c
|
||||
+++ b/src/proto_http.c
|
||||
@@ -3490,10 +3490,12 @@ static int http_transform_header(struct stream* s, struct http_msg *msg,
|
||||
* 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_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
|
||||
-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 http_txn *txn = s->txn;
|
||||
@@ -3539,12 +3541,14 @@ resume_execution:
|
||||
return HTTP_RULE_RES_STOP;
|
||||
|
||||
case ACT_ACTION_DENY:
|
||||
- txn->rule_deny_status = rule->deny_status;
|
||||
+ if (deny_status)
|
||||
+ *deny_status = rule->deny_status;
|
||||
return HTTP_RULE_RES_DENY;
|
||||
|
||||
case ACT_HTTP_REQ_TARPIT:
|
||||
txn->flags |= TX_CLTARPIT;
|
||||
- txn->rule_deny_status = rule->deny_status;
|
||||
+ if (deny_status)
|
||||
+ *deny_status = rule->deny_status;
|
||||
return HTTP_RULE_RES_DENY;
|
||||
|
||||
case ACT_HTTP_REQ_AUTH:
|
||||
@@ -4303,6 +4307,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
|
||||
struct redirect_rule *rule;
|
||||
struct cond_wordlist *wl;
|
||||
enum rule_result verdict;
|
||||
+ int deny_status = HTTP_ERR_403;
|
||||
|
||||
if (unlikely(msg->msg_state < HTTP_MSG_BODY)) {
|
||||
/* we need more data */
|
||||
@@ -4323,7 +4328,7 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
|
||||
|
||||
/* evaluate http-request 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) {
|
||||
case HTTP_RULE_RES_YIELD: /* some data miss, call the function later. */
|
||||
@@ -4369,7 +4374,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 */
|
||||
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 */
|
||||
|
||||
if (verdict == HTTP_RULE_RES_DENY) /* stats http-request deny */
|
||||
@@ -4500,9 +4505,9 @@ int http_process_req_common(struct stream *s, struct channel *req, int an_bit, s
|
||||
manage_client_side_cookies(s, req);
|
||||
|
||||
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;
|
||||
- stream_int_retnclose(&s->si[0], http_error_message(s, txn->rule_deny_status));
|
||||
+ stream_int_retnclose(&s->si[0], http_error_message(s, deny_status));
|
||||
stream_inc_http_err_ctr(s);
|
||||
sess->fe->fe_counters.denied_req++;
|
||||
if (sess->fe != s->be)
|
||||
--
|
||||
2.4.3
|
||||
|
||||
@ -8,14 +8,14 @@
|
||||
|
||||
Name: haproxy
|
||||
Version: 1.6.5
|
||||
Release: 2%{?dist}
|
||||
Release: 3%{?dist}
|
||||
Summary: HAProxy reverse proxy for high availability environments
|
||||
|
||||
Group: System Environment/Daemons
|
||||
License: GPLv2+
|
||||
|
||||
URL: http://www.haproxy.org/
|
||||
Source0: http://www.haproxy.org/download/1.5/src/haproxy-%{version}.tar.gz
|
||||
Source0: http://www.haproxy.org/download/1.6/src/haproxy-%{version}.tar.gz
|
||||
Source1: %{name}.service
|
||||
Source2: %{name}.cfg
|
||||
Source3: %{name}.logrotate
|
||||
@ -24,6 +24,7 @@ Source5: halog.1
|
||||
|
||||
Patch0: halog-unused-variables.patch
|
||||
Patch1: iprange-return-type.patch
|
||||
Patch2: fix-reqdeny-crash.patch
|
||||
|
||||
BuildRequires: lua-devel
|
||||
BuildRequires: pcre-devel
|
||||
@ -54,6 +55,7 @@ availability environments. Indeed, it can:
|
||||
%setup -q
|
||||
%patch0 -p0
|
||||
%patch1 -p0
|
||||
%patch2 -p1
|
||||
|
||||
%build
|
||||
regparm_opts=
|
||||
@ -139,6 +141,9 @@ exit 0
|
||||
%attr(-,%{haproxy_user},%{haproxy_group}) %dir %{haproxy_home}
|
||||
|
||||
%changelog
|
||||
* Wed Jun 15 2016 Ryan O'Hara <rohara@redhat.com> - 1.6.5-3
|
||||
- Fix reqdeny causing random crashes (CVE-2016-5360, #1346672)
|
||||
|
||||
* Fri Jun 03 2016 Ryan O'Hara <rohara@redhat.com> - 1.6.5-2
|
||||
- Utilize system-wide crypto-policies (#1256253)
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user