Closed Bug 724599 (CVE-2012-1950) Opened 8 years ago Closed 8 years ago

[CAL-2012-0001]Spoof after a URL is dragged to location bar, by canceling the load

Categories

(Firefox :: Address Bar, defect)

10 Branch
x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox10 --- wontfix
firefox11 --- wontfix
firefox12 --- wontfix
firefox13 + wontfix
firefox14 --- verified
firefox-esr10 14+ verified

People

(Reporter: curtisk, Assigned: dao)

References

(Depends on 1 open bug)

Details

(Whiteboard: [sg:moderate][advisory-tracking+])

Attachments

(5 files, 3 obsolete files)

[CAL-2012-0001]firefox drag spoof vulnerability


1 Affected Products
=================
tested Firefox 9.0.1
tested Firefox 10.0

2 Vulnerability Details
=====================
Code Audit Labs of vulnhunt.com had discovered a vulnerability in Firefox 10.0 which does not properly handle drag and drop operations on URL strings, which allows user-assisted remote attackers to spoof the Omnibox.

this issue is also affect chrome . we also reported to chrome ,they are addressing the vulnerability now.


3 Analysis
=========
N/A


4 Crash info:
===============
N/A


5 POC:
====
<title>Drag spoof testcase by Code Audit Labs of VulnHunt.com</title>
<body draggable="true">
<h3><a href="http://www.google.com" id="a">Drag me to address bar</a></h3>
<h3 id="note" style="display:none;">I am not google at all</h3>
</body>
<script>
ondragend = function(){
  stop();
  document.title = 'Done';
  a.style.display = 'none';
  note.style.display = 'block';
}
</script>



6 About Code Audit Labs:
=====================
Code Audit Labs secure your software,provide Professional include source
code audit and binary code audit service.
Code Audit Labs:" You create value for customer,We protect your value"
http://www.VulnHunt.com
Attached file Testcase as HTML
Attached image Screenshot
Screenshot. The basic problem is that the URL gets dragged, but the page immediately cancels the load (via window.stop) and the location still displays the "typed in" google.com URL.

I tend to think that this doesn't need to remain private.
Component: General → Location Bar
Product: Core → Firefox
QA Contact: general → location.bar
I think that this should be classified as [sg:moderate] because 1) you can't spoof the domain identifier/larry, only the the text box 2) it requires some level of user interaction
Whiteboard: [sg:moderate]
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #594980 - Flags: review?(gavin.sharp)
Attached patch useless test (obsolete) — Splinter Review
This doesn't fail without the patch. ondragend never got called for my synthetic drag.
Attachment #594983 - Flags: feedback?(enndeakin)
(In reply to Dão Gottwald [:dao] from comment #4)
> Created attachment 594980 [details] [diff] [review]
> patch

This prevents the dragend handler from firing, but a site can still just use an interval that calls stop() repeatedly to achieve the same effect, can't it? Being able to detect the drag end isn't really the problem here, IMO.

It would be nice if web pages couldn't interrupt URL bar-triggered loads by calling stop(). That would probably be complicated to fix, though.

An easier fix might be ensuring that the stop() reverts the URL bar.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> This prevents the dragend handler from firing, but a site can still just use
> an interval that calls stop() repeatedly to achieve the same effect, can't
> it?

I tried to modify the test case to do this but couldn't get it to work.
If it worked, we could presumably prevent it with LOAD_FLAGS_STOP_CONTENT (in addition to suppressing dragend).

> An easier fix might be ensuring that the stop() reverts the URL bar.

I couldn't find a decent way to differentiate between user- and script-initiated stops.
Blocks: 725611
Bug 725611 uses a different way of accomplishing the same thing via mousemove handlers and an onbeforeunload handler, so I think the only effective way to deal with this is going to be by changing the behavior on stop(), not dealing with the drag behavior.
http://gavinsharp.com/tmp/stopalot.html seems to work for me (essentially the same testcase with a setInterval(stop, 100) instead of dragend).

Why would we need to differentiate between the types of stops?

I can never remember how the userTypedClear/userTypedValue stuff actually works :(
Attachment #594980 - Flags: review?(gavin.sharp) → review-
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Why would we need to differentiate between the types of stops?

Because people may want to stop page loads without throwing away the URLs they just put there.
(In reply to Gavin Sharp from comment #9)
> I can never remember how the userTypedClear/userTypedValue stuff actually works :(

There's a nice comment in browser.xml that explains the minutiae, or did you want something more high-level?
(In reply to neil@parkwaycc.co.uk from comment #11)
> There's a nice comment in browser.xml that explains the minutiae, or did you
> want something more high-level?

Maybe. The comment talks about different scenarios, but it's hard to associate the various cases to the actual code that increments/decrements userTypedClear (including the code in tabbrowser).
(In reply to Dão Gottwald [:dao] from comment #10)
> Because people may want to stop page loads without throwing away the URLs
> they just put there.

I'm not sure that this matters much in practice, but it should be relatively easy to add a STOP_USER_TRIGGERED flag to nsIWebNavigation and pass it from BrowserStop, right? Not sure how to best propagate that to the progress listener, though (another state flag, too? that seems kind of wrong...).
I replaced isTabEmpty's empty-body check with an opener check, since the latter seems more accurate and safer and the former only happens to work because about:newtab doesn't have a body.
Attachment #594980 - Attachment is obsolete: true
Attachment #594983 - Attachment is obsolete: true
Attachment #594983 - Flags: feedback?(enndeakin)
Attachment #596981 - Flags: review?(gavin.sharp)
Blocks: 650992
[Triage Comment]
This isn't ready to land yet, we're committed to sg:{critical,high} and this is sg:moderate - why would we take this for esr10?  Please re-nominate if there's a good case for going outside the agreed upon process here. Thank you.
Comment on attachment 596981 [details] [diff] [review]
let stop() revert to the current URI

Can you add a comment that mentions why you're using isTabEmpty? Something like "if the current tab is empty, leave the typed URL (but allow it to change)". Perhaps also worth adding a comment explaining that setting userTypedValue resets userTypedClear.

(Really wish we could rename userTypedClear to "disallowURLBarUpdating" or somesuch, but the compatibility hit probably isn't worth it)
Attachment #596981 - Flags: review?(gavin.sharp) → review+
Shouldn't we be checking whether the request status was successful or failure?
(In reply to neil@parkwaycc.co.uk from comment #17)
> Shouldn't we be checking whether the request status was successful or
> failure?

Why? What case are you concerned about?
(In reply to Dão Gottwald from comment #18)
> (In reply to comment #17)
> > Shouldn't we be checking whether the request status was successful or
> > failure?
> Why? What case are you concerned about?
The case where the page refreshes automatically while you're trying to type a URL into the URL bar.
Attached patch patch v2Splinter Review
now checking the request status
Attachment #596981 - Attachment is obsolete: true
Attachment #596981 - Flags: feedback?(neil)
Attachment #610035 - Flags: feedback?(neil)
Comment on attachment 610035 [details] [diff] [review]
patch v2

>+                  if (!Components.isSuccessCode(aRequest.status) &&
>+                      !isTabEmpty(this.mTab)) {
>+                    // Restore the current document's location in case the
>+                    // request was stopped (possibly from a content script)
>+                    // before the location changed.
>+
>+                    this.mBrowser.userTypedValue = null;
f=me on this; from my point of view the rest of the patch is just padding.
Attachment #610035 - Flags: feedback?(neil) → feedback+
The "location bar reflects loaded page after stop()" test was failing, replacing aRequest.status with aStatus fixed it. I don't know what the former is, apparently it's not the same...
https://hg.mozilla.org/mozilla-central/rev/93f86e0dd442
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
No longer blocks: 724247
Duplicate of this bug: 724247
Cancelling a load group fires the OnStopRequest before cancelling the request, which explains why the request doesn't have the "right" status yet. I'm not sure what's normally expected, for instance it also seems to be possible to fire an OnStopRequest with different error status results.
Flags: in-testsuite+
[CC'ing Philip Chee so he can help SeaMonkey if we do need a port of this sec fix]
Verified fixed in trunk with nightly Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120330 Firefox/14.0a1.
Status: RESOLVED → VERIFIED
Summary: [CAL-2012-0001]firefox drag spoof vulnerability → [CAL-2012-0001]Spoof after a URL is dragged to location bar, by canceling the load
If this rates being fixed on the ESR branch we ought to take it in 13+, no? Moving tracking 14+ back to nomination.
Is there some reason we shouldn't land this in Firefox 13 (June) rather than Firefox 14 (mid-July)? Please request beta approval.
I am not inclined to land this in the late stages of beta given that it's sg:moderate and changes (crutfy) core browser UI progress listener code.
Are we tracking this for ESR 13+ still or should it be 14+?
This is 14+ at this point.
This is still marked Firefox 13 affected as well.
It's too late to land this for Firefox 13.
Dao: can you prepare an ESR patch (if required) and then request ESR approval?
Attached patch patch as landedSplinter Review
Attached patch esr10 patchSplinter Review
Attachment #640182 - Flags: approval-mozilla-esr10?
Do we have an original reporter beyond "Code Audit Labs" for this?
Whiteboard: [sg:moderate] → [sg:moderate][advisory-tracking+]
(In reply to Dão Gottwald [:dao] from comment #37)
> Created attachment 640182 [details] [diff] [review]
> esr10 patch

When will this patch be landing. Lukas? Alex?
Comment on attachment 640182 [details] [diff] [review]
esr10 patch

Good to land. Thanks Dao.
Attachment #640182 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Alias: CVE-2012-1950
Group: core-security
Sorry for being a bit late, but verified fixed in ESR using 2012-09-09 Firefox 10.0.8esrpre.
Depends on: 798249
You need to log in before you can comment on or make changes to this bug.