861302 - Allow indedexDB to be used from about:home

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Marco Bonardo [:mak]]

It's currently not working, we have a patch by Jan Varga that fixes the issue though we need a content developer to evaluate its safety.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Trying Boris, just by looking at ThirdPartyUtil.cpp hg log
Otherwise I suppose Sicking may review this...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736906 [details] [diff] [review]
patch v1.0

What guarantees that aHostURI can't be about:blank from some web page?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736906 [details] [diff] [review]
patch v1.0

Pending response to comment 2...

Comment 4

[Marco Bonardo [:mak]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 736906 [details] [diff] [review]
> patch v1.0
> 
> What guarantees that aHostURI can't be about:blank from some web page?

I'd say nothing, is about:home the only problematic case? I suppose the other about pages are more under our (or add-ons) control.
I can make about:home return NS_ERROR_INVALID_ARG

Comment 5

[Marco Bonardo [:mak]]

ehr sorry, I meant about:blank

Comment 6

[Boris Zbarsky [:bzbarsky]]

Addons can add about: stuff with arbitrary security characteristics, including making them same-origin with random websites.

The real question I had was whether the incoming URI is from a _principal_ or whether it's a document URI.  And if it's the latter, why.

Comment 7

[Marco Bonardo [:mak]]

So the call comes from nsGlobalWindow::GetIndexedDB, it invokes thirdPartyUtil->IsThirdPartyWindow(this, nullptr, &isThirdParty), so not passing any URI.
This invokes ThirdPartyUtil::GetURIFromWindow, that QI the window to nsIScriptObjectPrincipal, then it invokes GetPrincipal() and from the principal GetURI.  The URI is then directly passed to GetBaseDomain.

I skimmed through the getBaseDomain consumers and I didn't see much issues around.
Though, most of the ThirdPartyUtils methods base the result on IsThirdPartyInternal, that compares 2 domains.  Since both of them may be acquired through GetBaseDomain, that means it will return false if file:// and about: uris are compared, or even if about: uris with different privileges are compared.  I don't know how problematic that may be though, since I don't know much about the security checks that rely on ThirdPartyUtils.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736906 [details] [diff] [review]
patch v1.0

Oh, hmm.  Right, this is not just changing the indexeddb code... :(

Can we somehow special-case the about: stuff just for indexeddb, perhaps?

One other thing: this is apparently about: URIs that are _not_ system-principal that we're talking about here?

Comment 9

[Marco Bonardo [:mak]]

So far I only need it from about:home, that is content privileged.  I can't predict future needs though, but probably we should attack that problem when it arises.

I could hack a bypass in nsGlobalWindow::GetIndexedDB, checking if the URI is for an about page and skipping the isThirdPartyWindow check. Or even providing a whitelist that so far would just be about:home.
But again I admit my ignorance whether that would be safe to do, I suppose the whitelist of URIs that can skip the isThirdPartyWindow check is the safer path?

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This is a more restricted patch that just whitelists about:home for indexedDB

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 739858 [details] [diff] [review]
patch v1.1

>+          nsAutoCString spec;
>+          skipThirdPartyCheck = NS_SUCCEEDED(uri->GetSpec(spec)) &&
>+                                spec.EqualsLiteral("about:home");

Better (or at least less string-copying) would be:

          nsCAutoString path;
          skipThirdPartyCheck = NS_SUCCEEDED(uri->GetPath(path)) &&
                                path.EqualsLiteral("home");

r=me with that

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, and I agree that this is the safest patch we can do here, and probably the right way to go.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Thanks!

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f68b9e02ffc

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6f68b9e02ffc