Welcome! Log In Create A New Profile

Advanced

Backport proposal, opinion needed

Posted by Willy Tarreau 
Willy Tarreau
Backport proposal, opinion needed
April 19, 2017 12:20PM
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 <[email protected]>
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
Pavlos Parissis
Re: Backport proposal, opinion needed
April 19, 2017 01:10PM
On 19/04/2017 12:13 μμ, Willy Tarreau wrote:
> 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) ?
>


I also believe that it should be backported at least to 1.7 version[1].
It makes the output more clear and squeaks only the relevant bad config lines.

Cheers,
Pavlos


[1] IMHO: Users of 1.5 version should upgrade to 1.7, I don't see
any valid reason to stay on 1.5. From my personal experience I can tell
that 1.7 version is a rock solid release.
Willy Tarreau
Re: Backport proposal, opinion needed
April 19, 2017 02:50PM
Hi Pavlos,

On Wed, Apr 19, 2017 at 01:02:55PM +0200, Pavlos Parissis wrote:
> I also believe that it should be backported at least to 1.7 version[1].
> It makes the output more clear and squeaks only the relevant bad config lines.

Thanks for your feedback!

> [1] IMHO: Users of 1.5 version should upgrade to 1.7, I don't see
> any valid reason to stay on 1.5. From my personal experience I can tell
> that 1.7 version is a rock solid release.

It really depends on each user's level of expectation. Those willing to
"deploy and forget" will more easily go with 1.6 or 1.5 than 1.7 which
will still receive some important fixes for a while. However backporting
the above fix only to 1.7 adds another incentive for upgrading and at the
same time maintains the principle of least surprise for older versions so
that's probably a good tradeoff.

thanks!
Willy
Aleksandar Lazic
Re: Backport proposal, opinion needed
April 19, 2017 10:50PM
Am 19-04-2017 13:02, schrieb Pavlos Parissis:
> On 19/04/2017 12:13 μμ, Willy Tarreau wrote:
>> 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) ?
>>
>
>
> I also believe that it should be backported at least to 1.7 version[1].
> It makes the output more clear and squeaks only the relevant bad config
> lines.

+1

> Cheers,
> Pavlos
>
>
> [1] IMHO: Users of 1.5 version should upgrade to 1.7, I don't see
> any valid reason to stay on 1.5. From my personal experience I can tell
> that 1.7 version is a rock solid release.
Willy Tarreau
Re: Backport proposal, opinion needed
April 26, 2017 03:50PM
On Wed, Apr 19, 2017 at 10:39:16PM +0200, Aleksandar Lazic wrote:
> Am 19-04-2017 13:02, schrieb Pavlos Parissis:
> > I also believe that it should be backported at least to 1.7 version[1].
> > It makes the output more clear and squeaks only the relevant bad config
> > lines.
>
> +1

Thanks for your responses, I've now backported it to 1.7 as suggested.

Willy
Sorry, only registered users may post in this forum.

Click here to login