Closed Bug 1688815 Opened 3 years ago Closed 3 years ago

Update siteOrigin handling of nsIPrincipal for view-source URIs

Categories

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

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Bug 1688815: Update GetSiteOriginNoSuffix() to handle view-source correctly.
48 bytes, text/x-phabricator-request
Details | Review

As surfaced and discussed within Bug 1687104, we have to update the view-source handling of siteOrigin within the nsIPrincipal.

Assignee: nobody → ckerschb
Blocks: 1671166
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attachment #9199187 - Attachment description: Bug 1688815: Update siteOrigin, siteOriginNoSuffix to handle view-source correctly. → Bug 1688815: Update GetSiteOriginNoSuffix() to handle view-source correctly.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/028bc12edb76
Update GetSiteOriginNoSuffix() to handle view-source correctly. r=nika

(In reply to Cosmin Sabou [:CosminS] from comment #3)

Backed out for causing ContentPrincipal related xpcshell crashes.

Investigating...

Flags: needinfo?(ckerschb)

It seems the crash is happening within SubstitutingProtocolHandler::ConstructInternal - to me that crash is completely unrelated to my patch.

Running this through TRY again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4937bc51a434cc864e74014835fb5fd0850add

Hey Nika,

I am not really sure how to move forward here. When running extensions/test/xpcshell/test_ext_startup_request_handler.js on Android we are experiencing a crash in the SubstitutingProtocolHandler::ConstructInternal(). In particular we hit MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv) && mIOService);

Here is a simpliefied stack but you can also look at the full treeherder log:

0  libxul.so!mozilla::net::SubstitutingProtocolHandler::ConstructInternal([SubstitutingProtocolHandler.cpp]
1  libxul.so!mozilla::net::ExtensionProtocolHandler::ExtensionProtocolHandler() [ExtensionProtocolHandler.cpp]
2  libxul.so!mozilla::net::ExtensionProtocolHandler::GetSingleton() [ExtensionProtocolHandler.cpp]
3  libxul.so!NS_NewURI(nsIURI**, nsTSubstring<char> const&, char const*, nsIURI*) [nsNetUtil.cpp]
4  libxul.so!mozilla::ContentPrincipal::GetSiteOriginNoSuffix(nsTSubstring<char>&) [ContentPrincipal.cpp]
5  libxul.so!mozilla::BasePrincipal::GetSiteOrigin(nsTSubstring<char>&) [BasePrincipal.cpp]
6  libxul.so!mozilla::dom::DocGroup::GetKey(nsIPrincipal*, bool, nsTSubstring<char>&) [DocGroup.cpp]

I really don't know why we would be causing this error on Android and how to move forward. In my opinion NS_NewURI for a moz-extension URI must have been called before we call our newly introduced NS_NewURI within GetSiteOriginNoSuffix(), right?

FWIW, I added a test for a moz-extension URI to caps/tests/unit/test_site_origin.js which does not fail/crash.

Any suggestions on how to move forward?

Flags: needinfo?(nika)

This crash is happening due to the call for DocGroup::GetKey being called from ~Document() during the final shutdown cycle-collection while shutting down the browser. This final CC is occurring after ShutdownFinal and after the entire nsComponentManager has been shut down. The call is crashing in a bit of a weird place, but the underlying cause is that NS_NewURI doesn't work if the component manager is gone, which is pretty understandable. It seems like it's largely luck which allowed us to not crash here previously, as it happened to be that getting the siteorigin for the extension URI would work after ShutdownFinal.

I don't think we should be trying to get the SiteOrigin getter to work during the final CC, but we should probably make sure it returns an error instead of crashing. I'm not sure we should be doing this in ContentPrincipal though - it seems like perhaps NS_NewURI should gracefully handle being called after component manager shutdown by returning an error.

Flags: needinfo?(nika)
Depends on: 1692422

looking ...

Flags: needinfo?(ckerschb)

(In reply to Cristian Brindusan [:cbrindusan] from comment #8)

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=329974751&repo=autoland&lineNumber=4980

Hey Nika, thanks for your support in figuring out things. It seems it's an endless story. I thought we got it covered within Bug 1692422 where we ensured that NS_NewURI returned an error in case the ioservice is not available anymore. Apparently now we have another problem (see failure log from above).

If you have any suggestions on how to move forward, please let me know. Thank you!

Flags: needinfo?(nika)

I don't have an immediate intuition for what is probably happening here, as if we're in the same situation, we should theoretically be returning NS_ERROR_NOT_AVAILABLE from GetSiteOrigin in DocGroup::GetKey, which should cause us to not run the assertion which is failing here: https://searchfox.org/mozilla-central/rev/d504494eab038fd1bbc82eb4191bb334b4de2c8d/dom/base/Document.cpp#4045

It may be useful to run the failing test with logging of both mDocGroup->mKey and docGroupKey to figure out what the specific mismatched keys are which are causing the failure?

Flags: needinfo?(nika)

So the problem is best illustrated when attaching the patch and running toolkit/components/antitracking/test/xpcshell/test_staticPartition_authhttp.js where we end up with a mismatch of

docGroupKey: http://localhost:37625
mDocGroup->GetKey: http://localhost

and the assertion Assertion failure: mDocGroup->MatchesKey(docGroupKey) within Document::AssertDocGroupMatchesKey() fires.

The problem is that NS_NewURI returns a failure because the IoService within NS_NewURI is not available anymore but within GetSiteOriginNoSuffix in the patch we return NS_OK in the following case if (NS_FAILED(NS_NewURI(getter_AddRefs(origin), aSiteOrigin))) {.

Summing it up, our approach tries to leverage the effect that NS_NewURI returns an error in case it can not parse the URI, but unfortunately also returns an error in case the ioservice is not available anymore, so our leveraging approach fails.

I am not quite sure how to move forward - any suggestions?

Flags: needinfo?(nika)

Probably the easiest solution, though hacky, is to basically check nsCOMPtr<nsIIOService> ioService = do_GetIOService(); within Document::AssertDocGroupMatchesKey and only assert if the ioService is non null. Is that an option?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)

The problem is that NS_NewURI returns a failure because the IoService within NS_NewURI is not available anymore but within GetSiteOriginNoSuffix in the patch we return NS_OK in the following case if (NS_FAILED(NS_NewURI(getter_AddRefs(origin), aSiteOrigin))) {.

Ah, I didn't remember that we hard-ignore any error returned by NS_NewURI. Perhaps we should change that behaviour and instead actively return the parse error if it occurs? It would be interesting to know if that causes any actual errors.

If it does, I think we should do the IOService check in GetSiteOriginNoSuffix and return something like NS_ERROR_NOT_AVAILABLE if we can't get the service.

Flags: needinfo?(nika)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #14)

Ah, I didn't remember that we hard-ignore any error returned by NS_NewURI. Perhaps we should change that behaviour and instead actively return the parse error if it occurs? It would be interesting to know if that causes any actual errors.

Yes, that works and I agree, we should simply return that error. And, FWIW, it's not causing any problems (as far as I can tell).

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/67eed121767e
Update GetSiteOriginNoSuffix() to handle view-source correctly. r=nika

https://hg.mozilla.org/mozilla-central/rev/67eed121767e

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: