Welcome! Log In Create A New Profile

Advanced

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

Roman Arutyunyan
September 28, 2022 06:04AM
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++) {
+ 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;
}
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
Subject Author Views Posted

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

Dipl. Ing. Sergey Brester via nginx-devel 341 September 26, 2022 05:18PM

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

Maxim Dounin 45 September 27, 2022 05:08PM

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

Roman Arutyunyan 56 September 28, 2022 06:04AM

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

Dipl. Ing. Sergey Brester via nginx-devel 50 September 28, 2022 07:08AM

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

splitice 77 September 28, 2022 09:34AM

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

Roman Arutyunyan 82 September 28, 2022 11:50AM

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

Maxim Dounin 70 October 09, 2022 05:56PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 81
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready