Closed Bug 1584543 Opened 2 months ago Closed 2 months ago

Cert errors in sandboxed iframes are still broken

Categories

(Firefox :: Security, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- wontfix
firefox70 + verified
firefox71 --- verified

People

(Reporter: johannh, Assigned: johannh, NeedInfo)

References

Details

(Keywords: regression)

Attachments

(2 files)

...because the sandbox gives them a null principal and the RPM check fails.

STR:

data:text/html,<iframe sandbox="allow-scripts" src="https://expired.badssl.com"></iframe>

And open the devtools console to see the RPM error.

Turns out that our WebIDL-exposed info doesn't work either, I think the proper way to fix this is to use documentURI for these checks instead of the principal. With the principal we would do URI-based comparisons anyway, there's no reason to use it as there's no possibility we would need the other information the principal contains.

Summary: RPM doesn't give correct access to about: pages in sandboxed iframes → Cert errors in sandboxed iframes are still broken

This is a necessary change that was done for Fluent access in bug 1573276. In almost all cases,
we want to rely on the principal for making security decisions, but the principal does not
store the original URI in cases where an about: page was sandboxed (it becomes a null principal URI),
and thus we need to use the documentURI here.

We should consider uplifting this

Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10c7400edbc1
Make checks for in-content functionality depend on documentURI instead of principal URI. r=ckerschb,Gijs
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Do you think this would be OK to uplift for beta 13 (of 14)?

Flags: needinfo?(jhofmann)

Oh, would this fix bug 1573276?

(In reply to Liz Henry (:lizzard) from comment #8)

Oh, would this fix bug 1573276?

Not on its own, we need both fixes for error pages to be fully functional when in sandboxed iframes.

Comment on attachment 9097308 [details]
Bug 1584543 - Make checks for in-content functionality depend on documentURI instead of principal URI. r=ckerschb!,Gijs!

Beta/Release Uplift Approval Request

  • User impact if declined: Certificate error pages in iframes are lacking some functionality (only the basic error is displayed, but the user can't really interact)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: :
  • Open the URL from comment 0
  • Click on "Advanced" in the iframe
  • Make sure that all advanced information is displayed and that you can click on "View Certificate" and other elements
  • List of other uplifts needed: Bug 1573276
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The fix is relatively straightforward, but it modifies code that checks security boundaries, so it's always a good idea to be a little cautious with that. On the other hand this has a lot of automated tests.
  • String changes made/needed: None
Flags: needinfo?(jhofmann)
Attachment #9097308 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

I tested this issue on Windows 10, Mac OS X 10.14 and Ubuntu 18.04 with FF Nightly 71.0a1(2019-10-06) and I can confirm the fix.

Comment on attachment 9097308 [details]
Bug 1584543 - Make checks for in-content functionality depend on documentURI instead of principal URI. r=ckerschb!,Gijs!

OK for uplift for beta 13 along with the patch in bug 1573276.

Attachment #9097308 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

This conflicts with bug 1561443 which is only on central. Please provide a patch for beta.

Flags: needinfo?(jhofmann)

This is the patch for beta.

Thanks!

Flags: needinfo?(jhofmann) → needinfo?(aryx.bugmail)
Attachment #9099840 - Flags: approval-mozilla-beta+

I tested this issue on Windows 10, Mac OS X 10.14 and Ubuntu 18.04 with FF Beta 70.0b14 and I can confirm the fix.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.