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

[Dipl. Ing. Sergey Brester]

Sure, this was also my first intention. Just after all I thought the
whole buffer 
could be better in order to provide a possibility to debug for someone
searching 
for a bug. But there are another aids that would help, so indeed let it
be so.

As for the rest, well it is surely a subject of different discussion.
However I think 
someone who monitoring logs with foreign data is able to ignore wrong
chars
or unsupported surrogates using appropriate encoding facilities, but...
But I would suggest at least to replace a quote-char to provide
opportunity
to bypass the buffer in quotes with something similar this `... header:
"[^"]*" ...`
if parsing of such lines is needed. 

So may be this one either: 

+ for (p = buf; p != last; p++) {
+ if (*p == '"') {
+ *p = '''; continue;
+ }
+ if (*p == CR || *p == LF) {
+ break;
+ }
+ }

Although I don't believe the safe data (like IP, etc) shall take place
after "unsafe" 
(foreign) input (especially of variable length), but it is rather a
matter of common 
logging format for the error-log.
I mean normally one would rather expect something like that: 

- [error] 13868#6132: *1 broken header: "...unsafe..." while reading
PROXY protocol, client: 127.0.0.1, server: 0.0.0.0:80

+ [error] 13868#6132: while reading PROXY protocol, client: 127.0.0.1,
server: 0.0.0.0:80 - *1 broken header: "...unsafe..."

Unfortunatelly errorlog is not configurable at the moment at all. 

28.09.2022 12:02, Roman Arutyunyan wrote: 

> Hi Sergey,
> 
> 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 --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20220928/c956ad67/attachment.htm>