Closed Bug 1250482 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [post-critsmash-triage][adv-main47-])

Attachments

(1 file, 1 obsolete file)

+++ 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.
Flags: needinfo?(dveditz)
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...
ah ok, I see the original tab url if I switch tabs again.
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #8722468 - Flags: review?(mak77)
(I'll write a test as well, but that might take a bit longer to write.)
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.
Attachment #8722468 - Flags: review?(mak77) → review+
Attached patch Patch with testSplinter Review
Attachment #8722468 - Attachment is obsolete: true
Attachment #8722483 - Flags: review?(mak77)
Attachment #8722483 - Flags: review?(mak77) → review+
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>
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".
Flags: needinfo?(dveditz)
Keywords: sec-moderatesec-low
(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.
(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
This was merged earlier today:

https://hg.mozilla.org/mozilla-central/rev/730d0b594e31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Group: firefox-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.