Closed Bug 861302 Opened 7 years ago Closed 7 years ago

Allow indedexDB to be used from about:home

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1.0 (obsolete) — Splinter Review
Trying Boris, just by looking at ThirdPartyUtil.cpp hg log
Otherwise I suppose Sicking may review this...
Attachment #736906 - Flags: review?(bzbarsky)
Comment on attachment 736906 [details] [diff] [review]
patch v1.0

What guarantees that aHostURI can't be about:blank from some web page?
Flags: needinfo?(mak77)
Comment on attachment 736906 [details] [diff] [review]
patch v1.0

Pending response to comment 2...
Attachment #736906 - Flags: review?(bzbarsky)
(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
Flags: needinfo?(mak77)
ehr sorry, I meant about:blank
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.
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 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?
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?
Attached patch patch v1.1 (obsolete) — Splinter Review
This is a more restricted patch that just whitelists about:home for indexedDB
Attachment #736906 - Attachment is obsolete: true
Attachment #739858 - Flags: review?(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
Attachment #739858 - Flags: review?(bzbarsky) → review+
Oh, and I agree that this is the safest patch we can do here, and probably the right way to go.
Summary: Allow indedexDB to be used from about: pages → Allow indedexDB to be used from about:home
Attached patch patch v1.2Splinter Review
Thanks!
Attachment #739858 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6f68b9e02ffc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.