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

[Mathew Heard]

I really like the making safe of the error log as opposed to truncation.
The more information logged in cases like this the better.

Alternatively what about something that indicates further data was
truncated?

On Wed, 28 Sep 2022, 21:07 Dipl. Ing. Sergey Brester via nginx-devel, <
nginx-devel at nginx.org> 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. 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
>
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20220928/bf8ad528/attachment.htm>