1188538 - Don't load URIs if they don't have a security flag set

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Bobby Holley (:bholley)]

See bug 1186152 comment 12.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

This one has rendered INVALID given that we do not support GetProtocolFlags anymore, see:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/netwerk/base/nsIOService.cpp#904

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Ah, I guess I was too fast in closing this one - there is getFlagsForURI within nsIProtocolHandlerWithDynamicFlags - maybe that is the one to use.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Hey Bobby, based on the description and the linked bug I can't really tell whether this bug is still valid or not. Also because nsIProtocolHandler mentions that each handler must set one of 5 flags, but way more are listed:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/netwerk/base/nsIProtocolHandler.idl#170

If this one is still valid and you think it's worth fixing, we could easily add a check within the ContentSecurityManager to ensure that all URIs to be opened have valid security checks.

I personally like the general idea behind, because it would harden our infrastructure, but I don't know if it's still applicable.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)

Hey Bobby, based on the description and the linked bug I can't really tell whether this bug is still valid or not.

Seems like we still have the suboptimal behavior described: https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/caps/nsScriptSecurityManager.cpp#964

Also because nsIProtocolHandler mentions that each handler must set one of 5 flags, but way more are listed:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/netwerk/base/nsIProtocolHandler.idl#170

That's just because it's effectively an enum that's slotted into a bitfield with a bunch of other things before and after. Ideally we'd separate it into a separate enum field, but that might be too much work? If so, we can just hard assert that exactly one of those five bits are set.

If this one is still valid and you think it's worth fixing, we could easily add a check within the ContentSecurityManager to ensure that all URIs to be opened have valid security checks.

Not sure exactly where the right place to put the check is, but anything we do here should also fix up the mess in the SSM that I referenced above.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Ok, I think that bug very much aligns with the work we are doing to harden the security landscapce of Firefox (Meta Bug 1596359). I see what I can do.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Bobby, it seems all protocol handlers set one of the 5 security flags. Adding such an assertion in the ContentSecurityManager which is consulted within asyncOpen()/open() of every channel verifies that, see:
https://hg.mozilla.org/try/rev/474f2e10b1751dff6953df28f5381d6baf1ca158

My main question is however, what do we need to do within CheckLoadURIFLags:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/caps/nsScriptSecurityManager.cpp#964

Or put differently, what do you want me to do there?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)

Bobby, it seems all protocol handlers set one of the 5 security flags. Adding such an assertion in the ContentSecurityManager which is consulted within asyncOpen()/open() of every channel verifies that, see:
https://hg.mozilla.org/try/rev/474f2e10b1751dff6953df28f5381d6baf1ca158

Cool. As a cute bonus, you could mask off the five bits at once, and then check if the result is a power-of-two, though I don't object if you prefer the very-explicit-and-obvious approach.

My main question is however, what do we need to do within CheckLoadURIFLags:
https://searchfox.org/mozilla-central/rev/d69ec052bed8700af7a283e37b60b4af22734930/caps/nsScriptSecurityManager.cpp#964

Or put differently, what do you want me to do there?

(1) Delete all the existing handling below that line.
(2) Add a debug-only assertion that the chain either has URI_LOADABLE_BY_SUBSUMERS or URI_LOADABLE_BY_ANYONE.
(3) Add a comment indicating that the former case is handled by the caller, which is delegating to us as a helper.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1188538: Ensure every protocol handler sets a valid security flag. r=bholley,mixedpuppy

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/f4981f4405d978e04676ce53028c15451558d20e
https://hg.mozilla.org/mozilla-central/rev/f4981f4405d9