Buffer overflow with external protocol handler

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
15 years ago
12 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:blocker])

Attachments

(2 attachments, 2 obsolete attachments)

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.
AFAIK on Linux "telnet" and "rlogin" protocols don't work despite some
mentioniong of "xterm -e ..." in some js preference?
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.
Whiteboard: [sg:blocker]
(Assignee)

Comment 3

15 years ago
Georgi: do you think this is a bug in Mozilla, or in the telnet client? Looks
like it might be Mozilla.
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?

(Assignee)

Comment 5

15 years ago
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?
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.
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.

Blocks: 174647
Created attachment 103989 [details] [diff] [review]
propsed patch
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.
Created attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option
Attachment #103989 - Attachment is obsolete: true
Anyone want to review this?  It's a 1.2 blocker.
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

sr=dveditz
Attachment #103993 - Flags: superreview+

Comment 13

15 years ago
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.
Attachment #103993 - Flags: needs-work+
(Assignee)

Comment 14

15 years ago
Comment on attachment 103993 [details] [diff] [review]
proposed patch with correct diff(1) option

r=mstoltz. I'll try to get this checked in.
Attachment #103993 - Flags: review+
(Assignee)

Comment 15

15 years ago
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.
Attachment #103993 - Flags: review+

Comment 16

15 years ago
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
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.

Updated

15 years ago
Blocks: 168047
Keywords: adt1.0.2
(Assignee)

Comment 18

15 years ago
Georgi,
   if you can, please try to generate a stack trace of the crash.

Comment 19

15 years ago
Discussed in bBird team meeting.  Plussing for adt only.
Keywords: adt1.0.2 → adt1.0.2+
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

15 years ago
So this needs a new patch? drivers want this for 1.2 (real soon now). What's the
status?
(Assignee)

Comment 22

15 years ago
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.
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.
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?
Created attachment 104933 [details]
testcase
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

15 years ago
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.
I have win2k sp3 and IE 6.0, no additional fixes because windows update no
longer works on my machine.

Comment 29

15 years ago
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
(Assignee)

Comment 30

15 years ago
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...
(Assignee)

Comment 31

15 years ago
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

15 years ago
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

15 years ago
we should fix our code too.  The patch is the right thing to do.
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.
(Assignee)

Comment 35

15 years ago
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

15 years ago
Discussed in bBird team meeting.  Team does not want to pass on fix just yet.  

Comment 37

15 years ago
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

15 years ago
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

15 years ago
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.)
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
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.
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

15 years ago
adt wants but needs checkin on 11/5
Created attachment 105225 [details] [diff] [review]
another patch
Attachment #103993 - Attachment is obsolete: true
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.

Updated

15 years ago
Attachment #105225 - Flags: superreview+

Comment 46

15 years ago
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.
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.
(Assignee)

Comment 48

15 years ago
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?
Attachment #105225 - Flags: review+
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.
Keywords: mozilla1.0.2

Comment 50

15 years ago
Comment on attachment 105225 [details] [diff] [review]
another patch

a=chofmann for 1.0.2
Attachment #105225 - Flags: approval+

Updated

15 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+
Comment on attachment 105225 [details] [diff] [review]
another patch

a=shaver for the 1.2 branch.
Checked into 1.0.2 branch, 1.2 branch and the trunk -- FIXED
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Keywords: mozilla1.0.2+ → fixed1.0.2
Resolution: --- → FIXED

Comment 53

15 years ago
Verified on 2002-11-06-branch build on Win 2000.

Attached test case works fine. An exception is thrown for each test.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.2 → verified1.0.2
(Assignee)

Updated

15 years ago
Group: security
Blocks: 253311
You need to log in before you can comment on or make changes to this bug.