Closed Bug 1328860 Opened 3 years ago Closed 3 years ago

Install pref to have data: URIs not inherit the security context

Categories

(Core :: DOM: Security, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Blocks: 1324406
To provide motivation for sites to switch to HTTPS we're pushing to have all new features require a secure context, whether the features are security related or not. If we were to prevent data: URI documents inheriting the security context of the opener we would hobble data: URI pages in ways we can't foresee yet.

What's the idea behind making this a pref BTW? So we have it in the event that someone were to figure out some sort of exploit that would be prevented by flipping the pref?
(In reply to Jonathan Watt [:jwatt] from comment #1)
> What's the idea behind making this a pref BTW? So we have it in the event
> that someone were to figure out some sort of exploit that would be prevented
> by flipping the pref?

This Bug would be the first step towards having data: URIs not inherit the security context from the including document (see master bug [1]). We would have this pref so we can step by step convert all the tests within our testsuite to obey the standard. E.g. rewrite a test to use parent.postMessage instead of parent.foo. Once we have all tests converted and we feel confident that it's time to flip the pref, then we would do that by default and sooner or later remove the pref completely.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1324406#c0
Hey Smaug, as discussed over IRC and noted within our game plan [1] let's start with adding a pref and check that pref within nsDataHandler. This will enable freddyb to start converting tests. Agreed?

[1] https://public.etherpad-mozilla.org/p/datauri
Attachment #8830440 - Flags: review?(bugs)
Comment on attachment 8830440 [details] [diff] [review]
bug_1328860_install_pref_for_data_uris.patch

ed= false;
Missing space before =

But I think it would be better to move the AddBoolVarCache usage outside this code. We probably want to check it elsewhere too, and hopefully we can make protocol handlers to support any thread.
Perhaps move the pref handling to nsIOService::Init() ?
Attachment #8830440 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #4)
> But I think it would be better to move the AddBoolVarCache usage outside
> this code. We probably want to check it elsewhere too, and hopefully we can
> make protocol handlers to support any thread.
> Perhaps move the pref handling to nsIOService::Init() ?

I agree that the cached pref should be more publicly accessible but I am not entirely sure where. Semantically I would think that the cached pref should live somewhere around nsDataHandler. So I moved the code within the patch around between nsDataHandler and the ioService and I don't know what's best. If it lives on the ioService we have to export 'nsIOService.h' so it's available for example within nsScriptSecurityManager. I don't know if that is a big deal or not, I would imagine not. I think we agree that we want the cached variable wrapped inside a method, right? I think we don't want to grant access to the pure static cached variable like nsIOService::sDataURIInheritSecurityContext, or would you prefer that? Let me know what you prefer, I am fine either way and I wouldn't mind shuffling the code around one more time. Thanks!
Attachment #8830440 - Attachment is obsolete: true
Flags: needinfo?(bugs)
I'm mainly thinking about the threadsafety of protocol handlers. If we want to eventually move to threadsafe protocol handlers, better to not add more mainthread only stuff.
Having some getter method in nsIOService to access static varibles feels reasonable to me.
The pref change handling probably wouldn't be truly threadsafe, but I think that is edge case, and we mostly need to support having some pref value during startup and read it in the main thread

(not sure why you'd need to export nsIOService.h. caps/ already does have some LOCAL_INCLUDES in moz.build)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #6)
> I'm mainly thinking about the threadsafety of protocol handlers. If we want
> to eventually move to threadsafe protocol handlers, better to not add more
> mainthread only stuff.
> Having some getter method in nsIOService to access static varibles feels
> reasonable to me.
> The pref change handling probably wouldn't be truly threadsafe, but I think
> that is edge case, and we mostly need to support having some pref value
> during startup and read it in the main thread

I agree and I take all of your comments here as that you are fine with the patch, right?
Attachment #8830639 - Attachment is obsolete: true
Attachment #8830662 - Flags: review?(bugs)
Comment on attachment 8830662 [details] [diff] [review]
bug_1328860_install_pref_for_data_uris.patch

>+    bool IsInheritSecurityContextForDataURIEnabled();
>+
Make this static

>+    // Until Bug 1324406 and all it's dependencies are fixed
>+    // data: URIs inherit the security context.
>+    if (gIOService->IsInheritSecurityContextForDataURIEnabled()) {
And then nsIOService::IsInheritSecurityContextForDataURIEnabled()


That way there wouldn't be any theoretical null-safety issues during shutdown.
Attachment #8830662 - Flags: review?(bugs) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c11fe7c58837
Install pref to have data: URIs not inherit the security context. r=smaug
https://hg.mozilla.org/mozilla-central/rev/c11fe7c58837
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.