User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

1. Go to https://abrasax.club/readcookie.html
2. Click anywhere in the page to open another tab.
3. Click in Reader View icon.
4. Wait 15 seconds until the new tab is redirected to about:blank
5. Wait 4 seconds more to be redirected back to Reader View.
6. Reader View will render meta redirect page and SameSite cookie will be bypassed.

Actual results:

SameSite cookie is sent when Reader View is reloaded with a meta redirect.

Expected results:

SameSite cookie not sent.

Sorry, i forgot to tell to open https://poc.dup.house/pocs/setcookie.php to set samesite cookie before the first step

I see "not working :(" rendered in reader mode (tested with 86 beta, having opened both the cookie setting and reading pages in the same private browsing window), so it doesn't seem to work for me.

It'd help if you described in more detail:

• what the role of each of the 3-4 pages involved is
• why reader mode matters here
• what page is supposed to meta redirect to what
• why the exploit needs the history manipulation on the opened webpage

Hi Gijs, I'm sorry about the duplicate. I think I double-clicked the submission form. I just tested this issue in Firefox Nightly 87.0a1 (2021-02-11) (64-bit) for archlinux, and it seems to be working. I also tested Firefox 85.0.2 in Windows, and it worked too. In order to make it clear, I also annexed a video to this report.

The attack relies upon the fact that if you have a page "victim.com" with a SameSite cookies set, and then open the attacker's page containing a meta redirect to victim.com on Reader View, if you reload the page inside the Reader View, you get redirected to "victim.com", which sends the request with the user's cookies (including the cookies with SameSIte=Strict).

• what the role of each of the 3-4 pages involved is
  The first page (https://abrasax.club/readcookie.html) is the attacker-controlled website containing the meta redirect and Reader Mode available. The page (http://poc.dup.house/pocs/samesite.php) is the targeted page with SameSite cookies defined. The goal is to trick the user into opening https://abrasax.club/readcookie.html in Reader View, then I reload (or wait for the user to do it by himself). When it happens, the request is sent with the SameSite cookies.

• why reader mode matters here:
  Reader Mode matters because the bug seems to rely on the fact that if you open a page with meta refresh on Reader Mode and reload it inside the Reader Mode, the page is redirected to the meta tag page, and the request is sent with the SameSite cookies.

• what page is supposed to meta redirect to what
  In this scenario, the attacker's page redirects the user with a meta redirect to the targeted page (http://poc.dup.house/pocs/samesite.php), which has SameSite cookies defined and should not be sent. If you wait for 50 seconds, as defined in https://abrasax.club/readcookie.html meta redirect: meta http-equiv="refresh" content="50; url=https://poc.dup.house/pocs/samesite.php"> You should see a message saying: not working, since the SameSite cookie won't be sent. But when you open this page in reader mode and reload the page with Ctrl + R you get the message "same site policy bypassed".

• why the exploit needs the history manipulation on the opened webpage
  This isn't strictly needed. But since the bug occurs when the user reloads the page in Firefox Reader View, I manipulated the history to automate this stage in the PoC, so it wouldn't be necessary further user interaction to do it.

OK, I tried again and this time reproduction worked - not entirely sure why it didn't work before. I can't see a video - did you mean to attach one or link to one?

I'm a bit skeptical about what this can be used for - you cannot extract any content this way (you have no access to the reader mode page / document as it's not same origin), and reader mode only does GET requests, so I'm not sure how you'd abuse the samesite cookie presence, in addition to needing the user to enter reader mode? But perhaps I'm not creative enough... :-)

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. 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?

I'm also curious if we could just stop history navigation for cross-protocol principals (ie if https://example.com/ may not link to file:/// or about:reader?... or chrome://... then using windowRef.history.back() or whatever to navigate there also seems like it should be disallowed)... on the one hand that is more generic, on the other the benefit is probably limited in practice (ie there's not a lot of other cases where that restriction would make a difference, though it'd make some user-interaction-required attacks harder to execute). Anne, thoughts on that?

Hi Gijs, the attack scenario would be to exploit Cross Site Request Forgery in websites that implements samesite cookies in strict mode. By the way, about the video, I couldn't upload it here due to the size, can i create a private link on youtube?

(In reply to Matheus Vrech from comment #7)

Hi Gijs, the attack scenario would be to exploit Cross Site Request Forgery in websites that implements samesite cookies in strict mode.

A CSRF on a GET request, which is only secured against by using samesite cookies? That feels like it has other issues... (do people just not use CSRF tokens, or at least use POST/PUT/DELETE for operations that modify state, origin verification, custom request headers, or any other defense mechanisms anymore?). Older versions of IE, Edge, android webview, and even current Safari all have incomplete support for the SameSite attribute... :-(

By the way, about the video, I couldn't upload it here due to the size, can i create a private link on youtube?

Yes, that should be fine.

(In reply to :Gijs (he/him) from comment #8)

(In reply to Matheus Vrech from comment #7)

Hi Gijs, the attack scenario would be to exploit Cross Site Request Forgery in websites that implements samesite cookies in strict mode.

A CSRF on a GET request, which is only secured against by using samesite cookies? That feels like it has other issues... (do people just not use CSRF tokens, or at least use POST/PUT/DELETE for operations that modify state, origin verification, custom request headers, or any other defense mechanisms anymore?). Older versions of IE, Edge, android webview, and even current Safari all have incomplete support for the SameSite attribute... :-(

By the way, about the video, I couldn't upload it here due to the size, can i create a private link on youtube?

Yes, that should be fine.

video poc: https://youtu.be/cVz2XgrXcjA

Although people should use other methods to protect against CSRF I don't think we should just rely on that and ignore that this is still a samesite strict bypass. Samesite cookies is becoming an important protection against CSRF attacks and we should secure it well. I also disagree that we should consider it less important or lower due to other browsers implementations since Firefox has a well-working implementation and some people may be relying on it.

Yeah, I think isolating history more makes a lot of sense. We've had some discussion about this in https://github.com/whatwg/html/issues/6356 with regards to explicit user navigations and entering reader view should definitely count as well I think. We might also want to enforce the internal equivalent of COOP for reader view so once something goes into that view it appears closed to anyone who had a reference to that window.

(Adding Steven, since this is about cookies.)

(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. 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. 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.

(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. 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. 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.

Doesn't look like there are any JS consumers that create nsIRedirectHistoryEntry instances outside of tests. How safe is creating a JS "mock" one - nsIRedirectHistoryEntry isn't marked as builtinclass, but it feels pretty scary to pass a (mainthread only) JS thing into an API like that to leave it attached to the loadinfo and just hope nobody touches it away from the main thread... am I missing something?

Edit: also, what does "internal" [redirect] in the code comment at https://searchfox.org/mozilla-central/rev/a23e65c5d69a821f61d14c8ec1f69a120e3f77d1/netwerk/base/nsILoadInfo.idl#852 mean?

(In reply to :Gijs (he/him) from comment #12)

Doesn't look like there are any JS consumers that create nsIRedirectHistoryEntry instances outside of tests. How safe is creating a JS "mock" one - nsIRedirectHistoryEntry isn't marked as builtinclass, but it feels pretty scary to pass a (mainthread only) JS thing into an API like that to leave it attached to the loadinfo and just hope nobody touches it away from the main thread... am I missing something?

Yeah that is true and probably is, as you said, scary. We could always add an additional bit on the loadinfo, something like treatAsForeign and then update IsSameSiteForeign to account for it.

Edit: also, what does "internal" [redirect] in the code comment at https://searchfox.org/mozilla-central/rev/a23e65c5d69a821f61d14c8ec1f69a120e3f77d1/netwerk/base/nsILoadInfo.idl#852 mean?

Internal redirects are e.g in case of CSP upgrade-insecure-requests or also HSTS - basically everything that is implementation specific to Firefox. In contrast a 302 redirect is an actual redirect caused by a web server.

I'll try to find time for at least the fix from comment #11 / comment #13 in the coming cycle, but it's tricky with proton at the same time.

(In reply to Anne (:annevk) from comment #10)

Yeah, I think isolating history more makes a lot of sense. We've had some discussion about this in https://github.com/whatwg/html/issues/6356 with regards to explicit user navigations and entering reader view should definitely count as well I think. We might also want to enforce the internal equivalent of COOP for reader view so once something goes into that view it appears closed to anyone who had a reference to that window.

What is "the internal equivalent of COOP" right now and how would I do that? That is, is the "make this window appear closed and immutable to a caller" mechanism already available in DOM code?

Tom can probably answer that question (see comment 14).

Hmm, I don't know if the "make this window appear closed and immutable to a caller" mechanism already available in DOM code and where it is? So, redirect this question to Nika.

In COOP and COEP, we isolate windows by putting them into different BrowsingContext/document groups to restrict the access if it's needed.

Unfortunately we probably don't want to use the exact same strategy we use for COOP with about:reader tabs right now. When switching into and out of about:reader, we currently have some optimizations which traverse session history instead of performing a fresh load, which means that when viewing an about:reader page, the original page is likely in the bfcache, and after leaving the about:reader page, it probably ends up in the bfcache and can be quickly re-opened. Until the bfcache-fission rework in bug 1671510 is ready, and we switch to SHIP everywhere, bfcache is disabled for navigations which switch BrowsingContexts, and exiting or entering reader-mode would always lead to a fresh load.

We could potentially avoid the worst of this by only performing a COOP-like load if the about:reader tab has opener relationships, so we would keep bfcache in the majority of cases where no opener relationships are present.

(In reply to :Gijs (he/him) from comment #14)

What is "the internal equivalent of COOP" right now and how would I do that? That is, is the "make this window appear closed and immutable to a caller" mechanism already available in DOM code?

It's handled as part of how we perform potentially-process-switching navigations in DocumentLoadListener, during the MaybeTriggerProcessSwitch function. If we wanted to do a replace-load for about:reader tabs which share a BCG, it'd probably end up looking something like this (untested):

diff --git a/netwerk/ipc/DocumentLoadListener.cpp b/netwerk/ipc/DocumentLoadListener.cpp
index 25acb263d48bf..b03f0ad2dc725 100644
--- a/netwerk/ipc/DocumentLoadListener.cpp
+++ b/netwerk/ipc/DocumentLoadListener.cpp
@@ -1584,6 +1584,23 @@ bool DocumentLoadListener::MaybeTriggerProcessSwitch(
     }
   }
 
+  // If loading an about:reader page in a BrowsingContext which shares a
+  // BrowsingContextGroup with other toplevel documents, replace the
+  // BrowsingContext to destroy any references.
+  //
+  // FIXME: Ideally we'd do this for all loads to or from about:reader
+  // documents, but we limit it to documents with opener relationships for now
+  // to avoid unnecessary bfcache evictions. Once SHIP bfcache is complete, we
+  // should change this.
+  if (!parentWindow && browsingContext->Group()->Toplevels().Length() > 1) {
+    nsAutoCString filePath;
+    if (resultPrincipal->SchemeIs("about") &&
+        NS_SUCCEEDED(resultPrincipal->GetFilePath(filePath)) &&
+        filePath == "reader"_ns) {
+      replaceBrowsingContext = true;
+    }
+  }
+
   LOG(
       ("DocumentLoadListener GetRemoteTypeForPrincipal "
        "[this=%p, contentParent=%s, preferredRemoteType=%s]",

I have been absolutely swamped with other work, so I haven't gotten to this yet. I hope to have time sometime in the next month.

Hey, friendly ping!

(In reply to Matheus Vrech from comment #19)

Hey, friendly ping!

Yeah, I'm very sorry, the delay here has been ridiculous. This has been on my list but it keeps getting bumped...
I was OOO last week and I'm about to go out of office on Friday for another 2 weeks though, and I doubt I'm going to be able to squeeze this in today or tomorrow, either. Hopefully later in September, unless someone on the DOM team has more availability - the fix will need to live mostly in the DOM/loadinfo code, with probably a line or 2 of JS in the about:reader code to invoke the right API. Nika? :-(

(In reply to Matheus Vrech from comment #0)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

1. Go to https://abrasax.club/readcookie.html

FWIW, the https site no longer loads in either chrome or Firefox (Firefox says SSL_ERROR_RX_RECORD_TOO_LONG, Chrome says "ERR_SSL_PROTOCOL_ERROR"). The http version says:

404

NotFoundError: Not Found
    at /src/dist/app.js:181:33
    at Layer.handle [as handle_request] (/src/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/src/node_modules/express/lib/router/index.js:317:13)
    at /src/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/src/node_modules/express/lib/router/index.js:335:12)
    at next (/src/node_modules/express/lib/router/index.js:275:10)
    at /src/node_modules/express/lib/router/index.js:635:15
    at next (/src/node_modules/express/lib/router/index.js:260:14)
    at Function.handle (/src/node_modules/express/lib/router/index.js:174:3)
    at router (/src/node_modules/express/lib/router/index.js:47:12)
    at Layer.handle [as handle_request] (/src/node_modules/express/lib/router/layer.js:95:5)
    at trim_prefix (/src/node_modules/express/lib/router/index.js:317:13)
    at /src/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/src/node_modules/express/lib/router/index.js:335:12)
    at next (/src/node_modules/express/lib/router/index.js:275:10)
    at urlencodedParser (/src/node_modules/body-parser/lib/types/urlencoded.js:91:7)

I'll try running the poc locally but it'd be useful to have this hosted to ensure there's no mistakes in me trying to reproduce the same environment, and to help with QA verification once there is a fix. Is it possible to get the PoC back up?

I'm sorry, I had to use this address to another PoC but I'll fix it tonight. Until then you can use https://l4t.cc/readcookie.html in the same way, just remember to set cookies in http://poc.dup.house/pocs/samesite.php before testing.

Steps:

• Set cookies in poc.dup.house https://poc.dup.house/pocs/setcookie.php;
• go to l4t.cc https://l4t.cc/readcookie.html;
• click anywhere in the page. It will open a new window;
• view this window in reader mode;
• wait around 15 seconds when this page is redirected to https://poc.dup.house/pocs/samesite.php showing that samesite cookies were sent.

Please let me know if you had any issue reproducing in this environment.

(In reply to Matheus Vrech from comment #22)

I'm sorry, I had to use this address to another PoC but I'll fix it tonight.

No worries! I'm aware it's been an embarrassingly long time since there was much traction from our side.

Until then you can use https://l4t.cc/readcookie.html in the same way, just remember to set cookies in http://poc.dup.house/pocs/samesite.php

https://poc.dup.house/pocs/setcookie.php right?

before testing.

Thanks. I also got it to work locally, I think, so I should be OK for now at least for the fixes being written.

Yes! I meant to set it in setcookie to configure the environment (https://poc.dup.house/pocs/setcookie.php), sorry.

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. 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. 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 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? 😓

(In reply to :Gijs (he/him) from comment #26)

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. 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?

If the 2nd request was indeed another XHR, then we could set a loadflag, but it seems that the redirect target URI is being fed into the reject() callback of that promise and then propagates through various outside callers up to the parent code in aboutreader.jsm, which redirects using something like location.replace(). I'm not really sure how we can do this in a JavaScript-triggered redirect from one about:reader page to another about:reader page. One might be able to hack this redirect info into the subsequent about:reader-URL and then call the following XHR differently, but that seems icky. Leaving the needinfo for Christoph, to see if he has a better idea.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.

I am painfully aware of this patch and the fact that I need to figure out whether we can either do something reasonable to pass previous channel info for the new reader mode load, or rip out the meta-redirect-following completely, or "only" land the r+ patch -- thanks, reviewbot...

As I discussed with Nika last week, it's not clear to me whether trying to manually hack together a way to pass the redirect info to the XHR is going to be a case of whack-a-mole where we have to keep updating it to pass more and more other information which may also impact security, and having to effectively build a second path for this kind of redirect to happen (trying to mimic a "real" meta redirect and all its indirections as closely as possible) is pretty unattractive. I haven't made a decision yet; any and all suggestions still gratefully received...

Niklas/Micah, do you have thoughts here? In particular, I'm wondering if we can just remove the <meta> redirect handling code entirely. AFAIK it's only needed for cases where you want to manually use about:reader?url=..., as we should just be grabbing the current document otherwise. Does that sound reasonable?

(In reply to :Gijs (he/him) from comment #30)

Niklas/Micah, do you have thoughts here? In particular, I'm wondering if we can just remove the <meta> redirect handling code entirely. AFAIK it's only needed for cases where you want to manually use about:reader?url=..., as we should just be grabbing the current document otherwise. Does that sound reasonable?

I'm not entirely sure why we'd need to have the redirect for about:reader urls, so I think it's fine if that's removed!

(In reply to Micah [:mtigley] (she/her) from comment #31)

(In reply to :Gijs (he/him) from comment #30)

Niklas/Micah, do you have thoughts here? In particular, I'm wondering if we can just remove the <meta> redirect handling code entirely. AFAIK it's only needed for cases where you want to manually use about:reader?url=..., as we should just be grabbing the current document otherwise. Does that sound reasonable?

I'm not entirely sure why we'd need to have the redirect for about:reader urls, so I think it's fine if that's removed!

Having just looked at this, I realized that if we remove all the redirection logic we end up re-creating bug 1195976 and/or bug 1182778. :-\

Nika, am I missing a more "obvious" solution here? Basically we want to be able to navigate from about:reader?url=foo to about:reader?url=bar, but the resulting load in about:reader?url=bar that fetches bar should preserve whatever characteristics a navigation from foo to bar would have had, esp. from a security/usercontextid/(same-site)cookies/private-browsing/etc. perspective.

FWIW, The current redirect uses location.replace(), which is basically like a new, context-less top-level navigation.
It looks like what we need is something that looks like a server-side redirect. I know that we have various instance of this problem, but so far we've usually created a new channel which copied things out of the previous channel. Unfortunately, that is known to be error-prone and often times missing various bits of important context (CSP, OriginAttributes etc etc etc.).

IIRC the redirect which it's trying to mimic isn't a server-side redirect, but rather a client-side refresh using a <meta http-equiv=refresh> tag. We definitely want to keep around the logic for updating the URL bar after a server-side redirect. I'm not convinced that bug 1195976 would reoccur if we removed support for meta refresh tags as the source of that redirect was an HTTP 302, which is handled seperately: https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/toolkit/components/reader/ReaderMode.jsm#375

I'm not entirely sure I understand why bug 1182778 would be re-created if we remove this check so I may be missing something there.

I don't think we want to make meta refresh tags act like server-side redirects under the hood, as that's also not how they act for document loads (they're triggered as new toplevel loads after the specified delay but with a few extra flags set).

If we wanted to make how about:reader loads URIs reflect how documents are loaded as accurately as possible, our main option would probably to make it act like how we handle view-source: URIs (with all of the complexity everywhere which comes with that, which I'm guessing is why the feature was not built that way to begin with), and then to have reader-mode for documents with meta refresh tags generate their own tags internally when rendering (as view-source doesn't respect meta refresh tags itself).

TL;DR: If the bug here only has to do with meta refresh tags and not with server-side redirects, I think the easiest solution is to rip out the meta tag handling here: https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/toolkit/components/reader/ReaderMode.jsm#315-356, which I don't think will lead to any new or existing security issues, as we won't render the target of the refresh on the wrong page, we'll just render the refresh page itself. If we also have problems with server-side redirects, we may need to do more.

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

IIRC the redirect which it's trying to mimic isn't a server-side redirect, but rather a client-side refresh using a <meta http-equiv=refresh> tag. We definitely want to keep around the logic for updating the URL bar after a server-side redirect. I'm not convinced that bug 1195976 would reoccur if we removed support for meta refresh tags as the source of that redirect was an HTTP 302, which is handled seperately: https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/toolkit/components/reader/ReaderMode.jsm#375

I'm not entirely sure I understand why bug 1182778 would be re-created if we remove this check so I may be missing something there.

No, you are correct - I think I got confused. The reason for my confusion was that in terms of the reader mode implementation, the handling is almost identical - we do an XHR, and if either the response URL is different from the request URL, or we get a meta refresh in the resulting document, we bubble up to the parent that we need to load the target page instead. (Note that reader mode doesn't actually honour the delay at all, which is a separate, non-security bug...)
But in the meta redirect case, we can render the original document, whereas in the 302 case the XHR just finishes the redirect for us and provides us with the post-redirect document, so the only alternative is rendering a blank page or similar.

Back to this bug rather than the old spoofing issue: Because in the attack proposed in comment 0 / comment 5, both the attacking page and the page that provides the meta redirect are under attacker control, I expect that you could do the same thing with a 302. At the moment the page with the meta redirect is static, so it'd be more involved: you'd need the client-side to communicate to the server-side that the next time it gets a request for this page, it should return the 302, so that when the original attacking page forces a reload of the reader mode page, the request gets a 302, and we "manually" tell the parent to open the target page instead. But that seems pretty doable, as all these pages (except the target) are attacker-controlled. You might even be able to combine with a service worker and do it that way; I'm not sure.

I don't think we want to make meta refresh tags act like server-side redirects under the hood, as that's also not how they act for document loads (they're triggered as new toplevel loads after the specified delay but with a few extra flags set).

If we wanted to make how about:reader loads URIs reflect how documents are loaded as accurately as possible, our main option would probably to make it act like how we handle view-source: URIs (with all of the complexity everywhere which comes with that, which I'm guessing is why the feature was not built that way to begin with), and then to have reader-mode for documents with meta refresh tags generate their own tags internally when rendering (as view-source doesn't respect meta refresh tags itself).

Yeeeeaahhh. To paraphrase Marie Kondo, view-source does not bring me joy. It'd be a pretty big project to redo all the URL surface for reader mode so that it was its own protocol, and then ensure that it gets the same handling/complexity that you describe. I can file a separate bug for that if you think it's worth it?

TL;DR: If the bug here only has to do with meta refresh tags and not with server-side redirects, I think the easiest solution is to rip out the meta tag handling here: https://searchfox.org/mozilla-central/rev/4615b544a0f7166233c409c619b426c4025467a7/toolkit/components/reader/ReaderMode.jsm#315-356, which I don't think will lead to any new or existing security issues, as we won't render the target of the refresh on the wrong page, we'll just render the refresh page itself. If we also have problems with server-side redirects, we may need to do more.

Right, so, here's what I think I would propose, which is a little different but hopefully still doable and less work than the view-source route (I think):

1. rip out the meta refresh tag handling, but leave the 302 handling [for now]
2. make history.push/replaceState work for unlinkable about: URIs. I don't think we need to jump all the way to having nsStandardURL for about:, but we should be able to make the security checks pass if the bits before ? and/or # are the same. This is also worthwhile for other projects that frontend is considering - the fact that about URIs can't use this API is annoying in other about: pages too and restricts the options frontend has for various kinds of UI. (I've just been asked for a new page to have opening a "dialog" within that page add a back button entry, and although I could do it with just #foo hash manipulation, they are pretty ugly...)
3. use that instead of location.replace() for the 302 handling. That way we leave the XHR code to handle the redirect, which means all the network code will be handled appropriately, instead of making it all up ourselves.

Does that seem like a reasonable alternative to going whole-hog for the view-source-like option?

(In reply to :Gijs (he/him) from comment #35)

Yeeeeaahhh. To paraphrase Marie Kondo, view-source does not bring me joy. It'd be a pretty big project to redo all the URL surface for reader mode so that it was its own protocol, and then ensure that it gets the same handling/complexity that you describe. I can file a separate bug for that if you think it's worth it?

I don't love the idea of switching to using view-source if we can avoid it, as it comes hand-in-hand with a lot of complexity which I'd prefer to avoid expanding upon, so if we can find a good solution I'd prefer not to switch to that strategy.

Right, so, here's what I think I would propose, which is a little different but hopefully still doable and less work than the view-source route (I think):

1. rip out the meta refresh tag handling, but leave the 302 handling [for now]
2. make history.push/replaceState work for unlinkable about: URIs. I don't think we need to jump all the way to having nsStandardURL for about:, but we should be able to make the security checks pass if the bits before ? and/or # are the same. This is also worthwhile for other projects that frontend is considering - the fact that about URIs can't use this API is annoying in other about: pages too and restricts the options frontend has for various kinds of UI. (I've just been asked for a new page to have opening a "dialog" within that page add a back button entry, and although I could do it with just #foo hash manipulation, they are pretty ugly...)
3. use that instead of location.replace() for the 302 handling. That way we leave the XHR code to handle the redirect, which means all the network code will be handled appropriately, instead of making it all up ourselves.

Does that seem like a reasonable alternative to going whole-hog for the view-source-like option?

Using that approach unfortunately means that we wouldn't be able to re-do process selection for the post-redirect URI as we're not doing a fresh load of the about:reader URI, and we have special-case logic to isolate about:reader URIs based on their origin, so I think I'd prefer to continue to handle things similar to how we're handing them right now when it comes to doing independent about:reader loads.

I think a better solution might actually be to be more clever about how we handle redirects in the ReaderMode code. Theoretically we could cache the response which we get from the redirect, which we've already performed when we get the response back in _downloadDocument. Conveniently we already have a system for holding onto a response while we wait for the URI to load in a potentially different process using the gCachedArticles map (https://searchfox.org/mozilla-central/rev/a17011de808c24f015ad03debe7a157c1b43b602/browser/actors/AboutReaderParent.jsm#40-43), so potentially we could change ReaderMode to perform the redirect by pushing the article data into the parent process, storing it in that map, and then navigating to the new about:reader page. We'd then use the Reader:GetCachedArticle codepath to fetch the cached article and it wouldn't be re-loaded after the redirect.

Does that seem like a practical approach to take?

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

Does that seem like a practical approach to take?

Yes, this seems like it should work. I'll see if I can find time to put up a patch this week.

(In reply to :Gijs (he/him) from comment #37)

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

Does that seem like a practical approach to take?

Yes, this seems like it should work. I'll see if I can find time to put up a patch this week.

We had a 0-day and some other high prio work come up so I didn't finish doing this - I do have a patch that does this now, I think, but I need to find time to create an automated test.

Depends on D141359

Well, it took way too long, but patches are finally up, at least...

Comment on attachment 9268259 [details]
Bug 1692655 - tests, r?mtigley

Revision D141360 was moved to bug 1760602. Setting attachment 9268259 [details] to obsolete.

Nika, do you think we should also land the opener relationship patch from 6 months ago?

Landed: https://hg.mozilla.org/integration/autoland/rev/37f58bd49a56f6e2633d04a77895b6954abc6bcb

Backed out for causing linting failure:

https://hg.mozilla.org/integration/autoland/rev/8641c2aa19f12c80939776ed9ec2ea37b4f43ab9

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry&revision=37f58bd49a56f6e2633d04a77895b6954abc6bcb&selectedTaskRun=ailCzzGrTuuo8XXAYj_vww.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=371764620&repo=autoland

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/reader/ReaderMode.jsm:284:7 | Expected an error object to be thrown. (no-throw-literal) 

(In reply to :Gijs (he/him) from comment #43)

Nika, do you think we should also land the opener relationship patch from 6 months ago?

Sure, shouldn't hurt. Took a quick look over it again, and I think the feedback I posted back then is still relevant, but with that fixed it should be landable.

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #44)

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/reader/ReaderMode.jsm:284:7 | Expected an error object to be thrown. (no-throw-literal) 

That's a fun one :-)

Perhaps we should just return from that method as well & just return an object, because IIRC the caller is the one which catches the exception anyway?

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

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #44)

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/reader/ReaderMode.jsm:284:7 | Expected an error object to be thrown. (no-throw-literal) 

That's a fun one :-)

Perhaps we should just return from that method as well & just return an object, because IIRC the caller is the one which catches the exception anyway?

Either that or return a Promise.reject() which I had toyed with doing anyway. Will look more in my morning.

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

(In reply to :Gijs (he/him) from comment #43)

Nika, do you think we should also land the opener relationship patch from 6 months ago?

Sure, shouldn't hurt. Took a quick look over it again, and I think the feedback I posted back then is still relevant, but with that fixed it should be landable.

I'm a bit confused - I think I did address the SHIP check, and you r+'d that without further comments. Was there something else?

You're right, nevermind. I think I had misunderstood what my comment was trying to say.

r=nika,mtigley,smaug
https://hg.mozilla.org/integration/autoland/rev/f7ffaf8f64eb94dbe40df50d78fd9de92bb48b06
https://hg.mozilla.org/mozilla-central/rev/f7ffaf8f64eb

ensure loading reader mode in windows with an opener relationship creates a new BC, r=nika
https://hg.mozilla.org/integration/autoland/rev/26d9410c112bc9e857954158146fdee6638889cc
https://hg.mozilla.org/mozilla-central/rev/26d9410c112b

Comment on attachment 9273705 [details]
Bug 1692655 - esr

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sending of samesite cookie when it shouldn't be sent using readermode
• User impact if declined: Potential samesite strict bypass when using readermode
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is fairly straightforward, and has already landed on nightly

Comment on attachment 9273705 [details]
Bug 1692655 - esr

Approved for 91.9esr.

https://hg.mozilla.org/releases/mozilla-esr91/rev/f191afdbef72

Hi Matheus!

I've tried to reproduce this bug using an affected Nightly build (2021-02-12) Win 10 x64, but it seems that I'm having some hard time reproducing the initial problem. After I set the samesite cookie before the first step, and clicking anywhere on the index.html page, I'm redirected to https://abrasax.club/readcookie.html, which is unavailable ("Hmm. We’re having trouble finding that site." warning is displayed in Firefox). I'm doing something wrong here, maybe that page is missing from the poc file posted in comment 1?

Could you please take a look at this, or if you think that would be easier, you can also verify the fix on Esr91.9 and Firefox 100, since you understand better this bug?

(In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from comment #56)

Hi Matheus!

I've tried to reproduce this bug using an affected Nightly build (2021-02-12) Win 10 x64, but it seems that I'm having some hard time reproducing the initial problem. After I set the samesite cookie before the first step, and clicking anywhere on the index.html page, I'm redirected to https://abrasax.club/readcookie.html, which is unavailable ("Hmm. We’re having trouble finding that site." warning is displayed in Firefox). I'm doing something wrong here, maybe that page is missing from the poc file posted in comment 1?

Could you please take a look at this, or if you think that would be easier, you can also verify the fix on Esr91.9 and Firefox 100, since you understand better this bug?

Hello Ciprian!

I couldn't reproduce the bug anymore (Version: 101.0a1, Build ID: 20220428214715, User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:101.0) Gecko/20100101 Firefox/101.0). Please, feel free to test it again under the addresses: https://dup.house/vrechson-bug-1692655/setcookie.php, https://dup.house/vrechson-bug-1692655/samesite.php, and https://l4t.cc/readcookie.html.

Nika, did you intentionally not uplift (or ask for uplift for) the opener relationship patch for ESR? QA picked this up when verifying the bug.

Thanks Gijs and Matheus! I was able to reproduce the bug using the updated test pages from comment 58, on an affected Nightly build (99.0a1, Build ID 20220303190240).

The exploit is not reproducing anymore on the fixed builds, 91.9.0 Esr and 100.0 RC2. Tested across platforms, Win 10 x64, macOS 10.15 and Ubuntu 18.04 x64. The remaining patch (mentioned in comment 59) will be verified separately, if it gets uplifted in an upcoming ESR.

(In reply to :Gijs (he/him) from comment #59)

Nika, did you intentionally not uplift (or ask for uplift for) the opener relationship patch for ESR? QA picked this up when verifying the bug.

I intentionally didn't ask for uplift on that patch because the code for process selection has changed so much since 91 (ProcessIsolation.cpp didn't even exist), that porting it would be a large hassle, and it's technically not necessary for the bugfix, mostly a defense-in-depth strategy thing.