Disallow access in publicly exposed services

[kermieisinthehouse]

Several in-the-wild unprotected stash instances have been found, disclosing their contents on the public internet.

This patch tries to prevent that from being possible. When a request is received from an IP that is not a private network and authentication is not enabled, it logs details, blows a tripwire, and shuts down the server to protect the data. Stash will not start again until a config option, security_tripwire_accessed_from_public_internet is changed.

Users of external auth can set dangerous_allow_public_without_auth to true to continue using their setups.

Also created for reference: https://github.com/stashapp/stash/wiki/Authentication-Required-When-Accessing-Stash-From-the-Internet

[kermieisinthehouse]

Fixed those notes. I also can't get the server to start with this patch. It looks like cfg.GetSecurityTripwire(...) is always true. Can't figure that one out. Also, the server doesn't seem to exit, simply hang and not answer requests. Which has me thinking, should we just not shut down the server, but instead 403 every request after the tripwire is blown?

[kermieisinthehouse]

I've stopped the server from shutting down, now it just closes the database and returns every request with a little snippet about what's happening. I figure this will stop support requests for "My server crashed for no reason and I can't be bothered to read the logs"

I still have the problem with it being triggered all of the time. I don't know how the config options work well enough to fix this. Also it seems that if the config file is missing, it triggers too.

[kermieisinthehouse]

I've fixed the last few bugs and tested it. It seems all good now. The protection is not applied to the first-time setup, but I think that's fine.

[kermieisinthehouse]

@gitgiggety moving to comments for visibility.

I agree. I've added a trusted proxies section, and am allowing proxy chains from the local network by default. They still don't allow access from endpoints on the public internet. This should prevent the cases you've mentioned, such as leaving a reverse proxy exposed accidentally (provided it send X-FORWARDED-FOR). It also prevents spoofing x-forwarded-for to bypass the protection.

@WithoutPants when this is merged, is it possible to make a backported 0.9.1 release with just this? I don't have exact numbers but a decent chunk, maybe most, of affected instances ran the latest stable, so it's hopeful to assume they'd take a point release too, protecting them.

[kermieisinthehouse]

Fixed an issue where access from 127.0.0.1 was blocked.

Here's my testcases for this PR, I used curl to simulate the proxy headers.

# access from local IP
2021/09/28 19:27:17 "GET http://192.168.1.119:9999/ HTTP/1.1" from 192.168.1.5:41988 - 200 3124B in 48.22µs
# access from CGNAT IP
2021/09/28 19:29:05 "GET http://100.72.140.103:9999/ HTTP/1.1" from 100.68.156.57:40400 - 200 3124B in 34.715µs
# access from loopback
2021/09/28 19:33:16 "GET http://127.0.0.1:9999/ HTTP/1.1" from 127.0.0.1:52602 - 200 3124B in 43.811µs
# access from "localhost"
2021/09/28 19:38:14 "GET http://localhost:9999/ HTTP/1.1" from 127.0.0.1:52604 - 200 3124B in 68.538µs
#curl --header "X-FORWARDED-FOR: 192.168.1.1" 192.168.1.119:9999
2021/09/28 19:35:30 "GET http://192.168.1.119:9999/ HTTP/1.1" from 192.168.1.119:33302 - 200 3124B in 44.153µs
#curl --header "X-FORWARDED-FOR: 192.168.1.1, 192.168.1.2" 192.168.1.119:9999
2021/09/28 19:36:57 "GET http://192.168.1.119:9999/ HTTP/1.1" from 192.168.1.119:33304 - 200 3124B in 37.39µs
#curl --header "X-FORWARDED-FOR: 8.8.8.8" 192.168.1.119:9999
Trips the wire!
#curl --header "X-FORWARDED-FOR: MALFORMED" 192.168.1.119:9999
Trips the wire!
#curl --header "X-FORWARDED-FOR: 8.8.8.8, 192.168.1.1" 192.168.1.119
Trips the wire!

[WithoutPants]

I've done a bit of refactoring on the new code:

• moved the authentication handler code into a separate file in api
• moved the external access checking logic into the session package
• changed the trustedProxies setting to accept a list of strings, rather than a comma-separated string
• changed the security_tripwire_accessed_from_public_internet to store the IP address that triggered the tripwire. This IP address is printed as part of the log message when the tripwire is activated
• added a check for the tripwire activation at startup. If the tripwire is active, then it logs the same message as when it was originally tripped

I have updated the wiki page to be consistent with the new behaviour.

[kermieisinthehouse]

New changes staticly reviewed reviewed but not tested, approved.

[bradydjohnson]

Where in the config.yml file should the security_tripwire_accessed_from_public_internet line go? Does anyone have an example config.yml file they can share?

I tried to access my stash remotely using Tailscale as recommended here, but it didn't work. Then I think I removed security_tripwire_accessed_from_public_internet and now I am unable to access my Stash even on my home private network.