Closed Bug 1247968 (CVE-2020-6808) Opened 5 years ago Closed 1 year ago

Full URL spoof using javascript URI scheme

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- verified
firefox75 --- verified

People

(Reporter: Gijs, Assigned: bzbarsky)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main74+])

Attachments

(4 files)

Attached file FF-URL-SPOOFv2.html
+++ This bug was initially created as a clone of Bug #1228754 +++

(original test and steps from bug 1228754 and @qab, not from me)

1. Open attached file.
2. click link
3. click back button in the browser


Somehow, the URL stays in the URL bar. Interrogating the browser's currentURI shows the URL shown in the URL bar, but the document displayed is actually about:blank (and the documentURI and so on verify this, and document.documentElement.nodePrincipal is the original opener's principal (localhost if you host the file there, bugzilla if you opened it directly ).

(In reply to Boris Zbarsky [:bz] from bug 1228754 comment #33)
> Sounds like it, but it also sounds like something different from the
> original issue reported in this bug (the one I analyzed in comment 6). 
> Seems like we should get a new bug filed on the interaction involving
> session history, and probably ask Olli whether he can take a look.

--> ni Olli.
Flags: needinfo?(bugs)
Group: firefox-core-security → core-security
Component: Location Bar → Document Navigation
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Version: 42 Branch → Trunk
I thought I had taken this.

Trying to get to this once I've done the current reviews.
Flags: needinfo?(bugs)
Group: core-security → dom-core-security
And I so thought I had assigned this to myself. At least I had added this to my todo list.
Assignee: nobody → bugs
Adding the bounty flag as requested by the reporter.

Lowering the severity to sec-moderate reflecting that
 1) the user has to hit back on the error page (unusual suspicion-raising circumstances)
 2) you can only spoof sites that don't actually have certs

#2 in particular means you can't spoof Facebook, Google, "my bank", etc. Spoofing is only useful if it can be used to fake a valuable target site for phishing, etc. attacks.
Flags: sec-bounty?
Keywords: sec-highsec-moderate
(In reply to Daniel Veditz [:dveditz] from comment #3)
> Adding the bounty flag as requested by the reporter.
> 
> Lowering the severity to sec-moderate reflecting that
>  1) the user has to hit back on the error page (unusual suspicion-raising
> circumstances)
>  2) you can only spoof sites that don't actually have certs
> 
> #2 in particular means you can't spoof Facebook, Google, "my bank", etc.
> Spoofing is only useful if it can be used to fake a valuable target site for
> phishing, etc. attacks.

You can spoof pages that contain in invalid character. So, for example, you can spoof a page that looks like 'https://www.facebook.com`' (note the invalid character), so its not just websites with no certs. In my opinion most users would not notice the invalid character at the end. Also I assume there is a special character that is less noticeable than '`' which could be used.
Any update on this security bug?
Flags: needinfo?(bugs)
Any updates here? This bug is lingering quite a bit.
Samael, do you think you'd have time to help here? Just say no if you don't have, and I'll try to
find time.
I've been slacking horribly with this one.
Flags: needinfo?(bugs) → needinfo?(sawang)
As the prerendering stuff are progressing so slowly, I don't think I can take this bug recently...
Flags: needinfo?(sawang)
ok, no problem.
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #10)
> ok, no problem.

Got time for this. I'll try to figure it out.
Assignee: bugs → sawang
One thing I'm noticing is that Chrome doens't put javascript: into session history. As an extremely simple test, I did:

location.href="javascript:'whatever'";

In chrome, the document does change, but URL bar, history.length and location.href remain the same. In firefox both URL bar and location.href changed to javascript:'whatever', and history length increases.

I don't see a clear reason to keep javascript: in session history, maybe we should change this behavior.

BTW I think that's also the reason I sometimes keep being redirected again when pressing back. Addons such as Wikiwand is using javascript: to redirect. I have impression that some websites also do.
Per spec, history.length should increase.  Otherwise you get user dataloss: the user can't go back to the page that used to be there!

Also per spec the URL bar and location.href should become the URL of the document triggering the javascript: load or so.  At some point we should really sit down and align this last bit with the spec...  Pretty sure we have existing bugs tracking that.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> Per spec, history.length should increase.  Otherwise you get user dataloss:
> the user can't go back to the page that used to be there!
> 
> Also per spec the URL bar and location.href should become the URL of the
> document triggering the javascript: load or so.  At some point we should
> really sit down and align this last bit with the spec...  Pretty sure we
> have existing bugs tracking that.

I believe you're referring bug 836567. I found the spec for document.URL stuff [1] and am still trying to better understand how spec describes session history behaviors.

It looks to me that the document.URL stuff is all we need for this bug. I'll see what I can do.

[1] https://html.spec.whatwg.org/multipage/browsers.html#javascript-protocol
Attached patch WIPSplinter Review
Hi Boris,

May I have your feedback and advices here? I'm trying to use the override URI as the URI for document, session history entry, and docshell's current URI, but I'm not quite confident about how it should be implemented. Does this look the right path to you?
Flags: needinfo?(bzbarsky)
If we want to take the "change the URI" approach, why wouldn't we just have the javascript: channel return the URI we want from GetURI, and presumably set the LOAD_REPLACE flag so people ignore the originalURI?
Flags: needinfo?(bzbarsky)
Or perhaps even more simply, change which URI we pass to the NS_NewInputStreamChannel call in nsJSChannel::Init, if we know which URI we'll really want at that point.
In fact, I expect that the GetURI the nsJSChannel returns doesn't matter; what matters is the stream channel's URI.
Thanks Boris!

I will commit patches on bug 836567 since it's more reasonable to deal with the document.URL issue there, and revisit this bug afterward. I'm expecting I'll be able to just invalid this bug then.
(In reply to Samael Wang [:freesamael] from comment #19)
> I will commit patches on bug 836567

Any timeline on that? Doesn't seem to be much activity over there.

> and revisit this bug afterward. I'm expecting I'll be able to just invalid this bug then.

No, don't do that! Fixing a bug elsewhere doesn't make a related bug INVALID. You could maybe use "worksforme" for an old bug that magically goes away, but if it's a security bug and you know what fixed it (as in this case, bug 836567 presumably) then you need to mark it FIXED.
Depends on: 836567
Flags: needinfo?(sawang)
Looks like the 'String.contains' function doesn't exist anymore. Make sure to replace 

>"location.toString().contains('http')" 

into 

>"location.toString().startsWith('http')"

At line 13 of the original PoC attached in Comment 0 for the PoC work again.
(In reply to Daniel Veditz [:dveditz] from comment #20)
> (In reply to Samael Wang [:freesamael] from comment #19)
> > I will commit patches on bug 836567
> 
> Any timeline on that? Doesn't seem to be much activity over there.
> 
> > and revisit this bug afterward. I'm expecting I'll be able to just invalid this bug then.
> 
> No, don't do that! Fixing a bug elsewhere doesn't make a related bug
> INVALID. You could maybe use "worksforme" for an old bug that magically goes
> away, but if it's a security bug and you know what fixed it (as in this
> case, bug 836567 presumably) then you need to mark it FIXED.

Sorry I wasn't actively working on this recently. Made some attempts these 2 days but according to bug 836567 comment 19 I should be holding it for a moment. And thanks for the suggestion on the bugzilla process which I admit I'm not quite familiar.
Flags: needinfo?(sawang)
Any update here? This bug is getting very old.
Flags: needinfo?(sawang)
I'll try to get back to this next week. Keep the ni? for reminding.
Updates will be on bug 836567
Flags: needinfo?(sawang)
Well, we need someone else to take on this for obvious reasons.
Assignee: freesamael → nobody
Since Boris took over bug 836567, I am ni? him here.
Flags: needinfo?(bzbarsky)
Flags: sec-bounty? → sec-bounty+

OK, bug 836567 is fixed now. Is there still an issue here? The original testcase in this bug doesn't even give me a "go back" option (if it's the first thing I load in the tab). If it's not, I can go back just fine and the URL updates correctly.

Flags: needinfo?(bzbarsky) → needinfo?(gijskruitbosch+bugs)

Yeah, this seems to be OK on nightly now.

Assignee: nobody → bzbarsky
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

Verified as fixed on Firefox 75.0a1 (2020-02-12) and on Firefox 74.0b1 on Windows 10 x64, Ubuntu 16.04 and Mac OS X 10.14.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main74+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.