Insecure IP address detection

[tkolo]

In CurrentContext.cs:50 you assume that if there exists X-Forwarded-For header, it should be trusted over real user IP. This might lead to ability for end user to spoof their real IP address by sending their own header, as long as the application is not behind reverse proxy or the proxy is misconfigured and does not provide it's version of header.

[kspearrin]

Bitwarden is always behind a proxy of some sort. Whether it's Azure App Services or a self hosted nginx server. I would assume that these proxies don't allow the header to be spoofed so easily. Were you able to successfully exploit this?

[tkolo]

No, I found it only because I was browsing the code wondering how much effort would it take to port this to MySQL. My conclusion come only from staticaly analysing the code. For the record though, while I do agree with you that in most cases the existence of proxy is pretty much certain, you shouldn't rule out possibilities of users creating their custom self hosted environments. Nextcloud has this case solved elegantly by allowing user to define trusted proxy IPs in their config file. I think you should do the same.

[tkolo]

Out of curiosity I did some digging, and if my tests are right, the self-hosted configuration is broken. According to NginxConfigBuilder and results I've got after setting a sample instance, no part of bitwarden has X-Forwarded-For set by the proxy, hence they should all default to remote IP in case the header is absent (which will be the IP of proxy), or the client provided header in case it was spoofed. I also found some nginx proxy configuration in util/Nginx/proxy.conf, but it seems unused. The funny thing is it might also not be OK, depending what you want to achieve. In proxy.conf, the line in question says at follows:

proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;

according to nginx docs this will leave the original content of the header and append new remote IP address to it, as seen by nginx. The result will be two IPs separated by comma. So if client spoofs the X-Forwarded-For header with 1.2.3.4, and nginx then adds remote ip of 10.10.10.10 then the resulting value will be 1.2.3.4, 10.10.10.10. It seems what you really want to use is $remote_addr. Or ideally, nginx has something called real ip module which seems to be designed to deal with the problem at hand.

[tkolo]

Also, why does it do FirstOrDefault() there? I mean come on, if there are multiple headers for that then it's a clear sign something fishy is going on here.

[kspearrin]

I just tested this on both platforms and it appears that you can exploit it on both Azure and Nginx. Setting the X-Forwarded-For header in a request passes through to the instance. I'll need to do some more debugging to see where we're flawed here.

[kspearrin]

results I've got after setting a sample instance, no part of bitwarden has X-Forwarded-For set by the proxy, hence they should all default to remote IP in case the header is absent (which will be the IP of proxy), or the client provided header in case it was spoofed.

This was happening due to improper use of ForwardedHeadersOptions, here: https://github.com/bitwarden/core/blob/4d703541bd6bae8f86b16651c308eb149988dcf6/src/Core/Utilities/ServiceCollectionExtensions.cs#L290

I can't remember exactly why that was implemented in that way, but after reading some of the referenced issues it would seem that things have changed recently in newer versions of ASP.NET.

I just removed the use of UseForwardedHeaders algoether and X-Forwarded-For is now coming through as expected (see example below).

I also found some nginx proxy configuration in util/Nginx/proxy.conf, but it seems unused.

The Dockerfile bakes util/Nginx/proxy.conf into the image. If I remember correctly, it's at a default location the nginx uses for proxy configurations.

according to nginx docs this will leave the original content of the header and append new remote IP address to it, as seen by nginx

That appears to also be what Azure does (see below). So it would appear we are consistent with behavior.

---

Doing some debug testing on Azure shows the following headers coming in:

        [HttpGet("ip")]
        public IActionResult Ip()
        {
            string forwardHeaderValue = null;
            if(HttpContext.Request?.Headers?.TryGetValue("X-Forwarded-For", out StringValues forwardHeader) ?? false)
            {
                forwardHeaderValue = forwardHeader.FirstOrDefault()?.Trim();
            }

            return new JsonResult(new
            {
                XForwardedFor = forwardHeaderValue,
                RemoteIpAddress = HttpContext.Connection?.RemoteIpAddress?.ToString(),
                ReqHeaders = HttpContext.Request?.Headers
            });
        }

{
    "xForwardedFor": "192.168.1.1",
    "remoteIpAddress": "MYREALIP",
    "reqHeaders": {
        "Cache-Control": [
            "no-cache"
        ],
        "Connection": [
            "Keep-Alive"
        ],
        "Accept": [
            "*/*"
        ],
        "Accept-Encoding": [
            "gzip, deflate"
        ],
        "Cookie": [
            "ARRAffinity=a75f436fe1433e0a6050400ba97bd8e396794446f858ac4ca1d1981bfcf7c0d8"
        ],
        "Host": [
            "myiptesting.azurewebsites.net"
        ],
        "Max-Forwards": [
            "10"
        ],
        "User-Agent": [
            "PostmanRuntime/7.1.1"
        ],
        "X-Forwarded-For": [
            "192.168.1.1",
            "MYREALIP:SOMEPORT"
        ],
        "X-WAWS-Unencoded-URL": [
            "/api/values/ip"
        ],
        "X-Original-URL": [
            "/api/values/ip"
        ],
        "X-ARR-LOG-ID": [
            "4f2f0493-03c1-47a9-9774-dd045a4139a9"
        ],
        "DISGUISED-HOST": [
            "iptest.azurewebsites.net"
        ],
        "X-SITE-DEPLOYMENT-ID": [
            "iptest"
        ],
        "WAS-DEFAULT-HOSTNAME": [
            "myiptesting.azurewebsites.net"
        ],
        "MS-ASPNETCORE-TOKEN": [
            "75229ef3-b27c-471a-ae41-500ee3d7f540"
        ],
        "X-Original-For": [
            "127.0.0.1:49720"
        ],
        "X-Original-Proto": [
            "http"
        ]
    }
}

192.168.1.1 is my spoofed X-Forwarded-For that I am sending in. Looks like a bit of failed logic is in the use of FirstOrDefault() instead of the proper LastOrDefault().

More interestingly though is that RemoteIpAddress already returns the correct IP, so manually using X-Forwarded-For is not even necessary. Maybe this was something made better in recent versions of ASP.NET Core?

Anyways, I've committed a fix here for now, though I still need to test this on Nginx too for completeness.

[tkolo]

The Dockerfile bakes util/Nginx/proxy.conf into the image. If I remember correctly, it's at a default location the nginx uses for proxy configurations.

That is not enough. Nginx loads only nginx.conf, all the other files are implicitly loaded when included in that file. I haven't found any references in your docker's configuration that'd make ngnix include proxy.conf

That appears to also be what Azure does (see below). So it would appear we are consistent with behavior.

Actually, no. Judging by your output, what azure did was appending second X-Forwarded-For header to the HTTP request. In case of nginx you'd see something like this:

"X-Forwarded-For": [
      "192.168.1.1, MYREALIP:SOMEPORT"
]

192.168.1.1 is my spoofed X-Forwarded-For that I am sending in. Looks like a bit of failed logic is in the use of FirstOrDefault() instead of the proper LastOrDefault().

This is also not always true. One problem is it seems azure's behavior is not compatibile with nginx's, unless you change the latter to behave like azure, as I guess you can't really change azure to follow default nignx's behavior. The 2nd one is that two (or more) IPs in X-Forwarded-For headers might not necessarily mean that it's the first one that is correct and 2nd one is a spoof attempt. It might simply mean that application is by double reverse proxy, which might very well be true if somebody puts bitwarden's docker infrastructure between their own nginx for the sake of port re-usability. I got picky about double X-Forwarded-Host in my previous comment because I thought that what proxies would do is what nginx does: append new IP to existing header after comma. But your azure test clearly shows that is not always the case and doubled header might very well be a valid scenario.

[kspearrin]

Actually, no. Judging by your output, what azure did was appending second X-Forwarded-For header to the HTTP request. In case of nginx you'd see something like this:

That's what my Azure test showed though...?

    "X-Forwarded-For": [
        "192.168.1.1",
        "MYREALIP:SOMEPORT"
    ],

[tkolo]

You've got two string elements where each is a valid IP. I've got one string with two IPs separated by comma. So you still need to split it.

[kspearrin]

@tkolo I see, however, that doesn't really matter anymore since I've removed the manual parsing of the X-Forwarded-For header. ASP.NET is handling it now with HttpContext.Connection?.RemoteIpAddress as shown above. I confirmed that RemoteIpAddress also works correctly on the Nginx container as well.

We do still have another issue with Nginx though. I have confirmed that the proxy.conf is being loaded into the container. This is done through nginx.conf that is also copied during image compilation of the DockerFile: https://github.com/bitwarden/core/blob/master/util/Nginx/Dockerfile

I bashed into the container, made changes to proxy.conf, restarted, and can see them being picked up.

{
    "ReqHeader": {
        "Cache-Control": [
            "no-cache"
        ],
        "Connection": [
            "close"
        ],
        "Accept": [
            "*/*"
        ],
        "Accept-Encoding": [
            "gzip, deflate"
        ],
        "Host": [
            "bitwarden.website.com"
        ],
        "User-Agent": [
            "PostmanRuntime/7.1.1"
        ],
        "X-Real-IP": [
            "172.18.0.1"
        ],
        "X-Forwarded-For": [
            "192.168.1.1, 172.18.0.1"
        ],
        "X-Url-Scheme": [
            "https"
        ],
        "X-Forwarded-Proto": [
            "https"
        ]
    }
}

However, as you can see, we are still getting the docker host (172.18.0.1) as the real IP, which is not correct.

I am not sure what this fix is for this. I've found several threads with people suffering the same problems without a solution. https://www.google.com/search?q=docker+nginx+x-forwarded-for

All suggestions seem to indicate what I'm already doing and have confirmed is being applied.

[tkolo]

Oh, you do include proxy.conf indeed in here, I've missed it. My apologies. I'm not so certain about your fix though. By my understanding HttpContext.Connection.RemoteIpAddress should simply feed you remote end IP fed by transport (TCP) layer. And this looks like what happen in your example pasted above: you connected to your app directly with spoofed X-Forwarded-For header and saw that RemoteIp is correct, which was only because there was no proxy in between. Unfortunately official .NET docs don't say much about how this field behaves so who knows, maybe there is indeed some magic going on behind the scene. As for your last paste, is 192.168.1.1 your real IP there? if yes, then you're in the double proxy scenario, which I've described in my previous comment

[kspearrin]

No, 192.168.1.1 is a spoofed IP I passed in headers of the request. 172.18.0.1 is the docker host. The Docker host is returned by HttpContext.Connection.RemoteIpAddress.

I need to replicate this scenario in a small testable container so I can try various nginx configurations. Debugging this in the current solution has too many moving parts

[tkolo]

I found your problem: you get IP of docker host because your docker-compose config translates port redirect into MASQUARADE entry in iptables. And masquarading changes remote IP end by design. What you want there to happen is DNAT, which is also already correctly set up, it's just masquerade config taking precedence. Unfortuantely i'm far from being docker expert and can't tell you what exactly is causing this. But i'd dig in docker compose's documentation to track it down. If you want to reproduce a dirty fix locally to confirm that this is indeed the problem, then setup everything as usual with bitwarden's script, and after you ./bitwarden start, remove all docker's MASQUERADE entries from POSTROUTING chain in nat table. In my case when I set everything up in POSTROUTING table I get:

# iptables -t nat -L POSTROUTING   
Chain POSTROUTING (policy ACCEPT)
target     prot opt source               destination         
MASQUERADE  all  --  172.17.0.0/16        anywhere            
MASQUERADE  all  --  172.18.0.0/16        anywhere            
POSTROUTING_direct  all  --  anywhere             anywhere            
POSTROUTING_ZONES_SOURCE  all  --  anywhere             anywhere            
POSTROUTING_ZONES  all  --  anywhere             anywhere            
MASQUERADE  tcp  --  172.18.0.3           172.18.0.3           tcp dpt:https
MASQUERADE  tcp  --  172.18.0.3           172.18.0.3           tcp dpt:http

so to remove all 4 I need to:

# iptables -t nat -D POSTROUTING 7      
# iptables -t nat -D POSTROUTING 6
# iptables -t nat -D POSTROUTING 2
# iptables -t nat -D POSTROUTING 1

Note the reverse order of IDs, otherwise they shift and you have to count again. Then it should start giving you correct remote IP.

[bitwarden-bot]

Hi @tkolo, We're cleaning up our repositories in preparation for a major reorganization. Issues from last year will be marked as stale and closed after two weeks. If you still need help, comment to let us know and we'll look into it. Thanks!