%00 in a URL causes incorrect display of hovered link in status bar

VERIFIED FIXED in mozilla1.6final

Status

SeaMonkey
UI Design
--
major
VERIFIED FIXED
14 years ago
11 years ago

People

(Reporter: Arkady Sherman, Assigned: jag (Peter Annema))

Tracking

({fixed1.4.2, fixed1.6})

Trunk
mozilla1.6final
fixed1.4.2, fixed1.6

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix]security, spoof, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031129
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031129

The mozilla is vulnerable of this bug!

Reproducible: Always

Steps to Reproduce:
1. Follow  
2. instruction
3. on the http://www.secunia.com/internet_explorer_address_bar_spoofing_test/

Actual Results:  
Address Bar Spoofing

Expected Results:  
The bug should be fixed

Comment 1

14 years ago
"If your address bar only says "http://www.microsoft.com" then your browser is
vulnerable."

It's not, I see
http://www.microsoft.com%01%00@secunia.com/internet_explorer_address_bar_spoofing_test/

Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
(Reporter)

Comment 2

14 years ago
But for me It's only http://www.microsoft.com!
That's the problem?
(Reporter)

Comment 3

14 years ago
The bug presents as well in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.6b) Gecko/20031207 Firebird/0.7+

Comment 4

14 years ago
My address bar displays the complete address, however the status bar does not.
Is this the desired behavior?
(Reporter)

Comment 5

14 years ago
The same is for me. But the bug is I belive being unable to determine where does
the link refere to! So, with http://www.microsoft.com being seen in the status
bar, I gas that if I click that I will go to http://www.microsoft.com, but not
to another site!

Comment 6

14 years ago
The Internet Explorer bug is about the addressbar (= location bar), which is the
one at the top. Not at the bottom (= status bar), which is easy enough to fake
with JavaScript. Mozilla and Firebird even have a special preference to prevent
that. 

We have somewhere a bug about the display of control characters in the
statusbar. It's the %00 that cuts of the rest of the URL.

Comment 7

14 years ago
See also bug 122445 for another way to present fake information in the location
bar. That will also fix this bug. The %01%00 itself doesn't present a problem
for Mozilla, because we still show the entire URL.

*** This bug has been marked as a duplicate of 122445 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE
(Reporter)

Comment 8

14 years ago
But why there is not the same with status bar. I've got JavaScript switched off
for changing the status bar and used to trust what it shows.

Comment 9

14 years ago
*** Bug 228185 has been marked as a duplicate of this bug. ***
I think this bug should stay open to cover the fact that %00 in a URL causes the
statusbar display when hovering over a link to be incorrect.  (That is somewhat
serious when Javascript is turned off or when the ability of sites to change the
status bar is disabled, since in those cases the status bar ought to be able to
be trusted.)
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
I'm not quite sure where this bug should go, though.
Assignee: general → jag
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Apps
Ever confirmed: true
QA Contact: general → pawyskoczka
Summary: Address Bar Spoofing → %00 in a URL causes incorrect display of hovered link in status bar
If the XPIDL interface doesn't take a counted string type, isn't nul termination
doing to decide the string length when crossing an XPConnect boundary?

/be
The method in question seems to be nsIXULBrowserWindow::setOverLink.

The unescaping happens at the C++ caller's caller:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsWebShell.cpp&rev=1.622&mark=709#687
bz: do you think the nsWebShell.cpp code should unescape, but special-case
embedded NULs?

/be
maybe UnEscapeURIForUI should do the special-casing of %00, given that it's
intended for ui...

Comment 16

14 years ago
>maybe UnEscapeURIForUI should do the special-casing of %00, given that it's
>intended for ui...

yes, this should be a MUST.  actually, it should not unescape any control
characters.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031210 Firebird/0.7+
(daihard; XFT+GTK2; optimized for P4/SSE-2)

I get a strange boxed unprintable character in the status bar that chops the
URL, but the address bar shows the full URL.
Flags: blocking1.6+

Comment 18

14 years ago
IMHO the easiest way to fix this bug is to a) call UnEscapeNonAsciiURI instead
and b) fix NS_UnescapeURL to accept the flag that UnEscapeNonAsciiURI uses.

A more comprehensive fix would unescape all characters except when the result is
[\x00-\x20#$%&+,./:;=?@\x7F]

Comment 19

14 years ago
Created attachment 137275 [details] [diff] [review]
Proposed patch

This fixes the link in question. It does mean that some ascii characters will
no longer be unescaped, though I'm not sure if that's a big a problem.

Updated

14 years ago
Attachment #137275 - Flags: superreview?(darin)
Attachment #137275 - Flags: review?(dougt)

Comment 20

14 years ago
Comment on attachment 137275 [details] [diff] [review]
Proposed patch

i think some checks for the displayable ascii range would be better.

Comment 21

14 years ago
plus, what about all the other places where UnEscapeURIForUI is called?  don't
we need to fix the behavior of that function instead of trying to circumvent it
only for nsWebShell?
*** Bug 228211 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Flags: blocking1.4.2?
Shouldn't this be OS-> All?

Comment 24

14 years ago
Hardware, OS -> All as requested
OS: Windows 2000 → All
Hardware: PC → All
Actually, why do we want to unescape the URI at all?

Ideally, we want to show the URI that would (a) appear in the URI bar and, more 
importantly, (b) be the URI sent to the server. I've often noticed that when I 
hover over a data: URI, the status bar shows all kinds of junk, when it should 
just be showing the URI as escaped.


> A more comprehensive fix would unescape all characters except when the result 
> is [\x00-\x20#$%&+,./:;=?@\x7F]

That might work, except what encoding do you assume for the high-bit characters?
QA Contact: pawyskoczka → ian

Comment 26

14 years ago
> Actually, why do we want to unescape the URI at all?

See bug 81024 for example.

> what encoding do you assume for the high-bit characters?

We currently assume the document character set...
>> Actually, why do we want to unescape the URI at all?
> See bug 81024 for example.

That's about the hostname; should we just unescape that part's high-bit 
characters? I think we can safely always assume it's UTF-8 if we do just the 
host part (is that right?).


>> what encoding do you assume for the high-bit characters?
> We currently assume the document character set...

That can be trivially shown to not work, especially for, e.g., data: URIs.
*** Bug 228252 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Flags: blocking1.4.2? → blocking1.4.2+

Comment 29

14 years ago
Ian: for numerous non-ASCII "speaking" users, having the browser automatically
decode escaped-URLs is very helpful.  of course, unescaping must be done very
cautiously because there are many times when an unescaped byte might be
misinterpreted in the wrong charset.  clearly, it seems like data: URLs should
be excluded from those that we try to unescape.  i think the code in intl needs
to be beefed up to make limited choices about what it unescapes.
> That's about the hostname;

The filename has the same issue.  So do various directory names in URIs, etc.

> I think we can safely always assume it's UTF-8 if we do just the
> host part (is that right?).

It's not.

It's not clear to me why we have both UnEscapeURIForUI and UnEscapeNonASCIIURI,
and I won't really have time for the relevant CVS archeology in the near future,
but any changes to the way those functions are used should really look up why
they were added.  In the end, I suspect, we don't need both of those functions;
we just need UnEscapeURIForUI to be sane.

It's not clear to me why no one bothered to cc some intl people, who would
perhaps even know the answers to some of the questions raised...
Oh, and Ian, in the intl cases the URI sent to the server is URI-escaped
garbage.  Showing it the user is just not cool.

Comment 32

14 years ago
Also consider related bug 184074
> Ian: for numerous non-ASCII "speaking" users, having the browser automatically
> decode escaped-URLs is very helpful.

If you say so. Personally, as a non-ASCII "speaking" user I find I prefer to
know what the URI actually is before it is sent over the wire, but I admit I'm
not a typical user.


> [...]

Fair enough. I retract my objections; I think bz is right that we should try to
factor out our various attempts at this into a single "unescape for UI" function
that we use for when a URI is to be displayed in the UI (except for the location
bar, where I _really_ think we should show the real URI).
> Ian: for numerous non-ASCII "speaking" users, having the browser automatically
> decode escaped-URLs is very helpful.

Careful here: we also want to avoid spoofing by the use of similar-looking
characters. Being able to see the URI as it will be transmitted over the wire as
well as in decoded form is a security requirement IMO.

Comment 35

14 years ago
Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.6b) Gecko/20031208 MultiZilla/1.6.0.0b)
I'm a fairly knowledgeable, long time (and very faithful) user of Mozilla, and
even though I have disabled changing status bar text, I could easily get fooled
by this.  I'd probably rely more on the status bar pre-click than the address
bar post-click for checking whether the site URL displayed in the link is bogus,
and in my implementation of Mozilla, the staus bar does only display the bogus
site address when using the Secunia test page.

My choice would be, for these tricky URLs that aren't easily parsed by eye
(especially for the newbies we are trying to lure to Mozilla), is one of the
notifications suggested in Bug 122445.  At a minimum, I'd like to see the one in
Comment #93, which displays 'http://www.server.com/path [login=user,
password=pass]' -- thus making it clear there's more than meets the eye of the
casual observer.  I like even better the suggestion in 122445's Comment #84,
which, when a tricky link is clicked on, cautions the user about what is really
going on, and offers a chance to bail out.  I'd suggest making this kind of
detailed warning a security preferance, enabled by default (since we want
canverts from IE to not feel like they're still in a minefield).  Mozilla
doesn't break under malware like Internet Explorer, and I think we should strive
to make Mozilla the safest tool a user can find.

Comment 36

14 years ago
re comment #30
|UnEscapeNonASCIIURI| was added in bug 161479 (bz, you're there :-)) and the
only place it's used is nsJSProtocolHandler
(http://lxr.mozilla.org/seamonkey/source/dom/src/jsurl/nsJSProtocolHandler.cpp#705).
However, it's not clear why it's necessary. BTW,  |EnsureUTF8Spec| in the file
can be replaced by |nsIUTF8ConverterService|

Comment 37

14 years ago
*** Bug 228325 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Whiteboard: security, spoof
Another problem with Mozilla is, that you can´t see any difference between g and
q in the status bar. For example http://www.google.de and http://www.qooqle.de
look exactly the same in the status bar.

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031208
with Modern-Theme.

I don´t know if this is already a known bug, but it certainly can fool people
like the bug described above.

Bruno
Addition to Comment #38: I just encountered, that the problem can only be seen,
if Mozilla is maximized but not, if the window isn´t maximized.

Can anyone else see this behaviour or is it perhaps a problem with my graphics
chip (SIS 630)?

Bruno

Comment 40

14 years ago
> Another problem with Mozilla is, that you can´t see any difference between g and
> q in the status bar.

I can tell the difference but, in any case, it's a separate issue and not
related to this bug.
*** Bug 228324 has been marked as a duplicate of this bug. ***
I checked this bug with Mozilla 1.5 [ozilla/5.0 (X11; U; OpenVMS
COMPAQ_Professional_Workstation; en-US; rv:1.5) Gecko/20030929]
and cannot the reported result. I see %00 (and %01) followed by the rest of the URL.

Anyway, this bug illustrates once again why the entire "encoding" business (see
also MIME) is just a stupid misconception.

Comment 43

14 years ago
About this http://www.google.de and http://www.qooqle.de stuff mentioned in
Comment #38: Have you opened a bug where we can discuss this, since it sounds
like a font/font-size problem? Have you changed your fontsizes? I cannot
reproduce it with my setup on Linux.

(Sorry 'bout the spam)

Comment 44

14 years ago
Hit the news: <http://www.heise.de/newsticker/data/dab-15.12.03-001/> (German)

Comment 45

14 years ago
*** Bug 228540 has been marked as a duplicate of this bug. ***

Comment 46

14 years ago
Just a quick hint: you can use the userContent.css to notice these SpoofUrls.
Just add the following to the userContent.css in your Chrome dir:

:link[href*="%00@"], :visited[href*="%00@"],:link[href*="%01@"],
:visited[href*="%01@"] {
   cursor: help;
}

This changes the cursor to a questionmark over a link with %00@ in it.
You can also use this to recolor the link or even not to display it.

Comment 47

14 years ago
I'd vote for not unescaping the control characters 0x00-0x20, but at least 0x00
for 1.6final.

Digging deep in the source history, unifying unescape functions and thinking
about doing the right thing is good. But making Mozilla the next release version
not spoofable through the known "exploits" is IMO a first necessary step.

Comment 48

14 years ago
wondering if the patch we have now can be considered good enough for round one
(and release in 1.6)....  

looks like we may need more work on this going forward..  or is it the case that
the patch might send us in the wrong direction.

Comment 49

14 years ago
Created attachment 137564 [details]
Testcase 

Null bytes in URLs may also be an issue for this bug and related bugs (bug
122445, bug 225845). This "binary" HTML file contains some variants of the URL
from comment 1 with embedded 0x00/0x01.

Note the different behavior of Mozilla (1.6b) and IE (6.0):
IE opens the same URL on all links.
Mozilla opens http://www.microsoft.com/ on links 4 and 8 and tries to open
http://www.microsoft.com%01 on link 7.

Re comment 46: THX for the hint. Using something like 
  :link[href*="%00"], :visited[href*="%00"],
  :link[href*="%01"], :visited[href*="%01"],
  :link[href*="^A"], :visited[href*="^A"] { ... }
will detect all the links except link 4. (^A = binary 0x01).
Binary 0x00 does not work.

Comment 50

14 years ago
Created attachment 137585 [details] [diff] [review]
alternative patch

I added 'esc_ExcludeControl' (that is only meaningful for unescaping) and
modified nsISubTextURI::UnEscapeURIForUI call NS_UnescapeURL with that flag
set. 
With the flag set, C0 characters and DEL (0x7F) are excluded from unescaping. 
I also changed the meaning of esc_OnlyNonAscii. When escaping, it skips only
graphic ASCII characters (that is, 0x20 - 0x7E). In the other direction, it
skips (excludes from unescaping) all ASCII characters (as it used to).

Comment 51

14 years ago
Comment on attachment 137585 [details] [diff] [review]
alternative patch


>--- intl/uconv/src/nsTextToSubURI.cpp	20 Jul 2003 07:46:34 -0000	1.27

> NS_IMETHODIMP  nsTextToSubURI::UnEscapeURIForUI(const nsACString & aCharset, 

>+  nsCAutoString unescapedSpec;
>+  // exclude control octets (0x00 - 0x1f and 0x7f) from unescaping
>+  NS_UnescapeURL(PromiseFlatCString(aURIFragment), esc_ExcludeControl, unescapedSpec);
>   return convertURItoUnicode(PromiseFlatCString(aCharset), unescapedSpec, PR_TRUE, _retval);

I should have added esc_AlwaysCopy when calling NS_UnescapeURL or 
have to use the following:

PRBool changed = NS_UnescapeURL(PromiseFlatCString(aURIFragment),
esc_ExcludeControl, unescapedSpec);

return convertURItoUnicode(PromiseFlatCString(aCharset), changed ?
unescapedSpec : aURIFragment, PR_TRUE, _retval);

Comment 52

14 years ago
Created attachment 137693 [details] [diff] [review]
update with the change mentioned in comment #51

Any thought/opinion?
Attachment #137585 - Attachment is obsolete: true
*** Bug 228936 has been marked as a duplicate of this bug. ***

Comment 54

14 years ago
+cc andreas:

The "status bar should display URL's unescaped" design is possibly attributable
to him. I think in the past, I have liked the user-friendliness, but had
concerns about the larger impact, since I never had time to do a full set of
character set testing.

Comment 55

14 years ago
*** Bug 229013 has been marked as a duplicate of this bug. ***
*** Bug 229300 has been marked as a duplicate of this bug. ***

Comment 57

14 years ago
*** Bug 229351 has been marked as a duplicate of this bug. ***

Comment 58

14 years ago
*** Bug 229799 has been marked as a duplicate of this bug. ***

Comment 59

14 years ago
What do you want to do with this bug? It has to be fixed in 1.6final, IMHO and I
rather like my patch (attachment 137963 [details]) :-)
jshin: you mean attachment 137693 [details] [diff] [review]?  Please solicit module owner or peer review,
and super-review if needed, then go for a= for 1.6.

/be

Comment 61

14 years ago
Comment on attachment 137693 [details] [diff] [review]
update with the change mentioned in comment #51

asking for r/sr.

I've been waiting for some opinions because there's another patch. Let's ask
reviewers to decide :-)
Attachment #137693 - Flags: superreview?(darin)
Attachment #137693 - Flags: review?(dougt)

Updated

14 years ago
Attachment #137693 - Flags: superreview?(darin)
Attachment #137693 - Flags: superreview+
Attachment #137693 - Flags: review?(dougt)
Attachment #137693 - Flags: review?(darin)
Comment on attachment 137693 [details] [diff] [review]
update with the change mentioned in comment #51

+	     if (ISHEX(*p1) && ISHEX(*p2) && !(ignoreNonAscii && *p1 >= '8') &&
+		 !(excludeControl && 
+		   (*p1 < '2'|| (*p1 == '7' && (*p2 == 'f' || *p2 == 'F'))))) {

Add a space between '2' and ||.

sr=jst

Comment 63

14 years ago
Comment on attachment 137693 [details] [diff] [review]
update with the change mentioned in comment #51

Index: xpcom/io/nsEscape.h

>+  esc_ExcludeControl = PR_BIT(15)  /* excludes C0 and DEL from unescaping */

can you call this esc_SkipControl instead?  "skip" is shorter and
just as clear i think.

r=darin
Attachment #137693 - Flags: review?(darin) → review+

Comment 64

14 years ago
it would be good to get this in on the 1.6 branch as soon as possible... thanks

Comment 65

14 years ago
regarding chofmann's comment as 'a', I landed the patch to both trunk and 1.6 branch
Status: NEW → RESOLVED
Last Resolved: 14 years ago14 years ago
Flags: blocking1.6+
Resolution: --- → FIXED

Updated

14 years ago
Attachment #137693 - Flags: approval1.4.2?

Comment 66

14 years ago
Comment on attachment 137693 [details] [diff] [review]
update with the change mentioned in comment #51

a=mkaply for 1.4.2
Attachment #137693 - Flags: approval1.4.2? → approval1.4.2+

Comment 67

14 years ago
Should I check it into the 1.4 branch?

Comment 68

14 years ago
I'd just like to confirm that last night's Linux build resolved the problem,
very nicely. Good job! 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040106

Updated

14 years ago
Keywords: fixed1.4.2

Comment 69

14 years ago
As mike_jk noticed, it went into to 1.4 branch as well (I don't remember asking
for a1.4 :-), but thanks for a1.4). A slightly different patch was necessary for
nsTextSubURI.
Flags: blocking1.4.2+
Keywords: fixed1.6

Comment 70

14 years ago
sorry for bug spam. I forgot to mention that I had replaced 'exclude' with
'skip' per darin's comment.

Comment 71

14 years ago
*** Bug 230467 has been marked as a duplicate of this bug. ***

Comment 72

14 years ago
*** Bug 230711 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla1.6final

Updated

14 years ago
Attachment #137275 - Attachment is obsolete: true
Attachment #137275 - Flags: superreview?(darin)
Attachment #137275 - Flags: review?(dougt)
I believe the fix checked in for this has caused the regression mentioned in bug
232204.

Comment 74

13 years ago
*** Bug 233100 has been marked as a duplicate of this bug. ***
Whiteboard: security, spoof → [sg:fix]security, spoof
http://www.secunia.com/internet_explorer_address_bar_spoofing_test/

http://www%2Emicrosoft%2Ecom%01%00@secunia.com/internet_explorer_address_bar_spoofing_test/

Verified FIXED using build 2004-09-26-05 on Windows XP, Seamonkey trunk.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

11 years ago
Blocks: 325274
You need to log in before you can comment on or make changes to this bug.