Closed Bug 228176 Opened 21 years ago Closed 21 years ago

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

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.6final

People

(Reporter: asherman, Assigned: jag+mozilla)

References

(
URL
)

Details

(Keywords: fixed1.4.2, fixed1.6, Whiteboard: [sg:fix]security, spoof)

Attachments

(2 files, 2 obsolete files)

Testcase
587 bytes, application/x-zip-compressed
Details
update with the change mentioned in comment #51
6.02 KB, patch
darin.moz
: review+
jst
: superreview+
mkaply
: approval1.4.2+
Details | Diff | Splinter Review 
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
"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
But for me It's only http://www.microsoft.com!
That's the problem?
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+
My address bar displays the complete address, however the status bar does not.
Is this the desired behavior?
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!
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.
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
Closed: 21 years ago
Resolution: --- → DUPLICATE
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.
*** 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...
>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.
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]
Attached patch Proposed patch (obsolete) — — Splinter Review 
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.
Attachment #137275 - Flags: superreview?(darin)
Attachment #137275 - Flags: review?(dougt)
Comment on attachment 137275 [details] [diff] [review]
Proposed patch

i think some checks for the displayable ascii range would be better.
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. ***
Flags: blocking1.4.2?
Shouldn't this be OS-> All?
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
> 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. ***
Flags: blocking1.4.2? → blocking1.4.2+
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.
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.
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.
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|
*** Bug 228325 has been marked as a duplicate of this bug. ***
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
> 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.
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)
*** Bug 228540 has been marked as a duplicate of this bug. ***
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.
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.
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.
Attached file 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.
Attached patch alternative patch (obsolete) — — Splinter Review 
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 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);
Any thought/opinion?
Attachment #137585 - Attachment is obsolete: true
*** Bug 228936 has been marked as a duplicate of this bug. ***
+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.
*** Bug 229013 has been marked as a duplicate of this bug. ***
*** Bug 229300 has been marked as a duplicate of this bug. ***
*** Bug 229351 has been marked as a duplicate of this bug. ***
*** Bug 229799 has been marked as a duplicate of this bug. ***
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 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)
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 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+
it would be good to get this in on the 1.6 branch as soon as possible... thanks
regarding chofmann's comment as 'a', I landed the patch to both trunk and 1.6 branch
Status: NEW → RESOLVED
Closed: 21 years ago → 21 years ago
Flags: blocking1.6+
Resolution: --- → FIXED
Attachment #137693 - Flags: approval1.4.2?
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+
Should I check it into the 1.4 branch?
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
Keywords: fixed1.4.2
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
sorry for bug spam. I forgot to mention that I had replaced 'exclude' with
'skip' per darin's comment.
*** Bug 230467 has been marked as a duplicate of this bug. ***
*** Bug 230711 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla1.6final
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.
*** Bug 233100 has been marked as a duplicate of this bug. ***
Whiteboard: security, spoof → [sg:fix]security, spoof
Product: Core → Mozilla Application Suite
Blocks: 325274
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: