Last Comment Bug 161357 - Buffer overflow with external protocol handler
: Buffer overflow with external protocol handler
Status: VERIFIED FIXED
[sg:blocker]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: ---
Assigned To: Mitchell Stoltz (not reading bugmail)
: bsharma
Mentors:
Depends on:
Blocks: 253311 blackbird 1.2
  Show dependency treegraph
 
Reported: 2002-08-06 13:58 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2005-08-23 14:08 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
propsed patch (330 bytes, patch)
2002-10-24 10:29 PDT, georgi - hopefully not receiving bugspam
no flags Details | Diff | Splinter Review
proposed patch with correct diff(1) option (931 bytes, patch)
2002-10-24 10:58 PDT, georgi - hopefully not receiving bugspam
dveditz: superreview+
Details | Diff | Splinter Review
testcase (36.57 KB, text/html)
2002-11-02 00:21 PST, Daniel Veditz [:dveditz]
no flags Details
another patch (895 bytes, patch)
2002-11-05 13:37 PST, Daniel Veditz [:dveditz]
security-bugs: review+
darin.moz: superreview+
chofmann: approval+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2002-08-06 13:58:34 PDT
There is a buffer overflow in both Mozilla and the AOL Gecko client
involving helping applications on Windows.
The following:
<a href="telnet://{4000chars}">click me</a>
crashes mozilla and seems exploitable because EIP=00670067
0x67='g' and 00 seems because of Unicode.
Have not tested it personally but it is believed that Unicode buffer
overflows are exploitable.
Don't know whether this is mozilla's or windows's fault, but the attack
vector is mozilla, so this should be fixed.
Saw a similar public bug on bugzilla which described a similar problem but
with mail application and mailto: links.
Attached is a perl cgi.
Comment 1 georgi - hopefully not receiving bugspam 2002-08-07 02:36:42 PDT
AFAIK on Linux "telnet" and "rlogin" protocols don't work despite some
mentioniong of "xterm -e ..." in some js preference?
Comment 2 georgi - hopefully not receiving bugspam 2002-08-07 02:52:36 PDT
I think it is a good idea mozilla not to open any microsoft applications
(including telnet and hyperterminal) unless the user explicitly agrees.
At least by default.
In the past there was an exploit in which telnet url opened hyperterminal and
because of bug in hyperterminal code could be executed. 
In case another similar bug arise, the users may perceive it is mozilla's fault.
Comment 3 Mitchell Stoltz (not reading bugmail) 2002-10-17 17:30:22 PDT
Georgi: do you think this is a bug in Mozilla, or in the telnet client? Looks
like it might be Mozilla.
Comment 4 georgi - hopefully not receiving bugspam 2002-10-18 02:25:05 PDT
I think it is not strictly in telnet - IIRC there was a similar bug with mailto:
and external mailer and this works with both telnet and hyperterminal.
It is another question whether this is in Mozilla or in some windows library.
IIRC the stack trace mentioned some windows library, so I don't exclude the
possibility for a fault in a non-Mozilla library.
Shall check the stack soon.
It is interesting that IIRC IE does not allow long external URLs at all -
silently ignores them.

Is it difficult hardcoding a limit for the length of external URLs?

Comment 5 Mitchell Stoltz (not reading bugmail) 2002-10-18 15:58:32 PDT
What limit would we set? Pretty much anything we choose would be arbitrary. If
we truncate at 3000 chars, how do we know there isn't a buffer overflow at 2999
chars in some external app?
Comment 6 georgi - hopefully not receiving bugspam 2002-10-19 02:31:49 PDT
Right now I am having problems with the lame windoze box, so can't check the
stack, shall check soon.

Mitchell, you are mixing different problems.
This bug refers to a generic problem with spawning external apps (according to
bugzilla this happens with mailto: external apps also, not only telnet).

The crash is in Mozilla or in a mozilla/windows library and not in the external
app - if it were in the external app, the external app would have crashed and
mozilla keep running.

In case someone want to check the actual place of the crash, the following
procedure may do:

Using the procedure for reproducing the bug, start with a small buffer which
don't overflow. 
Then find the smallest value for which the crash happens (binary search will
do). At this point the stack is not completely smashed, so it can be examined.
If this does not work, slightly increment the overflow length. Large values of
buffer completely smash the stack IIRC.

It is another story to protect from bugs in external applications.
As I have stated in other bugs, I suggest an option which disables all external
apps. Or a whitelist for external apps.
Comment 7 georgi - hopefully not receiving bugspam 2002-10-21 04:46:39 PDT
Investigated the overflow using the procedure in comment #6.
The earliest crash is somewhere in shell32, but don't have windoze debugging
symbols, so can't tell exactly where it is.

My educated bet for the problem is:
uriloader/exthandler/win/nsOSHelperAppService.cpp" line 227 of 406
LONG r = (LONG) ::ShellExecute( NULL, "open", urlSpec.get(), NULL, NULL,
SW_SHOWNORMAL);

ShellExecute crashes badly when invoked with long third parameter from a test app.

My suggestion is to test the following:
Dont't pass usSpec.get() directly, but copy it to a temp variable.
If it is longer than a hardcoded limit (probably 1900 bytes), then forcefully
null terminate it at a safe position, or just exit.

Hope this helps.

Comment 8 georgi - hopefully not receiving bugspam 2002-10-24 10:29:11 PDT
Created attachment 103989 [details] [diff] [review]
propsed patch
Comment 9 georgi - hopefully not receiving bugspam 2002-10-24 10:32:48 PDT
The above attachement fixes the problem for me on windoze.
Probably other references to ShellExecute should be also wrapped this way.
Seems the buffer value which I have chosen does not overflow the stack, may be
checked manually with a test app which executes ShellExecute and then examine
the stack.
Comment 10 georgi - hopefully not receiving bugspam 2002-10-24 10:58:14 PDT
Created attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option
Comment 11 Christopher Blizzard (:blizzard) 2002-10-25 10:03:43 PDT
Anyone want to review this?  It's a 1.2 blocker.
Comment 12 Daniel Veditz [:dveditz] 2002-10-25 12:03:43 PDT
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

sr=dveditz
Comment 13 Doug Turner (:dougt) 2002-10-25 15:52:38 PDT
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

The patch does not apply using my version of patch for whatever reason.

There is a tab in front of "char tmp".

What is so special about 1500.	Lets remove the define, use the value directly,
and comment why this size is needed in the code.  Furthermore, I would have
thought MAX_PATH would have been enough room here.  Why 1500?

lets clean up this patch, explain why 1500, and get it in for 1.2final.
Comment 14 Mitchell Stoltz (not reading bugmail) 2002-10-25 15:59:39 PDT
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

r=mstoltz. I'll try to get this checked in.
Comment 15 Mitchell Stoltz (not reading bugmail) 2002-10-25 16:02:29 PDT
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

Whoops, missed last comment. review pending a response to Doug's questions.
I'll handle the tab.
Comment 16 rpotts (gone) 2002-10-25 16:39:12 PDT
I'm with dougt... It seems like MAX_PATH is a more appropriate value...

Also, i believe we could just call:

  urlSpec.SetLength(MAX_PATH);

rather than creating another temporary copy of the string...

- -rick
Comment 17 georgi - hopefully not receiving bugspam 2002-10-26 02:18:16 PDT
For comments #13 and #16:

Some research showed that the overflow seems in ShellExecute - calling it from a
test app with argument "telnet://{2080 chars}" produces a crash.
IIRC 2080 is the lowest value in which the crash occurs. (Note: values lower
than 2080 may not crash, but may corrupt the stack in potentially exploitable way).
So the value 1500 was chosen empirically to terminate the string and not corrupt
the stack via ShellExecute.

Before chosing MAX_PATH, please verify that it solves the problem as described
in the description of the bug.
Comment 18 Mitchell Stoltz (not reading bugmail) 2002-10-30 15:01:16 PST
Georgi,
   if you can, please try to generate a stack trace of the crash.
Comment 19 Michael Buckland 2002-10-30 17:47:55 PST
Discussed in bBird team meeting.  Plussing for adt only.
Comment 20 georgi - hopefully not receiving bugspam 2002-10-31 02:16:17 PST
I am unable to produce a meaningfull stack trace using a CVS build on
windows.
The stacks I get:
1. illegal address at 00670067 (depending on the overflow, the stack is
totally corrupt and overwritten). -or-
2. With marginal buffers I land somewhere in SHELL32.DLL but don't know where
because don't have system windows debugging symbols - get just several
addresses in SHELL32.
Mozilla functions in this case are not present in the stack.

In case someone is interested, I may send the stack from (2) after I
reinstall windows (now reinstalling it).

Once again, almost sure this is not mozilla fault, because:
1. The proposed patch by me fixes it.
2. A standalone application which calls
ShellExecute(...,"telnet://{4000chars}",...) crashes.
Comment 21 Asa Dotzler [:asa] 2002-11-01 11:18:44 PST
So this needs a new patch? drivers want this for 1.2 (real soon now). What's the
status?
Comment 22 Mitchell Stoltz (not reading bugmail) 2002-11-01 19:02:08 PST
doug, rick, darin,
    Do you think we should go with Georgi's recommended limit of 1500? Or
MAX_PATH, or the smaller of the two? Would imposing such a limit be too restrictive?

I can't reproduce the crash, so I'm not sure if it affects only the telnet
client, or other external protocol handlers. If it's only telnet, we could
enforce the limit for that protocol only.
Comment 23 Daniel Veditz [:dveditz] 2002-11-01 23:53:21 PST
MAX_PATH on windows is only 260, appropriate for a filepath (not always with UNC
names, though) but pretty short for many URLs.

On Win2K I don't see a crash, but shorter urls get passed through to the app and
longer ones don't appear to launch the app at all. Using regmon I see that we've
gotten as far as pulling the appropriate command line out of the registry
regardless of the length of the URL. The length that works appears to depend on
that command line -- for me aol:aaaaa... works with 2018 a's and doesn't with
2019. For telnet:aaaaa... 2024 a's works and 2025 doesn't. The command lines
pulled out of my registry are:

"C:\Program Files\Americal Online 7.0\aol.exe" -u"%1"
rundll32.exe url.dll, TelnetProtocolHandler %1

I should mention my test was using a 1.0.2 branch build, not the trunk.

In my opinion 1500 is perfectly safe, and allows for more interesting URL's than
260 chars. If someone wants to cut it down to 1K that'd be OK too.

I'll try this on a win98 machine later, might be a different length.
Comment 24 Daniel Veditz [:dveditz] 2002-11-02 00:19:46 PST
On Win98 the longest aol: url that would work was aol:aaaaa... with 464 a's.
Still no crash on longer ones, even up to 4000 a's.

Can anyone but Georgi get this to crash? Which *exact* version of windows?
Comment 25 Daniel Veditz [:dveditz] 2002-11-02 00:21:22 PST
Created attachment 104933 [details]
testcase
Comment 26 georgi - hopefully not receiving bugspam 2002-11-02 04:15:45 PST
Strange.
I crash on Win2K SP1 + IE 6.0 + a lot of m$ patches.
But *don't* crash on freshly installed Win2K.
This bug is related to bug 149627 (mailto: (external reader) clicking on link
crashes moz).
I get the same stack as the reporter in the above bug (00610061, the stack is
totally corrupt 0x0061= 'a' unicode).

When you don't crash, have you applied any m$ service packs?
Comment 27 Darin Fisher 2002-11-03 16:15:52 PST
i would suggest only limiting the length of externally handled URLs under
windows... afterall, URLs are meant to be arbitrary in length, though the
standard suggests that some software limits URLs to 4096 bytes in length.
Comment 28 Daniel Veditz [:dveditz] 2002-11-04 10:35:06 PST
I have win2k sp3 and IE 6.0, no additional fixes because windows update no
longer works on my machine.
Comment 29 rpotts (gone) 2002-11-04 10:58:51 PST
hey dan,

why do you think that MAX_PATH is too limiting?  The argument is being passed
into the 'lpFile' argument of ShellExecute(...) -- which 'implies' that file
length constraints apply...

i agree that limiting the URL to 260 characters is limiting... but it seems that
allowing anything more brings up the possibility that some APP will crash
because it doesn't handle a longer command line...

-- rick
Comment 30 Mitchell Stoltz (not reading bugmail) 2002-11-04 13:38:05 PST
Yes, but ultimately we aren't responsible for bugs in other apps. We *are*
responsible for keeping things like the aol: protocol working. I suggest we go
with Georgi's empirical approach - a limit of 1500 seems to stop the crash, and
that's probably enough length for most applications.

Otherwise, since the crash only seems to happen in certain configurations and
appears to have been fixed in the latest service packs, maybe we should do
nothing...
Comment 31 Mitchell Stoltz (not reading bugmail) 2002-11-04 14:16:52 PST
Considering that Georgi has been able to produce a crash only under SP1 (they're
up to SP3), I don't think we should take any action for now. This appears to be
a Microsoft bug that Microsoft has already fixed. Let's not risk regressions to
fix a problem that can be fixed by getting a newer service pack.

Doug, Darin, Rick, Georgi, etc, please tell me if you agree.
Comment 32 Darin Fisher 2002-11-04 14:35:50 PST
i agree; however, we should add a comment in the release notes or something that
encourages our users to upgrade from SP1 if they have not done so already.
Comment 33 Doug Turner (:dougt) 2002-11-04 14:43:58 PST
we should fix our code too.  The patch is the right thing to do.
Comment 34 Daniel Veditz [:dveditz] 2002-11-04 14:46:12 PST
Note the "other app" in this case is Windows and the program that dies because
of it is us. At the very least this becomes a easy DOS, one that could be
carried out through mail spam.

QA should test a few other versions of windows -- try other Win2k SP1 machines
to see if it really is that version of windows, or something else installed on
that machine. (is the crash in shell32.dll? We could check that version) Try
Windows ME to see if they made the same mistake as Win2k.

We should pick a limit of some kind, how 'bout 1K since that solves the problem
on the crashing instance we know about and is a nice round number we could
document. If the URL exceeds that length we should fail. There's little point in
a truncated URL: either it won't work, or worse it will and the user won't
notice it's wrong.
Comment 35 Mitchell Stoltz (not reading bugmail) 2002-11-04 15:30:26 PST
That's two for a hard limit in Mozilla, one against. I'm not so sure: there's no
mandated size limit on URLs (except possibly 4K), and any hard limit runs the
risk of breaking some external protocol handler that happens to use long URLs
legitimately. I don't like setting arbitrary limits to solve problems in other
software, because who's to say Windows 2003 won't crash at 500 chars?

We should take this bug off the adt1.0.2 radar, since there's a workaround
(upgrade Windows) and if we do decide to add a hard limit, it will need a lot of
regression testing (for whatever external protocols AOL uses, if nothing else).
Dan, Michael B, is this OK?
Comment 36 Michael Buckland 2002-11-04 16:32:09 PST
Discussed in bBird team meeting.  Team does not want to pass on fix just yet.  
Comment 37 Doug Turner (:dougt) 2002-11-04 16:45:20 PST
regression testing? Huh?  We are talking about limiting the length of file paths
to something sane.  Make it MAX_PATH as the windows docs suggest:

Address of a null-terminated string that specifies the file to open or print or
the folder to open or explore. The function can open an executable file or a
document file. The function can print a document file. 

_MAX_PATH is the maximum length of full path 
Comment 38 Darin Fisher 2002-11-04 18:50:12 PST
hmm.. so the windows docs are inconsistent with the idea of passing URLs
(arbitrary length strings) through that interface.. go figure ;-)  i'm sure they
never originally intended it to be used with URLs.  at any rate, i agree that it
would be prudent to abide by the MAX_PATH limitation.
Comment 39 Darin Fisher 2002-11-04 18:51:21 PST
BTW: it probably would be worthwhile to check what if any limitation IE6 imposes
on the URL length.  (appologies if someone already checked that -- i haven't
read each comment.)
Comment 40 Daniel Veditz [:dveditz] 2002-11-04 19:51:55 PST
According to http://www.squarefree.com/bookmarklets/limits.html IE allows urls
up to 2083 characters in links. For some unknown reason IE6.0 restricts
javascript URLs (bookmarklets) to 508 characters but allows the full length for
other links (IE5 doesn't discriminate against javascript). But OK, if MAX_PATH
will get this bug fixed for 1.2 I'm all for it and we can experiment in 1.3
Comment 41 Daniel Veditz [:dveditz] 2002-11-04 20:20:14 PST
Using Netscape download data (I don't have Mozilla figures) of the people using
Win2k to download Netscape 7.0
 19% Win2k
  8% Win2k SP1
 35% Win2k SP2
 38% Win2k SP3

Win2k total is about a quarter of windows users who download N7.0

Since SP3 was the result of the trustworth computing initiative it wouldn't be
surprising to find this crash in all other versions, meaning possibly 14% of
Netscape 7.0 windows users were susceptible. Maybe more if the initial XP
version was also susceptible (that'd take us to around 38% of windows users). On
the other hand maybe only Win2k Sp1 is vulnerable (2%), or maybe only Georgi's
machine due to some conflicting software he has installed.

We really need a QA test of the lab machines of various versions. Open this bug,
click the longest link in the telnet set of cases -- does it crash? We just
don't know enough.
Comment 42 georgi - hopefully not receiving bugspam 2002-11-05 02:28:32 PST
From the talkback of bug 149627 
Operating System Windows NT 5.0 build 2195
seems affected from a similar problem with external "mailto:", there is testcase
there.
For Comment #34: Yes, the crash is in shell32.dll for me. Check Comment #20 for
a standalone testcase.
Regarding whether this should be fixed or not: Since this is not reproducible on
fully patched windows, it is OK for me not to be fixed. But people are filing
bugs - bug 149627 on this. Having in mind the fix is trivial and does not break
anything (long external URLs are not loaded at all) this may also be fixed.
Regarding the discussing MAX_PATH or 1500 - I think a working empirical value is
more proper than a MAX_PATH of 260. In the case of mailto: URLs, in the URL may
be included parameters, not only file path, e.g. mailto:a@a?body=aaaaaaaa
Comment 43 Michael Buckland 2002-11-05 13:01:37 PST
adt wants but needs checkin on 11/5
Comment 44 Daniel Veditz [:dveditz] 2002-11-05 13:37:55 PST
Created attachment 105225 [details] [diff] [review]
another patch
Comment 45 Daniel Veditz [:dveditz] 2002-11-05 14:23:11 PST
I changed the value from 1k to 2K based on Darin's review comments. People are
crashing using mailto: urls so obviously at least some versions of IE are
expected to work with long URLS.  The ShellExecute() docs don't say anything
about length unfortunately, but the "file" argument is really a "file or object"
argument so MAX_PATH is not a reasonable assumption.

lpFile
[in] Pointer to a null-terminated string that specifies the file or object on
which to execute the specified verb. To specify a Shell namespace object, pass
the fully qualified parse name. Note that not all verbs are supported on all
objects. For example, not all document types support the "print" verb.
Comment 46 Darin Fisher 2002-11-05 14:28:39 PST
Comment on attachment 105225 [details] [diff] [review]
another patch

>Index: nsOSHelperAppService.cpp

>+// Some versions of windows (Win2k before SP3, Win XP before SP1)
>+// crash in ShellExecute on long URLs.
>+// IE 5 and 6 support URLS of 2083 chars in length, 1K is paranoid
>+    const PRUint32 maxSafeURL(1024);
>+    if (urlSpec.Length() > maxSafeURL)
>+      return NS_ERROR_FAILURE;

i'd be willing to bet that IE/ShellExecute's real limit is 2k...

compatibility with IE is paramount, provided it does not introduce a
known buffer overrun.  a limit of 2k appears to be safe.

moreover, the argument about ShellExecute's lpFile documentation is
weak.  the documentation says:

  lpFile
     [in] Pointer to a null-terminated string that specifies the
     file or object on which to execute the specified verb. To
     specify a Shell namespace object, pass the fully qualified
     parse name. Note that not all verbs are supported on all
     objects. For example, not all document types support the "print"
     verb.

an URL is not a file path.  therefore, an URL is not contrained to MAX_PATH
number of bytes.  according to the documentation for ShellExecute, there
is no upper limit on the length of an _URL_ passed into the function.

we know for a fact that folks may be passing around large mailto: URLs.  it
happens.  there's no point in taking a regression here if it doesn't solve
a known buffer overrun.

chances are ShellExecute's limit is not 2083, but more likely 2048.  anything
bigger than 2048 is probably a buffer overrun.	how do i know?	i don't, but
it seems reasonable to imagine that they would have picked a power-of-2 as
the size of some internal buffer.  bottom line: going w/ 2048 maximizes 
compatibility w/ IE while protecting against a known buffer overrun.

sr=darin if you change maxSafeURL to 2048.
Comment 47 georgi - hopefully not receiving bugspam 2002-11-05 14:42:50 PST
IIRC IE silently ignores long mailto: URLs.
Is someone able to open a 3K+ mailto: URL from internet explorer?

For the limit of 2K - I suggest lowering it with a few hundreds, better safe
than some exotic exploit corrupts locals on the stack without crashing.
Comment 48 Mitchell Stoltz (not reading bugmail) 2002-11-05 14:49:43 PST
Comment on attachment 105225 [details] [diff] [review]
another patch

r=mstoltz. I'd like to confirm that a limit of 2048 is small enough to avoid
the crash. Have we confirmed that, or are we guessing?
Comment 49 georgi - hopefully not receiving bugspam 2002-11-05 15:13:49 PST
I don't know whether this is safe, that's why I suggested decreasing it.
Putting a var on the stack, then calling ShellExecute and checking the var after
the call may help.
Comment 50 chris hofmann 2002-11-05 19:02:36 PST
Comment on attachment 105225 [details] [diff] [review]
another patch

a=chofmann for 1.0.2
Comment 51 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-11-05 21:47:37 PST
Comment on attachment 105225 [details] [diff] [review]
another patch

a=shaver for the 1.2 branch.
Comment 52 Daniel Veditz [:dveditz] 2002-11-05 22:03:03 PST
Checked into 1.0.2 branch, 1.2 branch and the trunk -- FIXED
Comment 53 bsharma 2002-11-06 13:59:07 PST
Verified on 2002-11-06-branch build on Win 2000.

Attached test case works fine. An exception is thrown for each test.

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