1416307 - When RefreshURI gets called with a null principal, we end up using the page's referrer as a principal

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Valentin Gosu [:valentin] (he/him)]

(In reply to Valentin Gosu [:valentin] from bug 1414425 comment 12)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-))
> > Comment on attachment 8926440 [details] [diff] [review]
> > Make sure the appropriate triggeringPrincipal is set for a meta refresh
> > 
> > >+      *   May be null, in which case a principal will be built based on the
> > >+      *   referrer URI, or will use the system principal when there is none.
> > 
> > What referrer URI?  This API doesn't take a referrer URI...
> 
> We get the referrer from the docshell:
> http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/docshell/base/nsDocShell.cpp#6940-6942
> 
> And we use that if there is no triggeringPrincipal
> http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/docshell/base/nsDocShell.cpp#1571-1579

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment bug 1414425 comment 16)
> Comment on attachment 8926888 [details] [diff] [review]
> Make sure the appropriate triggeringPrincipal is set for a meta refresh
> 
> >+++ b/browser/base/content/tab-content.js
> 
> OK, so now that we understand the setup here, this code looks like a
> security bug.  It will use the referrer of the page in the docshell as the
> triggering principal, whereas it should probably be using that page's
> principal or so...  Please file a followup bug on this?
> 
> >+++ b/browser/base/content/test/general/browser_refreshBlocker.js
> 
> Similar.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Valentin, are you going to take this?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I'd rather someone else took this. Thanks!

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Samael, please help this, thanks!

Comment 4

[Frederik Braun [:freddy]]

Can you bump this in your queue, Samael?

Comment 5

[Samael Wang [:freesamael] (away for now)]

Attachment: Set triggeringPrincipal to the principal constructed from mCurrentURI in ForceRefreshURI in case of meta refresh. r?bz

MozReview-Commit-ID: IFipvjSf8GG

Comment 6

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8940079 [details] [diff] [review]
Set triggeringPrincipal to the principal constructed from mCurrentURI in ForceRefreshURI in case of meta refresh. r?bz

Since the referrer is set to mCurrentURI for non-meta refreshes, it looks to me that all we want to do is to set triggeringPrincipal based on mCurrentURI in the case of a meta refresh. Is that right?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8940079 [details] [diff] [review]
Set triggeringPrincipal to the principal constructed from mCurrentURI in ForceRefreshURI in case of meta refresh. r?bz

I don't think we should be involving referrers or uris in any way here.  If we want to use the currently loaded document's principal as the triggering principal, let's do that directly instead of using things that might sort of match it, maybe, or might not.

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: v1

MozReview-Commit-ID: 8lxKFeUlyzq

Comment 9

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8942131 [details] [diff] [review]
v1

Could you take a look on this one?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8942131 [details] [diff] [review]
v1

GetDocument() has side-effects.  Probably better to not call it until we really need it (so if aPrincipal) is null.

You need to update the API documentation at https://searchfox.org/mozilla-central/rev/7476b71e0010ab3277b77cc0ae4d998c4b1d2b64/docshell/base/nsIRefreshURI.idl#38-41

r=me with those two issues fixed.

Comment 11

[Samael Wang [:freesamael] (away for now)]

Attachment: Set triggeringPrincipal to current document's principal, if aPrincipal is not given. r=bz

MozReview-Commit-ID: 8lxKFeUlyzq

Comment 12

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8942574 [details] [diff] [review]
Set triggeringPrincipal to current document's principal, if aPrincipal is not given. r=bz

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The triggering principal on meta refresh would become the referrer's principal whereas it should be the current document's principal. Various security checks can be incorrect, but I don't really know how to construct an attack based on this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
All supported branches are affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, but it should be farely easy.

How likely is this patch to cause regressions; how much testing does it need?
Should be at low risk.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

This can land on February 6, two weeks into the next cycle. We're about to make the Firefox 58 release candidate. This missed the window for sec-approval now.

Comment 14

[Samael Wang [:freesamael] (away for now)]

We'll just need someone to land the patch on 2/6, and maybe also rebase the patch for ESR later, not sure.

Comment 15

[Andrew Overholt [:overholt]]

Olli, can you land this on February 6?

Comment 16

[Andrew Overholt [:overholt]]

Does this *need* to land today? smaug is on PTO this week ...

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcf48189000ab8ee16f236be57c9659c7243b42

This grafts cleanly to Beta and ESR52 as-landed. Please request approval on it when you get a chance.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9fcf48189000

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8942574 [details] [diff] [review]
Set triggeringPrincipal to current document's principal, if aPrincipal is not given. r=bz

[Approval Request Comment]
See comment 12

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8942574 [details] [diff] [review]
Set triggeringPrincipal to current document's principal, if aPrincipal is not given. r=bz

Sec-high, Beta59+, ESR52.7+

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9df27a7142ff

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/54f2f7f93b30