getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
38.9k stars 4.18k forks source link

possibility to avoid SSRF vulnerability #39102

Open gromnsk opened 2 years ago

gromnsk commented 2 years ago

Problem Statement

right now, in case of JS exception, there could be exception.stacktrace.frame.filename field in JSON payload. This field is vulnerable for SSRF. Anyone can send exception and change filename on some external or internal URI. It could be breach to internal network.

Solution Brainstorm

To avoid SSRF vulnerability and to keep functionality working I suggest to add whitelist of domains from which Sentry can scrape files

getsentry-release commented 2 years ago

Routing to @getsentry/ingest for triage. ⏲️

untitaker commented 2 years ago

This is a feature. We're fetching from arbitrary domains because our customer's applications run on arbitrary domains. Changing this behavior would require product concessions like asking for a domain name upfront, but even that change would not prevent the SSRF.

A GET request per se isn't supposed to have side-effects so in a well-behaving application there's no practical implications of this SSRF.

gromnsk commented 2 years ago

@untitaker according on hackerone(https://hackerone.com/reports/374737) there was a SSRF vulnerability and it's still exists. My fix doesn't broke anything in current configurations and just gave opportunity to those who want to avoid this problem - avoid this problem. If there is something wrong, just let me know and I'll do it in another way, but the problem still exists and it's not fixed, I can use it and anyone else can. For now I don't see how I can avoid this problem, if you have some information it would be great if you share it

untitaker commented 2 years ago

@gromnsk I see. Our prod config of SENTRY_DISALLOWED_IPS is:

# This configuration is duplicated in Symbolicator
SENTRY_DISALLOWED_IPS = (
    # We would like to disable ipv6 because we don't know which addresses are
    # local. Also Google has no ipv6 support anyway.
    #
    # However, adding a ipv6 wildcard here would break all hosts that just
    # offer ipv6. Instead we just block ipv6 loopback and rely on Google's lack
    # of ipv6 support for the rest.
    "::1",
    # http://en.wikipedia.org/wiki/Reserved_IP_addresses
    "0.0.0.0/8",
    "10.0.0.0/8",
    "100.64.0.0/10",
    "127.0.0.0/8",
    "169.254.0.0/16",
    "172.16.0.0/12",
    "192.0.0.0/29",
    "192.0.2.0/24",
    "192.88.99.0/24",
    "192.168.0.0/16",
    "198.18.0.0/15",
    "198.51.100.0/24",
    "224.0.0.0/4",
    "240.0.0.0/4",
    "255.255.255.255/32",
)

We should adapt a similar value for the default configuration.

Unfortunately we cannot rely on ipv6 being blocked in custom Sentry installations, so I am not sure what sane defaults would be that don't break the feature entirely.

@mdtro wdyt?

untitaker commented 2 years ago

also cc @mitsuhiko since he's most likely been around when the original implementation has been written.

github-actions[bot] commented 1 year ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

mdtro commented 1 year ago

Hi @gromnsk,

I'll take a look at this blind SSRF and the related PR. 😄

In the meantime, there are some options to mitigate this.

1) Use the SENTRY_DISALLOWED_IPS setting. 2) You can configure your self-hosted instance to use a proxy and it's domain filtering functionality.

github-actions[bot] commented 1 year ago

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀