Welcome! Log In Create A New Profile

Advanced

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

Roman Arutyunyan
September 28, 2022 11:50AM
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
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
Subject Author Views Posted

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

Dipl. Ing. Sergey Brester via nginx-devel 341 September 26, 2022 05:18PM

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

Maxim Dounin 45 September 27, 2022 05:08PM

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

Roman Arutyunyan 56 September 28, 2022 06:04AM

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

Dipl. Ing. Sergey Brester via nginx-devel 50 September 28, 2022 07:08AM

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

splitice 77 September 28, 2022 09:34AM

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

Roman Arutyunyan 81 September 28, 2022 11:50AM

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

Maxim Dounin 70 October 09, 2022 05:56PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 74
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready