Bugzilla

I have a working patch based on Nika's suggestion, with a test. This breaks the exploit because about:reader creates its own BC, severing the `opener` relationship, so the attacker can't navigate back into reader mode.

However, I would prefer also to fix at least 1 other part of the problem here, namely how reader mode itself deals with redirects, ie:

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to :Gijs (he/him) from comment #6)
> > I'm also not 100% sure how to fix this. Christoph, when reader mode is invoked without a document but with just a URL, it has to fetch the content itself. It's using XHR for that (in order to parse the returned HTML doc) and [manually follows meta redirects](https://searchfox.org/mozilla-central/rev/951a0342d600a1db263cac35ac85dbd4b3305aeb/toolkit/components/reader/ReaderMode.jsm#296). Is there some way we can communicate to the second XHR that is following the meta redirect, ie that it's a redirect chain, such that we wouldn't send samesite cookies?
> 
> The same-site cookie code checks for cross-origin redirects within [IsSameSiteForeign](https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieCommons.cpp#544-558). So we could potentially query the loadinfo from the XHR request and fake a redirectEntry using `loadinfo->appendRedirectHistoryEntry()` so that `IsSameSiteForeign` can do it's job. I guess that could work.

Unfortunately, I hit a bit of a wall here - for other security reasons, it's important we update the toplevel URL when following a meta redirect (cf bug 1182778), ie we don't want to have a case where the document loaded is `about:reader?url=foo.com` and actually the content shown is from `bar.com` rather than `foo.com`. To do this, we actually navigate to a new about:reader?url=... page when following the redirect. This means that by the time we run the XHR for the second load, the original info for the first redirect is gone. Because the navigation to the new page happens as a pretty simple assignment to window.location, it's not obvious how we add more info - short of adding a bunch of other URL parameters. That feels yucky, will likely mean auditing all the current URI consumers to check nobody is relying on there not being any other URL parameters, and is also a bit annoying in terms of the API surface that nsILoadInfo exposes. Specifically, when I looked at that, I realized that all the current consumers do the same thing to determine the arguments to the nsIRedirectHistoryEntry constructor: they inspect the "old" channel. So I have a patch to push that logic into the AppendRedirectHistoryEntry code itself (which is net code removal, yay!), and taking an `nsIChannel` argument instead -- but that makes it harder to add information passing for reader mode. :-\

My next thought was to use `history.replaceState` to fake the navigation, so that we do this immediately when the initial XHR fails, but unfortunately (a) we fail [here](https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/docshell/base/nsDocShell.cpp#11097-11099) because we can't get a userpass info for the magical about: URL, but even if we fix that, no `about:reader` url is ever same origin with another as far as `nsIScriptSecurityManager::CheckSameOriginURL` is concerned, because you can't QI those URLs to `nsIStandardURL`. I am... not convinced we want to open that can of worms. :-\

Christoph or Freddy, do you have any clever ideas here? Am I missing something obvious? If not, how would we feel about "only" shipping the BC separation to address this, rather than also fixing reader mode to pass more accurate info for the redirect? Or do you think I should just bite the bullet and start adding more URL params, or changing how CheckSameOriginURL works for about: URLs? 😓

I have a working patch based on Nika's suggestion, with a test. This breaks the exploit because about:reader creates its own BC, severing the `opener` relationship, so the attacker can't navigate away from or back into reader mode.

However, I would prefer also to fix at least 1 other part of the problem here, namely how reader mode itself deals with redirects, ie:

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to :Gijs (he/him) from comment #6)
> > I'm also not 100% sure how to fix this. Christoph, when reader mode is invoked without a document but with just a URL, it has to fetch the content itself. It's using XHR for that (in order to parse the returned HTML doc) and [manually follows meta redirects](https://searchfox.org/mozilla-central/rev/951a0342d600a1db263cac35ac85dbd4b3305aeb/toolkit/components/reader/ReaderMode.jsm#296). Is there some way we can communicate to the second XHR that is following the meta redirect, ie that it's a redirect chain, such that we wouldn't send samesite cookies?
> 
> The same-site cookie code checks for cross-origin redirects within [IsSameSiteForeign](https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieCommons.cpp#544-558). So we could potentially query the loadinfo from the XHR request and fake a redirectEntry using `loadinfo->appendRedirectHistoryEntry()` so that `IsSameSiteForeign` can do it's job. I guess that could work.

Unfortunately, I hit a bit of a wall here - for other security reasons, it's important we update the toplevel URL when following a meta redirect (cf bug 1182778), ie we don't want to have a case where the document loaded is `about:reader?url=foo.com` and actually the content shown is from `bar.com` rather than `foo.com`. To do this, we actually navigate to a new about:reader?url=... page when following the redirect. This means that by the time we run the XHR for the second load, the original info for the first redirect is gone. Because the navigation to the new page happens as a pretty simple assignment to window.location, it's not obvious how we add more info - short of adding a bunch of other URL parameters. That feels yucky, will likely mean auditing all the current URI consumers to check nobody is relying on there not being any other URL parameters, and is also a bit annoying in terms of the API surface that nsILoadInfo exposes. Specifically, when I looked at that, I realized that all the current consumers do the same thing to determine the arguments to the nsIRedirectHistoryEntry constructor: they inspect the "old" channel. So I have a patch to push that logic into the AppendRedirectHistoryEntry code itself (which is net code removal, yay!), and taking an `nsIChannel` argument instead -- but that makes it harder to add information passing for reader mode. :-\

My next thought was to use `history.replaceState` to fake the navigation, so that we do this immediately when the initial XHR fails, but unfortunately (a) we fail [here](https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/docshell/base/nsDocShell.cpp#11097-11099) because we can't get a userpass info for the magical about: URL, but even if we fix that, no `about:reader` url is ever same origin with another as far as `nsIScriptSecurityManager::CheckSameOriginURL` is concerned, because you can't QI those URLs to `nsIStandardURL`. I am... not convinced we want to open that can of worms. :-\

Christoph or Freddy, do you have any clever ideas here? Am I missing something obvious? If not, how would we feel about "only" shipping the BC separation to address this, rather than also fixing reader mode to pass more accurate info for the redirect? Or do you think I should just bite the bullet and start adding more URL params, or changing how CheckSameOriginURL works for about: URLs? 😓