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 <a...@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-le...@nginx.org
