Last Comment Bug 802026 - (CVE-2013-0759) URL spoofing with credentials info of URL & 204
: URL spoofing with credentials info of URL & 204
: csectype-spoof, sec-high
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: 16 Branch
: All All
-- normal (vote)
: Firefox 20
Assigned To: :Gavin Sharp [email:]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: Marco Bonardo [::mak]
Depends on:
  Show dependency treegraph
Reported: 2012-10-16 00:12 PDT by Masato Kinugawa
Modified: 2014-07-24 14:38 PDT (History)
10 users (show)
rforbes: sec‑bounty+ in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

spoof_credentials_and_204.html (1.44 KB, text/html;charset=utf-8)
2012-10-16 00:12 PDT, Masato Kinugawa
no flags Details
patch (7.76 KB, patch)
2012-12-12 01:21 PST, :Gavin Sharp [email:]
dao+bmo: review+
dveditz: sec‑approval+
Details | Diff | Splinter Review
beta/aurora patch (7.02 KB, patch)
2012-12-13 17:33 PST, :Gavin Sharp [email:]
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑esr17+
Details | Diff | Splinter Review
esr10 patch (7.07 KB, patch)
2012-12-13 17:34 PST, :Gavin Sharp [email:]
bajaj.bhavana: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
updated ESR10 patch (6.02 KB, patch)
2012-12-15 15:39 PST, :Gavin Sharp [email:] approval‑mozilla‑esr10+
Details | Diff | Splinter Review
ESR17 patch (7.70 KB, patch)
2012-12-15 15:40 PST, :Gavin Sharp [email:] approval‑mozilla‑esr17+
Details | Diff | Splinter Review
followup fix for trunk/beta (1.08 KB, patch)
2012-12-15 15:42 PST, :Gavin Sharp [email:]
no flags Details | Diff | Splinter Review

Description User image Masato Kinugawa 2012-10-16 00:12:54 PDT
Created attachment 671741 [details]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

1. Go to attachment file and click "go" button.
2. Address bar points "". But in fact, it is not "".

Expected results:

Firefox should display URL correctly.
Comment 3 User image Daniel Veditz [:dveditz] 2012-10-17 10:40:21 PDT
We've had problems with navigation, 204's and the address bar in the past. Is the old bug back or is this new?
Comment 4 User image Mark Goodwin [:mgoodwin] 2012-10-17 12:52:15 PDT
(In reply to Daniel Veditz [:dveditz] from comment #3)
> We've had problems with navigation, 204's and the address bar in the past.
> Is the old bug back or is this new?

I don't know if the UI redress issue is new; I also don't know if the technique for suppressing the warning dialog is known.
Comment 5 User image Olli Pettay [:smaug] 2012-12-05 09:10:35 PST
So isn't this more like an UI issue. What I see is location bar which has ...lots of white space...
Comment 6 User image Olli Pettay [:smaug] 2012-12-05 09:11:48 PST
Daniel, you moved this from LocationBar component to DocumentNavigation. Any reason why?
Comment 7 User image :Gavin Sharp [email:] 2012-12-12 01:21:37 PST
Created attachment 691245 [details] [diff] [review]

There's a simpler way to reproduce:
1) Load "http://spoof   " (copy/paste from here, those are some fancy unicode spaces).
2) (Accept the warning)
3) Focus URL bar, press Esc.

Expected: URL stays the same
Actual: URL switches to the "spoofy" value

The underlying issue is that we're not calling createExposableURI when URLBarSetURI reverts the URL to the current URI. This happens in the case above, but also when finishing toolbar customization and, since bug 724599, when a load fails, as in this bug's testcase.

I searched the addons MXR for users of XULBrowserWindow._uriFixup - edilee's addon is the only one I found find:
Comment 8 User image :Gavin Sharp [email:] 2012-12-12 01:23:34 PST
Mardak: see the last part of comment 7. You'll need to stop using XULBrowserWindow._uriFixup in AwesomeBar HD. I recommend just retrieving the service  yourself rather than using a random Firefox property.
Comment 9 User image :Gavin Sharp [email:] 2012-12-12 12:12:33 PST
Comment on attachment 691245 [details] [diff] [review]

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Fairly easily, given the test. It's clear that we're touching spoofing-related functionality (display of the URL in the URL bar, stripping of the user/pass).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's clear what we're fixing, but probably not that clear how you would exploit this in practice.

Which older supported branches are affected by this flaw?


If not all supported branches, which bug introduced the flaw?


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch applies pretty much as-is to beta/aurora, very little uplift risk.

How likely is this patch to cause regressions; how much testing does it need?

I think it's unlikely to cause regressions - we're just being more consistent and applying existing URL bar display behavior to more cases. For beta/aurora, I can avoid the property removal that could potentially affect add-ons.
Comment 10 User image Al Billings [:abillings] 2012-12-12 14:10:31 PST
Adding Alex so we can discuss when we want to take this given that it affects branches and we're in the middle of the cycle.
Comment 11 User image Daniel Veditz [:dveditz] 2012-12-13 16:16:41 PST
Comment on attachment 691245 [details] [diff] [review]


For the branches--especially ESRs--we need a patch without the _uriFixup removal.
Comment 12 User image :Gavin Sharp [email:] 2012-12-13 17:33:26 PST
Created attachment 692103 [details] [diff] [review]
beta/aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: URL bar vulnerable to spoofing
Testing completed (on m-c, etc.): none yet
Risk to taking this patch (and alternatives if risky): relatively low risk. No add-on impact, and we're just making existing code used for displaying URLs in the URL bar apply more broadly.
String or UUID changes made by this patch: none
Comment 13 User image :Gavin Sharp [email:] 2012-12-13 17:34:08 PST
Created attachment 692106 [details] [diff] [review]
esr10 patch

[Approval Request Comment]
(see comment 12)
Comment 14 User image bhavana bajaj [:bajaj] 2012-12-14 16:06:43 PST
Comment on attachment 692103 [details] [diff] [review]
beta/aurora patch

Approving on branches as its a low risk patch for a sec high bug and unlikely to cause regressions.
Comment 15 User image :Gavin Sharp [email:] 2012-12-15 12:52:32 PST
Comment 16 User image :Gavin Sharp [email:] 2012-12-15 12:59:22 PST
Comment 17 User image :Gavin Sharp [email:] 2012-12-15 13:00:45 PST
Comment on attachment 692106 [details] [diff] [review]
esr10 patch

This patch is going to need an update, the test won't work as-is on ESR10.
Comment 18 User image :Gavin Sharp [email:] 2012-12-15 13:05:43 PST
Comment on attachment 692103 [details] [diff] [review]
beta/aurora patch

This patch will also need a test update for ESR17. Notes to self:
- whenNewWindowLoaded doesn't exist on the ESRs
- whenDelayedStartupFinished doesn't exist in the right head.js on ESR10
- openToolbarCustomizationUI/closeToolbarCustomizationUI don't exist on ESR10
- pageloaderror may not be a valid test on esr10 (since it's pre-bug 724599)
Comment 19 User image :Gavin Sharp [email:] 2012-12-15 15:22:08 PST
Followup to fix some test bustage:
Comment 20 User image :Gavin Sharp [email:] 2012-12-15 15:24:06 PST
Comment 21 User image :Gavin Sharp [email:] 2012-12-15 15:39:25 PST
Created attachment 692664 [details] [diff] [review]
updated ESR10 patch

I just removed the two non-working sub-tests ("customization" and "loading a new page)", leaving just the handleRevert test for ESR10.
Comment 22 User image :Gavin Sharp [email:] 2012-12-15 15:40:07 PST
Created attachment 692665 [details] [diff] [review]
ESR17 patch

For ESR17, I just added whenNewWindowLoaded to head.js.
Comment 23 User image :Gavin Sharp [email:] 2012-12-15 15:42:23 PST
Created attachment 692666 [details] [diff] [review]
followup fix for trunk/beta

This is the followup I landed in comment 19 to work around bug 822056 (which I didn't catch locally, because I was testing before per-window PB got turned on).
Comment 24 User image :Gavin Sharp [email:] 2012-12-15 15:43:09 PST
(In reply to :Gavin Sharp (use for email) from comment #23)
> Created attachment 692666 [details] [diff] [review]
> followup fix for trunk/beta

(I also rolled this in when I pushed the original patch to Aurora)
Comment 26 User image :Gavin Sharp [email:] 2012-12-15 15:59:17 PST
Sigh, the beta bustage was the same as the ESR bustage - whenNewWindowLoaded was not defined. I added it as with ESR-17:
Comment 27 User image Ryan VanderMeulen [:RyanVM] 2012-12-16 04:52:35 PST
This is still broken on Aurora.
Comment 28 User image :Gavin Sharp [email:] 2012-12-16 10:36:40 PST
Oops, somehow managed to forget to open an Aurora TBPL tab :( Too many branches.
Comment 31 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-04 11:53:16 PST
Confirmed reproducible with Firefox 16.0.

Verified fixed with:
* Firefox 20.0a1 2013-01-04
* Firefox 19.0a2 2013-01-04
* Firefox 18.0b7
* Firefox 10.0.12esr build1
* Firefox 17.0.1esrpre 2013-01-03

Is this something we can put in testsuite?
Comment 32 User image xu shaopei 2013-02-17 23:36:16 PST
Comment on attachment 671741 [details]

><!DOCTYPE html>
><meta charset="utf-8">
>function a(){
>'                                                                                                                                                                          ', 'x');
>	setTimeout(function(){
>'', 'x');
>	}, 500);
><input type="button" value="go" onclick="a()">

Note You need to log in before you can comment on or make changes to this bug.