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

[Roman Arutyunyan]

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
-------------- next part --------------
# 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++) {
+        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;
 }