Closed Bug 1199601 Opened 7 years ago Closed 7 years ago

Make formatValue more resilient to spoofing attacks


(Firefox :: Address Bar, defect)

Not set



Firefox 47
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed


(Reporter: Gijs, Assigned: Gijs)



(Keywords: sec-want, Whiteboard: [adv-main45-][post-critsmash-triage])


(1 file, 1 obsolete file)

Spinning this off from bug 1195976 where the immediate need is addressed by the patch for the core bug that resulted in a broken URL being passed to the location bar.

However, we just had another report (bug 1199430) where one of the impacts of another non-Firefox-UI bug also includes the location bar mis-highlighting the domain.

I think it's worth spending some time to make that code less easy to fool, to reduce the impact of such bugs in the future, even if it is "only" spoofing / sec-low.
Group: firefox-core-security
Keywords: sec-want
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8673686 - Flags: review?(dao)
Assignee: nobody → gijskruitbosch+bugs
Comment on attachment 8673686 [details] [diff] [review]
Patch v1.1

Redirecting here from bug 1246330.
Attachment #8673686 - Flags: review?(dao) → review?(mak77)
Comment on attachment 8673686 [details] [diff] [review]
Patch v1.1

Review of attachment 8673686 [details] [diff] [review]:

please add tests, like "", "", "", "" to browser_urlHighlight.js
Those should be fine for the changes to the last group.

Though, you also want some test for the change to "(?:[^\/#?@]+@)?". I'm not completely sure what this is catching, the old regex seems to properly handle cases where the user:password part contains those chars, so I'd like to see examples of how that is preventing us to mismatch, first.
Attachment #8673686 - Flags: review?(mak77)
Blocks: 1246330
Attached patch Patch v1.2Splinter Review
Note that interestingly, one of your own examples,, without the 'http', does not currently highlight at all correctly on Nightly (and continues to not do so without the change to (?:[^\/#?]+@)? ) - though I got rid of the extra @ in there which does not seem to have been correct (sorry, it's been 4 months since I wrote the patch, so I just had to re-verify what I was doing). I added a number of other tests to verify we do the right thing here. Thanks for bringing that up!
Attachment #8673686 - Attachment is obsolete: true
Attachment #8718760 - Flags: review?(mak77)
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

Review of attachment 8718760 [details] [diff] [review]:

Attachment #8718760 - Flags: review?(mak77) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: it's possible for the wrong part of a URL to be highlighted in the URL bar
[Describe test coverage new/current, TreeHerder]: has tests!
[Risks and why]: pretty low, all we're doing is changing the regexp that deals with highlighting, for which there are pretty good unit tests already
[String/UUID change made/needed]: nope
Attachment #8718760 - Flags: approval-mozilla-beta?
Attachment #8718760 - Flags: approval-mozilla-aurora?
Group: firefox-core-security → core-security-release
Comment on attachment 8718760 [details] [diff] [review]
Patch v1.2

Ah, regexp :p Should not be too risky, taking it.

Should be in 45 beta 7
Attachment #8718760 - Flags: approval-mozilla-beta?
Attachment #8718760 - Flags: approval-mozilla-beta+
Attachment #8718760 - Flags: approval-mozilla-aurora?
Attachment #8718760 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main45-]
Whiteboard: [adv-main45-] → [adv-main45-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.