bugsnagRequestFromWaiRequest & co leak sensitive data

[MaxGabriel]

I was looking at one of our Bugsnag errors today and noticed it included all the headers of the request. This included some sensitive ones, like the session cookie (!). Other sensitive data you wouldn't want to send might include a CSRF token.

Thoughts on preventing this? Should a user be asked to provide a filtering function/whitelist/blacklist to prevent this from happening? Ideally it could be somewhat secure by default, automatically filtering out potentially sensitive data.

Rough proposal: For the functions that update from a WAI request, create a new function that takes a function argument that is used to filter the headers. For the existing function names, use a default filtering function that removes cookie and any key that contains "XSRF" "CSRF" or "Session".

http://hackage.haskell.org/package/bugsnag-haskell-0.0.1.2/docs/src/Network.Bugsnag.Request.html#bugsnagRequestFromWaiRequest

[MaxGabriel]

Also, if you agree this is a potentially dangerous security vulnerability, maybe deprecating the old versions is called for after fixing this

[pbrisbin]

Hmm. This is a tough one.

I think, internally to bugsnagRequestFromWaiRequest, we should add redactSensitiveHeader because that's really easy to do, fixes the most egregious case, and is very unlikely to be un-desired.

I think we should make it very clear in the docs what does and does not get redacted.

I don't think we need to make it explicitly configurable because updateEventFromWaiRequest is the top-level function that is used directly by the end-user and it would be very easy for them to apply their own Request -> Request sanitization before calling it -- I see no need for it to accept its own function argument to do that.

And I don't think we should redact more things internally (like Cookie) specifically because we're not providing a direct method for undoing that, so we can't do too much.

I could probably be convinced to expose some handy Request -> Request functions to perform stricter sanitization to make it easier for end-users to put the pieces together, but at present, I'm not sure it's necessary.

It also wouldn't be a terrible idea to open an upstream Issue in http-client to see if they think redactSensitiveHeader should redact Cookie/Set-Cookie too.

Assuming that's all we do, I still think making a fix to redact Authorization is worth of treating it with some security-related ceremony. So if there's a process for deprecating a package on the basis of a security issue, I'm happy to follow it.

[MaxGabriel]

@pbrisbin To clarify, it doesn't look like http-client exports redactSensitiveHeader—you're saying bugsnag-haskell should make its own version of that function and use that?

[pbrisbin]

Oh boo. I didn't realize is wasn't exported -- so I suppose I would re-implement it.

[MaxGabriel]

So my thinking behind Cookie is that it very likely contains extremely sensitive information. For any service using sessions, you can just copy someone's cookie to take actions on their behalf. So it effectively gives the keys to the kingdom to anyone at Bugsnag with the ability to look at our exceptions.

[pbrisbin]

That's reasonable :+1:. And to confirm, there's no need to look for Set-Cookie because that's in Responses only, not Requests, right?

[MaxGabriel]

Yep that sounds right to me!

Re:

it would be very easy for them to apply their own Request -> Request sanitization before calling it

I don't think this is quite true. The WAI Request object is read-only, so you couldn't provide your own Request. You could however, redact information from the BugsnagRequest after using updateEventFromWaiRequest, which seems fine. I mostly say this for posterity/since it sounds like that information will go into the docs.

Related to all this, would you like me to implement this stuff?

[pbrisbin]

I don't think this is quite true. The WAI Request object is read-only

Hmm. That certainly complicates things.

You could however, redact information from the BugsnagRequest after using updateEventFromWaiRequest.

That's a great idea! In fact, I think you've literally just described a BeforeNotify.

What do you think about having:

redactRequestHeaders :: [HeaderName] -> BeforeNotify
redactRequestHeaders = undefined

Then we can make redactRequestHeaders ["Authorization", "Cookie"] the default bsBeforeNotify?

Related to all this, would you like me to implement this stuff?

You're definitely welcome to. Though as you can see my preferred design is pretty in flux at the moment. If you don't, I'll probably get to it tomorrow or Monday.

[MaxGabriel]

I guess you go ahead and implement it—I just wrote out an implementation if you want to copy paste from it, but probably you'll come up with something cleaner

module Mercury.Network.Bugsnag.Util (redactBugsnagEvent) where

import ClassyPrelude
import Network.Bugsnag
import Yesod.Core.Handler (defaultCsrfHeaderName)
import Network.HTTP.Types.Header (Header, HeaderName)

redactBugsnagEvent :: BugsnagEvent -> BugsnagEvent
redactBugsnagEvent event = event {
    beRequest = redactBugsnagRequest <$> beRequest event
  }

redactBugsnagRequest :: BugsnagRequest -> BugsnagRequest
redactBugsnagRequest br = br { brHeaders = map redactSensitiveHeader <$> (brHeaders br) }

redactSensitiveHeader :: Header -> Header
redactSensitiveHeader (key, _) | key `elem` sensitiveHeaderNames = (key, "<REDACTED>")
redactSensitiveHeader h = h

-- Note: Header names are case-insensitive per the HTTP spec, so capitalization doesn't matter here
sensitiveHeaderNames :: [HeaderName]
sensitiveHeaderNames = 
  [ "Authorization" -- HTTP Basic Auth
  , "Cookie" -- Cookies contain session information which is extremely sensitive
  , defaultCsrfHeaderName -- Can remove this at your discretion
  ]

[pbrisbin]

This is fixed in v0.0.1.3. Since we're still pre-release (v0.0.*), I'm now thinking we don't need to go crazy with the Security ceremony and package deprecation. But I'm glad you caught this before 1.0!

[pbrisbin]

Hey @MaxGabriel sorry to ping you on this old Issue, but I was just thinking: isn't the _SESSION value in the Cookie useless without the private key? In the case of the default scaffold, that means an attacker would need to have the client_session_key.aes file.

I'm glad we added this behavior (and Authorization or CSRF are likely to be usable by themselves if compromised), but I'm not sure there was any real security issue with the Cookie specifically. RIght?

[MaxGabriel]

This is my thinking: The private key would be needed to forge arbitrary sessions, but if you just had a single person's exact session you could submit requests on their behalf, which is still a serious security vulnerability.

[pbrisbin]

if you just had a single person's exact session you could submit requests on their behalf, which is still a serious security vulnerability.

Ah yes. Fair enough.

[MaxGabriel]

@pbrisbin I noticed that we're still sending sensitive headers to bugsnag in our application. It looks like bsBeforeNotify settings runs first, then the function passed to notifyWith is called:

https://github.com/pbrisbin/bugsnag-haskell/blob/f5889c4704aa0c329a5452227f02693884ddcbcf/src/Network/Bugsnag/Notify.hs#L32-L33

I tested this with some trace statements and confirmed my notifyBugsnagWith function is reading writes from the bsBeforeNotify.

Is this a bug and the order just needs to be flipped?

[pbrisbin]

I'll admit I don't totally understand your message, but I think I see what you're saying: the Settings before-notify is where we redact headers, but the function given directly to beforeWith is when the WAI Request is copied into the Event Request -- so the order they currently run in leads to no redacting. Eek.

I'm not sure what's best to do because the order is intentional: I want the most local override (the direct argument) to have the highest precedence. I think it'd be confusing to perform some direct override but have some of our implicit handlers undo your changes.

I guess it doesn't make sense to redact headers as part of the global default settings, we should redact as part of the request-copying. Let me play with that a bit and let you know.