Hello!
On Wed, Sep 28, 2022 at 02:02:30PM +0400, Roman Arutyunyan wrote:
> Hi Sergey,
>
> On Mon, Sep 26, 2022 at 11:16:05PM +0200, Dipl. Ing. Sergey Brester via nginx-devel wrote:
> >
> >
> > Hi,
> >
> > below is a patch to fix a weakness by logging of broken header by
> > incorrect proxy protocol.
> >
> > If some service (IDS/IPS) analyzing or monitoring log-file, regularly
> > formatted lines may be simply confused with lines written not escaped
> > directly from buffer supplied from foreign source.
> > Not to mention it may open a certain vector allowing "injection" of user
> > input in order to avoid detection of failures or even to simulate
> > malicious traffic from legitimate service.
> >
> > How to reproduce:
> >
> > - enable proxy_protocol for listener and start nginx (here localhost on
> > port 80);
> > - echo 'set s [socket localhost 80]; puts $s "testntestntest"; close $s'
> > | tclsh
> >
> > Error-log before fix:
> >
> > 2022/09/26 19:29:58 [error] 10104#17144: *3 broken header: "test
> > test
> > test
> > " while reading PROXY protocol, client: 127.0.0.1, server: 0.0.0.0:80
> >
> > Error-log after fix:
> >
> > 2022/09/26 22:48:50 [error] 13868#6132: *1 broken header:
> > "test→→test→→test→→" while reading PROXY protocol, client: 127.0.0.1,
> > server: 0.0.0.0:80
> >
> > It is not advisable to log such foreign user input unescaped to the
> > formatted log-file: instead of "...ntestn..." the attacker can write
> > correctly formatted line simulating a 401-, 403-failure or rate-limit
> > overran, so IDS could block a innocent service or mistakenly ban
> > legitimate user.
> >
> > The patch proposes simplest escape (LF/CR-char with →, double quote with
> > single quote and additionally every char larger or equal than 0x80 to
> > avoid possible logging of "broken" utf-8 sequences or unsupported
> > surrogates, just as a safest variant for not-valid foreign buffer)
> > in-place in the malicious buffer directly (without mem-alloc, etc).
> >
> > Real life example -
> > https://github.com/fail2ban/fail2ban/issues/3303#issuecomment-1148691902
>
> Thanks for reporting this. The issue indeed needs to be fixed. Attached is
> a patch similar to yours that does this. I don't think we need to do anything
> beyond just cutting the first line since there's another similar place in
> nginx - ngx_http_log_error_handler(), where exactly that is implemented.
>
> Whether we need to skip special characters when logging to nginx log is
> a topic for a bigger discussion and this will require a much bigger patch.
> I suggest that we only limit user data to the first line now.
>
> [..]
>
> --
> Roman Arutyunyan
> # HG changeset patch
> # User Roman Arutyunyan <arut@nginx.com>
> # Date 1664359213 -14400
> # Wed Sep 28 14:00:13 2022 +0400
> # Node ID 001b2449cfd730fd688a7298458e25113c15a947
> # Parent 615268a957ab930dc4be49fe5f6f88cd7e377f12
> Log only the first line of user input on PROXY protocol v1 error.
>
> Previously, all received user input was logged. If a multi-line text was
> received from client and logged, it could reduce log readability and also make
> it harder to parse nginx log by scripts. The change brings to PROXY protocol
> the same behavior that exists for HTTP request line in
> ngx_http_log_error_handler().
>
> diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
> --- a/src/core/ngx_proxy_protocol.c
> +++ b/src/core/ngx_proxy_protocol.c
> @@ -185,8 +185,14 @@ skip:
>
> invalid:
>
> + for (p = buf; p != last; p++) {
I would rather prefer "p < last", as in
ngx_http_log_error_handler() and in the skip section of this
function (and many other places).
While there is no real difference, the "p < last" looks more in
line with the existing code, and also slightly safer if due to a
bug elsewhere "buf" will happen to be larger than "last".
> + if (*p == CR || *p == LF) {
> + break;
> + }
> + }
> +
> ngx_log_error(NGX_LOG_ERR, c->log, 0,
> - "broken header: \"%*s\"", (size_t) (last - buf), buf);
> + "broken header: \"%*s\"", (size_t) (p - buf), buf);
>
> return NULL;
> }
Otherwise looks good.
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
