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)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

Be more careful about removing our SEC_FORCE_INHERIT_PRINCIPAL bit in LoadInfo constructors
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.
> So in terms of observable behavior this is not biting us.

Well, except maybe for the secure context determination stuff....
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.
Attachment #8825283 - Attachment is obsolete: true
Attachment #8825283 - Flags: review?(ckerschb)
Priority: -- → P1
Whiteboard: [domsecurity-active]
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+
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
https://hg.mozilla.org/mozilla-central/rev/bf0da540bfa9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.