Closed Bug 1573276 Opened 3 months ago Closed 2 months ago

Certificate and network errors in sandboxed iframes completely broken (show XML Parsing Error)

Categories

(Firefox :: Security, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 blocking fixed
firefox71 --- fixed

People

(Reporter: johannh, Assigned: acat)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]:
Broken cert error pages in iframes

STR:

I'm pretty clueless about this one, the mentioned entity seems to exist and we have tests that specifically test cert errors in iframes. No idea.

mozregression points to bug 467035 and it seems to fit. Alex, can you please take a look at this?

Thanks!

Flags: needinfo?(acat)

Aha, I figured out why this wasn't caught by tests, my test site has sandbox="allow-scripts" on that iframe, which seems to cause the issue.

So,

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

seems to work, while

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

is broken.

We should have a regression test for this.

I hope that helps!

Flags: in-testsuite?
Summary: Cert errors in iframes show XML Parsing Error → Cert errors in sandboxed iframes show XML Parsing Error

This also affects the (non-cert) network error pages. We can't ship this.

Severity: normal → critical
Summary: Cert errors in sandboxed iframes show XML Parsing Error → Certificate and network errors in sandboxed iframes completely broken (show XML Parsing Error)

Sandboxed iframes have null principals and therefore the check in https://searchfox.org/mozilla-central/rev/c7e8bc49/dom/security/nsContentSecurityManager.cpp#641 is not passing. I'm not sure what is the best way to fix this, would it be possible that about:netErrors load with non-null principals even in sandboxed iframes? I wonder what's the difference with Fluent, since it uses the same nsContentUtils::PrincipalAllowsL10n logic and therefore I would expect it to not work either.

FWIW, I tested by changing that code to allow all DTDs loads and the iframe netError loads, but it looks slightly broken (Learn more link does not work, Go back buttons do not work, Advanced... section does not have any text, etc.). Not sure if that's known/expected.

Flags: needinfo?(acat)

Christoph,
The Tor Browser team wants to fix this regression because of locale leakage. Do you who would be the best person to resolve or consult with?

Flags: needinfo?(ckerschb)

Seems to me the simplest solution would be to change the checks at https://searchfox.org/mozilla-central/rev/c7e8bc49/dom/security/nsContentSecurityManager.cpp#641 to always allow document URIs of about:neterror/certerror/blocked to work.

(In reply to Alex Catarineu from comment #3)

FWIW, I tested by changing that code to allow all DTDs loads and the iframe netError loads, but it looks slightly broken (Learn more link does not work, Go back buttons do not work, Advanced... section does not have any text, etc.). Not sure if that's known/expected.

Some of this makes sense because of the iframe sandboxing. We could potentially work around that but it's a separate bug. But even just showing the full error message would be a step forward.

(In reply to :Gijs (he/him) from comment #6)

(In reply to Alex Catarineu from comment #3)

FWIW, I tested by changing that code to allow all DTDs loads and the iframe netError loads, but it looks slightly broken (Learn more link does not work, Go back buttons do not work, Advanced... section does not have any text, etc.). Not sure if that's known/expected.

Some of this makes sense because of the iframe sandboxing. We could potentially work around that but it's a separate bug. But even just showing the full error message would be a step forward.

I would presume that this is because of Fluent not working, but maybe I'm wrong.

(In reply to Alex Catarineu from comment #3)

Sandboxed iframes have null principals and therefore the check in https://searchfox.org/mozilla-central/rev/c7e8bc49/dom/security/nsContentSecurityManager.cpp#641 is not passing. I'm not sure what is the best way to fix this, would it be possible that about:netErrors load with non-null principals even in sandboxed iframes?

We shouldn't start messing with Principals, but it seems the triggeringPrincipal (in that case the NullPrincipal) provides the wrong information here. It's only a problem for iframes, right? And, the iframe load is under our control, right? I would imagine so, right?

So what we could do is the following:

  • Within DoCheckLoadURIChecks, and in particular within aLoadInfo->InternalContentPolicyType() == nsIContentPolicy::TYPE_INTERNAL_DTD
  • Query the loadingDocument from the loadinfo and check if the URI of that document is about:certerrer, etc.
  • First check if uri->SchemeIs("about") and within that if check check if the path starts with certerror. E.g. use StringBeginsWith(path, neterror) to avoid any subtle bugs.

It's a little hacky but the best I can come up with; we certainly don't want to change principals.

Flags: needinfo?(ckerschb)

Johann, looks like we have a patch, is this something you can either check in or help Alex get into inbound?

Flags: needinfo?(jhofmann)

Just reviewed :)

Noting that

data:text/html,<iframe sandbox="allow-scripts" src="https://www.itisatrap.org/firefox/its-a-trap.html"></iframe>

Is also broken because of the Fluent issue, not this bug. We need a follow-up once we "resolve" this, so if we want to uplift it would probably be both patches. I'll figure out if we need to file bugs for old versions tomorrow (I believe every version with fluent in-content pages is affected)...

Assignee: nobody → acat
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
Flags: needinfo?(acat)
Attachment #9087665 - Attachment description: Bug 1573276 - force allow DTD loads for error pages → Bug 1573276 - Always allow localization in error pages

Updated revision.

Flags: needinfo?(acat)

Alex, is this ready to land?

Flags: needinfo?(acat)

There is a pending blocking review from peterv.

Flags: needinfo?(acat) → needinfo?(peterv)
Flags: needinfo?(peterv)

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/344ae0abff73
Always allow localization in error pages r=johannh,peterv

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Duplicate of this bug: 1577134
See Also: → 1584543

Can you request uplift to beta? Thanks!

Flags: needinfo?(acat)
Flags: in-testsuite? → in-testsuite+

Comment on attachment 9087665 [details]
Bug 1573276 - Always allow localization in error pages

Need to uplift this as well for bug 1584543.

Attachment #9087665 - Flags: approval-mozilla-beta+

I assume beta uplift request is not needed anymore.

Flags: needinfo?(acat)
You need to log in before you can comment on or make changes to this bug.