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

[Roman Arutyunyan]

Hi,

On Wed, Sep 28, 2022 at 01:07:18PM +0200, Dipl. Ing. Sergey Brester wrote:
>  
> 
> 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.

A possible option would be to log data in hex, but that would make the log
less readable.

> 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;
> + }
> + }

I don't think we'll do something like this.  Spoiling the buffer would be
confusing for people.

> 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. 

You're right.  We try to follow this rule where possible, but everything
starting from "while" till the end of line is added automatically by 
the http log handler ngx_http_log_error().  Changing this would break
compatibility with existing log parsing scripts.  Also,
ngx_http_log_error_handler() logs request line, which can contain unsafe and
miltiline data as well.  If we move the "while" part to the start of the line,
this unsafe request line will move with it.

> 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
>  

--
Roman Arutyunyan