1250482 - URI spoof when opening page in a new tab with a moz-action URI

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1228754 +++

losslessDecodeURI does not like moz-action URIs.

(In reply to Abdulrahman Alqabandi from bug 1228754 comment #28)
> Here is a small bug that I believe is due to the faulty behavior described
> here.
> 
> PoC:
> 
> <a id='q' href='moz-action:switchtab,' target='_blank'>click</a>
> <script>q.onclick=$=>{
> 	setTimeout($=>{alert('View the new tab and check the
> url\n\n\n\nby:@qab')},33);
> }</script>
> 
> http://js.do/code/80600
> 
> For some reason the new tab contains the same url as the opener (once you
> switch tabs)

This is a full URL spoof, but I don't know if the originating code can control the content in this case, and in any case, the URL in question is the URL of the originating page, so without more detail/context it's not clear how you would use this to make the page in question actually spoof to "spoof.com" rather than "evil.com", so to speak.

Comment 1

[Marco Bonardo [:mak]]

I think Drew recently changed the code to not pass moz-action URIs through losslessDecodeURI, but maybe I misremember.

Btw, if I try the POC I only end up on a new tab with an about:blank address...

Comment 2

[Marco Bonardo [:mak]]

ah ok, I see the original tab url if I switch tabs again.

Comment 3

[:Gijs (he/him)]

Attachment: Patch v1.0

Marco, I know you're busy, but this patch is quite small and hopefully, like me, you still have the URLBarSetURI stuff paged in. The other change is related to what happens if you put in just:

moz-action:foo

without the comma, which currently throws an exception because the url starts with moz-action:, but doesn't match the regexp later, and so the destructuring assignment fails, causing other breakage.

Comment 4

[:Gijs (he/him)]

(I'll write a test as well, but that might take a bit longer to write.)

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8722468 [details] [diff] [review]
Patch v1.0

Review of attachment 8722468 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +2353,1 @@
>        value = losslessDecodeURI(uri);

I think I'd prefer if we'd try/catch losslessDecodeURI and if it throws use about:blank. And we don't need to losslessDecodeURI "about:blank", just set value to it, since it's the same.
That way the validity checks would stay in losslessDecodeURI itself.
The reason is mostly that if we add further exclusions/exceptions to losslessDecodeURI this will keep working.

Comment 6

[:Gijs (he/him)]

Attachment: Patch with test

Comment 7

[Abdulrahman Alqabandi]

Here is a PoC that does not require any manual tab switching.


<a href="javascript:var q=open('https://www.facebook.com/favicon.ico#q',123);`<script>setTimeout($=>{open('moz-action:foo',123)},111)<\/script>`">Click me</a>

Comment 8

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/730d0b594e31

Comment 9

[Daniel Veditz [:dveditz]]

I'm not seeing the value of this as a "url spoof" -- can you have any content other than the error page?

(In reply to :Gijs Kruitbosch from comment #0)
> This is a full URL spoof,

We should be a little clearer with this terminology since it appears in the severity levels referenced by our bounty program. An entire URL is present in the spoof, but that URL was not "fully spoofed". In particular the sec-high rating is looking for a spoof of a TLS page that includes the lock -AND- the correct site information if you click the lock (it's not necessary to spoof the cert details dialog if someone digs into page-info). 

In the example from comment 7 there is no lock, and if you click the info dialog you get "moz-action:foo Connection is not secure".

Comment 10

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> I'm not seeing the value of this as a "url spoof" -- can you have any
> content other than the error page?

Not as far as I can see / as reported. The URL bar text is confused and that's bad. But that's about it.

> (In reply to :Gijs Kruitbosch from comment #0)
> > This is a full URL spoof,
> 
> We should be a little clearer with this terminology since it appears in the
> severity levels referenced by our bounty program. An entire URL is present
> in the spoof, but that URL was not "fully spoofed". In particular the
> sec-high rating is looking for a spoof of a TLS page that includes the lock
> -AND- the correct site information if you click the lock (it's not necessary
> to spoof the cert details dialog if someone digs into page-info). 
> 
> In the example from comment 7 there is no lock, and if you click the info
> dialog you get "moz-action:foo Connection is not secure".

Right, sorry for being unclear by the use of the wrong terminology. I agree with the details you added.

Comment 11

[Abdulrahman Alqabandi]

(In reply to Daniel Veditz [:dveditz] from comment #9)
> I'm not seeing the value of this as a "url spoof" -- can you have any
> content other than the error page?
> 
> (In reply to :Gijs Kruitbosch from comment #0)
> > This is a full URL spoof,
> 
> We should be a little clearer with this terminology since it appears in the
> severity levels referenced by our bounty program. An entire URL is present
> in the spoof, but that URL was not "fully spoofed". In particular the
> sec-high rating is looking for a spoof of a TLS page that includes the lock
> -AND- the correct site information if you click the lock (it's not necessary
> to spoof the cert details dialog if someone digs into page-info). 
> 
> In the example from comment 7 there is no lock, and if you click the info
> dialog you get "moz-action:foo Connection is not secure".

I agree, I assumed it was 'sec-moderate' for the effort :P

Comment 12

[:Gijs (he/him)]

This was merged earlier today:

https://hg.mozilla.org/mozilla-central/rev/730d0b594e31