Closed
Bug 1329885
Opened 7 years ago
Closed 7 years ago
LoadInfo constructors don't handle turning off the SEC_FORCE_INHERIT_PRINCIPAL correctly when SEC_SANDBOXED
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla53
| Tracking | Status | |
|---|---|---|
| firefox53 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 1 obsolete file)
|
2.44 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
They have the same bug as nsBaseChannel::Redirect used to. The good thing is that SEC_FORCE_INHERIT_PRINCIPAL is ignored altogether when GetLoadingSandboxed() is true. So in terms of observable behavior this is not biting us.
|
Assignee | |
Comment 1•7 years ago
|
||
Attachment #8825283 -
Flags: review?(ckerschb)
|
|
Assignee | |
Comment 2•7 years ago
|
||
> So in terms of observable behavior this is not biting us.
Well, except maybe for the secure context determination stuff....|
|
Assignee | |
Comment 3•7 years ago
|
||
And indeed, loading this file from a file:// URL: <!DOCTYPE html> <script>document.write(isSecureContext)</script> <iframe src="http://software.hixie.ch/utilities/cgi/test-tools/delayed-file?pause=0&mime=text%2Fhtml&text=%3Cscript%3Edocument.write(isSecureContext)%3C%2Fscript%3E"></iframe> <iframe sandbox="allow-scripts" src="http://software.hixie.ch/utilities/cgi/test-tools/delayed-file?pause=0&mime=text%2Fhtml&text=%3Cscript%3Edocument.write(isSecureContext)%3C%2Fscript%3E"></iframe> shows that this bug is screwing up the isSecureContext value for that second iframe: it comes out true when it should be false. Well, and the bogus way we set mForceInheritPrincipalDropped keeps screwing it up if we fix that... Luckily, we're not using SecureContext for anything yet. Also luckily, if the secure outer thing were https this would be blocked by mixed content blocking. That last makes it a bit hard to write a testcase for this.
|
|
Assignee | |
Comment 4•7 years ago
|
||
Attachment #8825289 -
Flags: review?(ckerschb)
|
|
Assignee | |
Updated•7 years ago
|
Attachment #8825283 -
Attachment is obsolete: true
Attachment #8825283 -
Flags: review?(ckerschb)
|
||
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [domsecurity-active]
|
|
||
Comment 5•7 years ago
|
||
Comment on attachment 8825289 [details] [diff] [review] Be more careful about removing our SEC_FORCE_INHERIT_PRINCIPAL bit in LoadInfo constructors Review of attachment 8825289 [details] [diff] [review]: ----------------------------------------------------------------- Rrrh - thanks a lot for fixing that. I should have looked at that when reviewing the fix within BaseChannel. (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #3) > Luckily, we're not using SecureContext for anything yet. Also luckily, if > the secure outer thing were https this would be blocked by mixed content > blocking. That last makes it a bit hard to write a testcase for this. Given that it's potentially only a problem within SecureContext, there is no reason to uplift the patch, right?
Attachment #8825289 -
Flags: review?(ckerschb) → review+
|
|
Assignee | |
Comment 6•7 years ago
|
||
Yes, I don't see a reason to uplift this.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0da540bfa9 Be more careful about removing our SEC_FORCE_INHERIT_PRINCIPAL bit in LoadInfo constructors. r=ckerschb
|
||
Comment 8•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/bf0da540bfa9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•