[PATCH] fix weakness by logging of broken header by incorect proxy protocol (IDS/IPS/LOG-analysis)

[Maxim Dounin]

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 at 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/