Hi all,
Stephan (in Cc) reported me two nice segfaults in the config parser when
feeding haproxy with some horribly fuzzed invalid configurations. To make
it clear, it happens only when haproxy *fails* to start due to an error.
But it's not a reason for failing the dirty way. Every time it was a
problem in smp_resolve_args() which is used to resolve acl args.
The root cause of the issue is that there are certain types of errors
where it's very tricky to unroll what has been started (eg: add multiple
keywords to a list then you have to remove them and exactly them, taking
care not to free a shared memory are if at least one remains because this
one will be freed later), etc.
The first bug was a use-after-free causing all sort of random things like
memory corruption or an infinite loop when trying to exit, which can be a
problem for those aggregating configs from customers. The second one was
a "more conventional" null dereference. I could fix both of them but I
realized that the deeper reason is that we try to perform all the cross-
reference checks after we've met such errors, which doesn't make sense
and even causes some absurd errors to be reported. So I wrote the simple
patch below for 1.8 and I think it would make sense to backport this into
earlier versions to make everyone's life easier. It would also make the
parser much more robust against such issues in the future.
Now the question is : this is not a bug fix but a small improvement which
may have some impact on those being used to read error reports, so does
anyone have any objection against this being backported (if so to regarding
a specific version maybe) ?
For reference, in the patch there's an example of config which produces
stupid errors right now, with their output and the same output after the
patch.
Please just let me know, I'd personally prefer to backport it.
Thanks,
Willy
--
>From b83dc3d2ef5ffa882aed926ee4d6a82bd94024f0 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Wed, 19 Apr 2017 11:24:07 +0200
Subject: MEDIUM: config: don't check config validity when there are fatal
errors
Overall we do have an issue with the severity of a number of errors. Most
fatal errors are reported with ERR_FATAL (which prevents startup) and not
ERR_ABORT (which stops parsing ASAP), but check_config_validity() is still
called on ERR_FATAL, and will most of the time report bogus errors. This
is what caused smp_resolve_args() to be called on a number of unparsable
ACLs, and it also is what reports incorrect ordering or unresolvable
section names when certain entries could not be properly parsed.
This patch stops this domino effect by simply aborting before trying to
further check and resolve the configuration when it's already know that
there are fatal errors.
A concrete example comes from this config :
userlist users :
user foo insecure-password bar
listen foo
bind :1234
mode htttp
timeout client 10S
timeout server 10s
timeout connect 10s
stats uri /stats
stats http-request auth unless { http_auth(users) }
http-request redirect location /index.html if { path / }
It contains a colon after the userlist name, a typo in the client timeout value,
another one in "mode http" which cause some other configuration elements not to
be properly handled.
Previously it would confusingly report :
[ALERT] 108/114851 (20224) : parsing [err-report.cfg:1] : 'userlist' cannot
handle unexpected argument ':'.
[ALERT] 108/114851 (20224) : parsing [err-report.cfg:6] : unknown proxy mode
'htttp'.
[ALERT] 108/114851 (20224) : parsing [err-report.cfg:7] : unexpected
character 'S' in 'timeout client'
[ALERT] 108/114851 (20224) : Error(s) found in configuration file :
err-report.cfg
[ALERT] 108/114851 (20224) : parsing [err-report.cfg:11] : unable to find
userlist 'users' referenced in arg 1 of ACL keyword 'http_auth' in proxy 'foo'.
[WARNING] 108/114851 (20224) : config : missing timeouts for proxy 'foo'.
| While not properly invalid, you will certainly encounter various problems
| with such a configuration. To fix this, please ensure that all following
| timeouts are set to a non-zero value: 'client', 'connect', 'server'.
[WARNING] 108/114851 (20224) : config : 'stats' statement ignored for proxy
'foo' as it requires HTTP mode.
[WARNING] 108/114851 (20224) : config : 'http-request' rules ignored for
proxy 'foo' as they require HTTP mode.
[ALERT] 108/114851 (20224) : Fatal errors found in configuration.
The "requires HTTP mode" errors are just pollution resulting from the
improper spelling of this mode earlier. The unresolved reference to the
userlist is caused by the extra colon on the declaration, and the warning
regarding the missing timeouts is caused by the wrong character.
Now it more accurately reports :
[ALERT] 108/114900 (20225) : parsing [err-report.cfg:1] : 'userlist' cannot
handle unexpected argument ':'.
[ALERT] 108/114900 (20225) : parsing [err-report.cfg:6] : unknown proxy mode
'htttp'.
[ALERT] 108/114900 (20225) : parsing [err-report.cfg:7] : unexpected
character 'S' in 'timeout client'
[ALERT] 108/114900 (20225) : Error(s) found in configuration file :
err-report.cfg
[ALERT] 108/114900 (20225) : Fatal errors found in configuration.
Despite not really a fix, this patch should be backported at least to 1.7,
possibly even 1.6, and 1.5 since it hardens the config parser against
certain bad situations like the recently reported use-after-free and the
last null dereference.
---
src/haproxy.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/haproxy.c b/src/haproxy.c
index 2b1db00..02f90a8 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1035,6 +1035,15 @@ static void init(int argc, char **argv)
exit(1);
}
+ /* do not try to resolve arguments nor to spot inconsistencies when
+ * the configuration contains fatal errors caused by files not found
+ * or failed memory allocations.
+ */
+ if (err_code & (ERR_ABORT|ERR_FATAL)) {
+ Alert("Fatal errors found in configuration.\n");
+ exit(1);
+ }
+
pattern_finalize_config();
err_code |= check_config_validity();
--
1.7.12.1
