ClientIp enricher should have a way to not read X-Forwarded-For

[pergardebrink]

Instead of directly reading the X-Forwarded-Header by default, it's much better to use the ForwardedHeaders middleware (https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-7.0) and then let that middleware set the HttpContext.Connection.RemoteIpAddress so we only log that information instead.

If, for some reason someone don't want to use that, then the ClientIp enricher could read this by header. Since there's a danger of IP spoofing (which this enricher does not have any protection against), it's better to let the ForwardedHeader middleware and configuration take care of this (the KnownNetworks and KnownProxies configuration).

I can implement a suggestion for this (by making it possible to set a configuration to disable reading headers or similar), but I'd personally prefer if the default would be changed to not read X-Forwarded-For (which would be a breaking change).
Maybe something for a 3.0 release?

[mo-esmp]

Thank you for the suggestion and any contribution is very welcome. We can make it configurable and leave it to the client to make a choice: either read the client IP from the ForwardedHeaders middleware or directly from the request header.

[vbakke]

I second this. This is not an enhancement, but a CVE.
(EDIT: This is under no circumstances an 'enchantment', as previously stated. 🧙‍♂️)

Anyone using Serilog.Enrichers.WithClientIp() will unknowingly trust blindly any x-forwarded-for header coming their way.

This is very bad as a default value. Logfiles will render useless when it comes to actual remote IP. Only if the service is behind a trusted reverse proxy that removes and sets the x-forwarded-for should this header be trusted.

I agree with problems with backwards compatibility. I suggest that we first issue a warning about trusting x-forwarded-for in the documentation, and a compiler warning that the default will change in near future.

As a reference: Have a look at a similar bug at: https://nvd.nist.gov/vuln/detail/CVE-2023-22474

[mo-esmp]

@vbakke Thanks for reminding me of this; soon I will address it.

[vbakke]

Thank you, @mo-esmp! Of all vulnerabilities out there, this is not the most important one.

But in context of serilog-enrichers-clientinfo it is actual quite crucial. If an application wants to log the client IP address, I assume there is a reason for it. At the moment ClientInfo logs the correct IP address, unless someone is lying. And to be honest, I'd rather log the truth about the lairs, than about everyone else. ;)

I think the NodeJS Express module has got this right. (https://expressjs.com/en/guide/behind-proxies.html).
Same with Caddy Server (https://caddyserver.com/docs/caddyfile/directives/reverse_proxy#defaults).
I'd recommend following the same principles for ClientInfo.

That is: Don't trust X-Forwarded-For as default. Use the sockets IP address. (Yes, that can also be spoofed, but still.)
If behind a proxy, explicitly specify either the IP addresses of the reverse proxies that you can trust, or the number of reverse proxies that you can trust.

Please not that there are 2 - two - vulnerabilities is ClientInfo.

1. the default trust of X-Forwarded-For (if present)
2. picking the left-most IP address in the header as the client address in the XFF header

Reverse proxies

Unfortunately many reverse proxies are configured incorrectly by default on this topic. I discovered that Azure Gateway slips the header straight through unless you tell Azure to drop it. Apparently nginx also passes X-Forwarded-For straight through if you forget to include the RealIp module. (Caddy Server however blocks incoming values unless it is told to trust other proxies upstream.)

I sent the following header to a Serilog application, behind an Azure Gateway:
X-Forwarded-For: 10.10.10.10, 11.12.13.14

My Serilog application received:
X-Forwarded-For: 10.10.10.10, 11.12.13.14, {my_actual_ip}

And Serilog.Enrichers.ClientInfo logged my IP address as 10.10.10.10.

Example

Let's say my IP address is 88.88.88.88.
And let's say the Serilog application is behind a load balancer (10.20.20.20), which is behind the web application firewall (10.10.10.10).

The application would normally receive legit headers like:
X-Forwarded-For: 88.88.88.88, 10.10.10.10, 10.20.20.20

If my client sends a bogus header:
X-Forwarded-For: 11.12.13.14, and the reverse proxies were misconfigured, the application would receive:
X-Forwarded-For: 11.12.13.14, 88.88.88.88, 10.10.10.10, 10.20.20.20.

If Serilog was configured to trust '10.10.10.10, 10.20.20.20', it should log '88.88.88.88'.
If Serilog was configured to trust 2 reverse proxies, it should also log '88.88.88.88'.

Please let me know if there is anything I can to do contribute, or test.
Unless you'd like to do it, I can file a CVE when this has been fixed and published.

[vbakke]

Thank you, @mo-esmp. I can confirm that Serilog ClientInfo enricher no longer logs fake (or real) http header X-Forwarded-For.

I like the solution, relying on the Microsoft framework is a better idea. However, Microsoft's ForwardedHeadersOptions is a bit tricky for newcomers.

Since this will be a breaking change everyone behind a reverse proxy, load balancer or WAF, I propose the following text in the README:

ClientIp
Note! ClientIp enricher reads client IP from HttpContext.Connection.RemoteIpAddress. Since version 2.1, for security reasons, it no longer reads the x-forwarded-for header. (See Reverse proxies below.)

To include ClientIp in your logs, add the following to you logger configuration:

[Keep the existing two examples for enabling ClientIp]

Reverse proxies
If your application is behind a reverse proxy, load balancer, WAF, etc you will need to configure ForwardedHeadersOptions for the HttpContext.Connection.RemoteIpAddress to contain the correct value.

Make sure you read Scenarios and use cases and select the scenario that applies to your situation.

If you are behind a regular reverse proxy, the following might be what you need:

builder.Services.Configure<ForwardedHeadersOptions>(options =>
{
    string? knownProxy = builder.Configuration["Proxy:ReverseProxyIp"];
    if (!String.IsNullOrEmpty(knownProxy))
    {
        options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto;
        options.KnownProxies.Add(IPAddress.Parse(knownProxy));
    }
});

and:

app.UseForwardedHeaders();

If you know what you are doing, you can still log the x-forwarded-for header by using the RequestHeader enricher.

CorrelationId
....

[mo-esmp]

@vbakke Thank you for your suggestion regarding updates. I will modify the README file, but I am uncertain about including the pseudocode. It might be preferable to allow users to decide how they want to configure their forward header options.

[vbakke]

I agree that including guidelines for using another package (ForwardedHeadersOptions) is risky business. And normally I would not have included that.

However, we have just broken a feature that "worked out of the box". (Well, seemingly at least.) Many people will soon experience that Serilog stops logging the correct client IP. Nothing will warn them compile-time. So, they will only discover this after investigating their logs, realising that all log entries will contain the IP address of their reverse proxy.

I think we need to help them understand why we did it, and try to guide them to solve their problem that suddenly appeared. (The client IP will be a fairly major reason to use the ClientInfo enricher.)

I find Microsoft's Scenarios and use cases is badly written. It does not in a clear way cover the most likely scenario: An application behind a single reverse proxy.

The KnownProxies is mentioned higher up (under Forwarded Headers Middleware options). But this example includes a custom header name, specifies ForwardLimit = 2 but only applies one KnownProxies. And the address they add is a rare loopback address: 127.0.10.1.

That is why I added the example code. To help people in the right direction, since the Microsoft documentation for this class is so convoluted. And reverse proxies is a topic I find many programmers lack basic knowledge of, and therefore are more likely to make a mistake in their setup.

PS I have not had the time to register a CVE (never done it before). But I think I have found what CVA ("CVE authority") to use for Serilog. I'll keep you posted.

[vbakke]

@mo-esmp: CVE-2024-44930 has now been created.