getsentry / sentry

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

Handle Public-Key-Pinning (HPKP) violation reports #2165

Closed reedloden closed 6 years ago

reedloden commented 8 years ago

To expand upon the CSP reporting support added in #729 and #2154, would love to have HPKP reporting support as well. This would really make Sentry very useful for handling these reports coming from browsers.

https://developer.mozilla.org/en-US/docs/Web/Security/Public_Key_Pinning https://developers.google.com/web/updates/2015/09/HPKP-reporting-with-chrome-46 https://tools.ietf.org/html/rfc7469#section-3

reedloden commented 8 years ago

(cc @mattrobenolt since he just added the CSP support)

mattrobenolt commented 8 years ago

You are on top of our shit. :+1:

I'll look into this. We're technically just testing the CSP reporting ourselves internally before launching publicly to get a feel for these things.

I'll add this to my list and see if we can just do some generic "security reporting" solution.

reedloden commented 8 years ago

Cool, and thanks again for getting this implemented. So excited to start using this, even just for CSP reports.

You might can also get some ideas from @scotthelme's https://report-uri.io service (see https://scotthelme.co.uk/csp-and-hpkp-violation-reporting-with-report-uri-io/), though that's just one guy's take on this problem. Everywhere else I've seen this done has just written their own custom collector (such as https://github.com/skbkontur/csp_reporter) and parser, so would be good to have something more widely available that people could use rather than reinventing the wheel every time (especially if it comes with the power of Sentry's existing functionality).

mattrobenolt commented 8 years ago

Everywhere else I've seen this done has just written their own custom collector (such as https://github.com/skbkontur/csp_reporter) and parser

Yeah, so our needs (if you look at the code for our support) is probably much different than what others need. I'm not sure it's too valuable to have a generic parser for a CSP report since frankly, it's so simple.

The challenge we have is trying to group the same violation from different browsers to try and reduce the noise. Firefox likes to report the same thing a bit differently than Chrome, for example, so we try to do some normalizing of the values to make them group together as expected.

But ideally, having this inside Sentry is a good framework since we already do notifications and alerting around issues, so bolting on features like this just makes sense. :)

mattrobenolt commented 8 years ago

Hey @reedloden, do you have a way to actually test sending a report? I haven't seemed to be able to get Chrome or Firefox to actually send one to the report-uri.

I've added a Public-Key-Pins header with a bad pin-sha265 value (assuming this will cause it to trigger), with a report-uri, and I don't get anything. :(

ScottHelme commented 8 years ago

If there is no valid pin then the policy won't be cached or enforced. This stops you from easily DoSing yourself.

I use another subdomain to demonstrate hpkp violations. You can read more here, https://scotthel.me/pkpf

Please excuse my brevity, I'm on mobile.

reedloden commented 8 years ago

Firefox doesn't yet support report-uri for HPKP (https://bugzilla.mozilla.org/show_bug.cgi?id=1091176), but Chrome definitely does as of Chrome 46.

@ScottHelme's blog post helps with other issues.

reedloden commented 8 years ago

@mattrobenolt, any update on this and #2190? The CSP reports have been very useful (even with all the noise), so would be nice to have support for these other types of reports as well. :)

reedloden commented 8 years ago

@mattrobenolt, anything I can do to help here? Right now, I'm splitting my reporting between Sentry and between report-uri.io, and I would like to move it all to Sentry (so I can host it internally eventually).

Happy to provide any support you need.

dcramer commented 8 years ago

@reedloden tbh the feature isnt super high priority for us. It's not overly difficult to build, but its more of a "scratch your own itch" kind of thing. We're pretty much all internally prioritizing strong infrastructure and workflow improvements at the moment.

jesseendahl commented 6 years ago

FWIW I'd also love to see support for this in Sentry, for the same reason (don't like having to use multiple services for reporting).

dcramer commented 6 years ago

For easy ref, here's the specification for what the report looks like:

https://tools.ietf.org/html/rfc7469#section-3

reedloden commented 6 years ago

HPKP is getting deprecated by Chrome (and wouldn't be surprised if other browsers follow), so my recommendation would be to WONTFIX this for now.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/he9tr7p3rZ8 https://www.reddit.com/r/netsec/comments/796exs/public_key_pinning_being_removed_from_chrome/

However, plenty of other things that need additional support added (X-XSS-Protection, Expect-CT, Expect-Staple) or the generic Reporting API.

alex-hofsteede commented 6 years ago

Yep, had just implemented this in https://github.com/getsentry/sentry/pull/6417 when I saw it was getting deprecated :( But it will be a good starting point for adding other types of security reports

reedloden commented 6 years ago

Closing this out, as HPKP is deprecated.

ScottHelme commented 6 years ago

*HPKP isn't deprecated yet. Chrome have announced they intend to deprecate it. It's still supported in Firefox and a defined RFC.

reedloden commented 6 years ago

@ScottHelme Fair enough. Do you think it's worth supporting in Sentry at this time, though? That's basically what I was meaning...

ScottHelme commented 6 years ago

Only Sentry could answer that one but my guess would probably be no :)