388372 - URL bar should ignore character directionality

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since bug 366797 made us show unescaped rather than escaped URLs in the URL bar, directional formatting characters and right-to-left charcaters can affect the presentation of the URL while it is being edited.  This seems to have a risk of spoofing bugs, although I haven't found any cases where the problem occurs in the non-editing view.

Steps to reproduce:
1. load one of these URLs:
 http://mozilla.org/htap?yreuq [note that path and query are labeled correctly]
 http://host.evil.com/gro.doog [note that this URL goes to evil.com ]
2. move the mouse over the URL bar and away again

Actual results: The presentation of the URL changes drastically when switching between the two URL bar modes.  When the host is highlighted (i.e., when the mouse is not over the URL bar and the URL bar is not focused), the directional formatting characters are ignored.  When the host is not highlighted, the directional formatting characters reorder the URL.

Expected results:  character directionality and directional formatting characters should not influence the display of what is loaded in the URL bar.  (Probably not even when the user is pasting a URL in the URL bar, which isn't a regression from bug 366797.)

Comment 1

[Jesse Ruderman]

Marking security-sensitive because I can reproduce the bug in Firefox 2: the second link makes it look like I'm at good.com.  (This surprised dbaron because he thought Firefox 2 escaped everything...)

Comment 2

[Simon Montagu :smontagu]

IMO (based on RFC 3987) we should always escape bidi formatting characters, so the expected results are http://mozilla.org/%E2%80%AEhtap?yreuq%E2%80%AC and http://%E2%80%AEhost.evil.com/gro.doog%E2%80%AC%

Comment 3

[Carsten Book [:Tomcat]]

on trunk i crash on loading this Bugzilla Page ! (not a testcae) 

Crash ID: 
http://crash-stats.mozilla.com/report/index/3583a94a-3c61-11dc-afb1-001a4bd43ed6?date=2007-07-27-16

Comment 4

[Reed Loden [:reed]]

So, loading the second example shows me a "Problem loading this page" message that states "Firefox can't find the server at .moc.live.tsoh". If I attempt to paste the URL in the second example in gedit or even in a textbox in Firefox, my cursor gets completely screwed up and shows as being both between the last '/' and the 'g' and after the 'h' (at the same time!). Also, my URL bar states "http://good.org/moc.live.tsoh" instead of the correct evil.com, but mousing over the URL bar does not change it. Also, when I reload this bug, the text for the second example is not "purple", meaning that Firefox thinks I never visited that page at all.

Loading the first example doesn't seem to do the same thing. I'm sent to http://www.mozilla.org/%e2%80%aehtap?yreuq, which is obviously a 404. Also, the link in the bug turns purple (already visited).

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6pre) Gecko/20070724 BonEcho/2.0.0.6pre ID:2007072403

Comment 5

[Reed Loden [:reed]]

Oh, and when I view source this page, everything in comment #0 starting at the first example until the </pre> is rtl instead of ltr. If I copy/paste that text into gedit, it shows up as ltr.

Comment 6

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 7

[Mike Connor [:mconnor]]

Hmm, on trunk right now I don't see an issue here yet, except maybe the statusbar text?

Comment 8

[Dão Gottwald [:dao]]

The statusbar actually gets it right, the Location Bar doesn't.

Comment 9

[Mike Connor [:mconnor]]

Ok, so, we should fix it then!

Comment 10

[Dão Gottwald [:dao]]

I'm willing to fix this if I can figure out how. It's not a regression though.

Comment 12

[Behnam Esfahbod [:zwnj]]

Attachment 290987 [details], the example I mentioned in bug 406322, has no DNS problem, and works fine if my DNS resolves ".ایران" tld.  It has just a LRM after the slash between host and path.

Comment 13

[Dão Gottwald [:dao]]

Attachment: trunk patch

Comment 14

[Dão Gottwald [:dao]]

Attachment: better trunk patch

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Shouldn't we be displaying them escaped, per comment 2, rather than removing theme entirely?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #2)
> IMO (based on RFC 3987) we should always escape bidi formatting characters, so
> the expected results are http://mozilla.org/%E2%80%AEhtap?yreuq%E2%80%AC and
> http://%E2%80%AEhost.evil.com/gro.doog%E2%80%AC%

The links in comment 0 don't include the trailing "%E2%80%AC%", so are those really the expected results?

If I tweak dao's patch to be:

+      // always encode bidirectional formatting characters (RFC 3987 section 4.1 paragraph 6)
+      value = value.replace(/[\u200e\u200f\u202a\u202b\u202c\u202d\u202e]/g, encodeURIComponent);

then I get http://mozilla.org/%E2%80%AEhtap?yreuq and http://%e2%80%aehost.evil.com/gro.doog .

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #15)
> Shouldn't we be displaying them escaped, per comment 2, rather than removing
> theme entirely?

The way I understand RFC 3987 ("IRIs MUST NOT contain bidirectional formatting characters [...] They affect the visual rendering of the IRI but do not appear themselves") and comment 0 ("directional formatting characters should not influence the display") is that we should just ignore them. That's also what the status bar does (because XUL lacks bidi support?).

Comment 18

[:Gavin Sharp [email: [e-mail]]]

I'm just concerned about stripping out information from the URL bar that's actually been sent in the request. Seems like it could be confusing to have two different pages loaded that have the same displayed URL.

Comment 19

[Behnam Esfahbod [:zwnj]]

Sorry if it's out-of-topic.  It's not a solution, but a way to don't let user get confised.

Can we limit the allowed characters of the input text-box of address-bar?  For example applying a simple character search on every change?

This helps user to understand she cannot input such characters there.

Comment 20

[Simon Montagu :smontagu]

(In reply to comment #16)
> The links in comment 0 don't include the trailing "%E2%80%AC%", so are those
> really the expected results?

That may just be an artefact of bugzilla's linkification. I assumed that they were intended as part of the URLs

Comment 21

[Simon Montagu :smontagu]

(In reply to comment #17)
> The way I understand RFC 3987 ("IRIs MUST NOT contain bidirectional formatting
> characters [...] They affect the visual rendering of the IRI but do not appear
> themselves") and comment 0 ("directional formatting characters should not
> influence the display") is that we should just ignore them. 

See section 3.2 of the RFC:

4. Re-percent-encode all octets produced in step 3 that in UTF-8
      represent characters that are not appropriate according to
      sections 2.2, 4.1, and 6.1.

Comment 22

[Dão Gottwald [:dao]]

Ok, thanks.

Comment 23

[Dão Gottwald [:dao]]

Attachment: encode bidi formatting characters, trunk version

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 297030 [details] [diff] [review]
encode bidi formatting characters, trunk version

encodeURIComponent seems like a better choice semantically (and perhaps more performant, though the difference is almost certainly negligible).

Comment 25

[Dão Gottwald [:dao]]

For the record, the patch needs an update anyway, so I'm waiting for bug 412458.

Comment 26

[Dão Gottwald [:dao]]

Attachment: encode bidi formatting characters, trunk version

updated and using encodeURIComponent

Comment 27

[Reed Loden [:reed]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.940; previous revision: 1.939
done

Comment 28

[:Gavin Sharp [email: [e-mail]]]

I filed bug 415491 about potentially moving this code into UnescapeURIForUI, at least for the status bar which already uses that function.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Uh..  Yeah.  This should have been done in UnescapeURIForUI, thus avoiding bug 460927 as well.

Is this when I get to mutter about sr in browser/?

Comment 30

[:Gavin Sharp [email: [e-mail]]]

I'd love to have a discussion (in some more appropriate forum) about the merits and drawbacks of requiring SR in /browser, but frankly it's rather annoying to see "SR would have caught this" comments any time you find a flaw in a patch that wasn't SRed.

In this case specifically, I don't think SR feedback would have significantly changed the outcome. We chose to take a specific, simpler fix and filed a bug on the more involved general fix, which I think is a net win over the situation we were in before.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(Oops, didn't notice bzbarsky wasn't CCed when I replied to his comment.)

Comment 32

[Boris Zbarsky [:bzbarsky]]

I'm not sure filing and forgetting security bugs is necessarily a win, especially since the bug that was filed wasn't even flagged as a security bug.  Once this one got opened anyone who cared could have figured out how to exploit other uses of UnescapeURIForUI...

Comment 33

[Boris Zbarsky [:bzbarsky]]

And by no means do I say this "any time I find a flaw".  I say it when I find specific "failure to really think about other parts of the code outside this little piece here too" flaws, which are what sr is meant to deal with.

I should note that there was no indication in the bug that the more general fix was even considered as a way of fixing this problem until a week after the patches landed....

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #32)
> I'm not sure filing and forgetting security bugs is necessarily a win,
> especially since the bug that was filed wasn't even flagged as a security bug.

Fair enough - "filing and forgetting" and not recognizing the security implications is obviously my fault. I don't think any of the other callers are quite as critical as the location bar is, though.

(In reply to comment #33)
> And by no means do I say this "any time I find a flaw".  I say it when I find
> specific "failure to really think about other parts of the code outside this
> little piece here too" flaws, which are what sr is meant to deal with.

You are absolutely justified in pointing out any cases of "failure[s] to really think about other parts of the code" by reviewers, but I don't really agree with the implication that SR is the only way to obtain that, and I'm not sure it's a fair characterization of what went on in this bug given that I did consider the other code, and even filed a bug about it.

I mostly commented because bug 395144 (where you made a similar comment, for a patch that actually was reviewed by an SR) was recent in my mind. 

> I should note that there was no indication in the bug that the more general fix
> was even considered as a way of fixing this problem until a week after the
> patches landed....

That's mostly because our URL bar doesn't use UnescapeURIForUI, so changing it wouldn't have actually fixed this bug. That's something else that we need to investigate...

Comment 35

[Boris Zbarsky [:bzbarsky]]

OK, point taken.  I should probably also make it clearer when I'm being tongue-in-cheek (which in comment 29) I actually was...

Comment 36

[Daniel Veditz [:dveditz]]

Is this a problem in SeaMonkey 1.1, too? If not we can definitely unhide this bug. If it is a problem, is it going to get fixed or could we open this bug anyway.

It's potentially even a problem on the 1.9-based SeaMonkey 2 since this bug and patch are in browser.js -- SeaMonkey uses different files which may or may not contain the same mistake.

Comment 37

[Robert Kaiser]

Neil should know better than me - is this a problem on SeaMonkey (trunk as well as 1.1)?

Comment 38

[[e-mail]]

(In reply to comment #36)
> Is this a problem in SeaMonkey 1.1, too?
It's potentially a problem according to bug 425280 comment #33 although I was only able to reproduce the second spoof.

> It's potentially even a problem on the 1.9-based SeaMonkey 2 since this bug and
> patch are in browser.js -- SeaMonkey uses different files which may or may not
> contain the same mistake.
Fortunately this patch had already landed before we ported bug 366797 to SeaMonkey 2 so we incorporated it in to our code.