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

[Dipl. Ing. Sergey Brester]

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


Regards,
Sergey. 

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"
@@ -139,6 +139,20 @@ skip:

 invalid:

+ p = buf;
+ while (p < last) {
+ const u_char c = *p;
+ switch (c) {
+ case LF:
+ case CR:
+ *p = (u_char)'x1a';
+ break;
+ case '"':
+ *p = (u_char)''';
+ break;
+ default:
+ if (c >= 0x80) {*p = '?';}
+ break;
+ }
+ p++;
+ }
 ngx_log_error(NGX_LOG_ERR, c->log, 0,
 "broken header: "%*s"", (size_t) (last - buf), buf);

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20220926/9ba2e0cb/attachment.htm>