Closed Bug 161357 Opened 23 years ago Closed 22 years ago

Buffer overflow with external protocol handler

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: security-bugs, Assigned: security-bugs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:blocker])

Attachments

(2 files, 2 obsolete files)

testcase
36.57 KB, text/html
Details
another patch
895 bytes, patch
security-bugs
: review+
darin.moz
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review
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.
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?
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.
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.
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 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+
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+
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+
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.
Blocks: blackbird
Keywords: adt1.0.2
Georgi, if you can, please try to generate a stack trace of the crash.
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.
So this needs a new patch? drivers want this for 1.2 (real soon now). What's the status?
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?
Attached file 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?
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.
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
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...
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.
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.
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.
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?
Discussed in bBird team meeting. Team does not want to pass on fix just yet.
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
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.
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
adt wants but needs checkin on 11/5
Attached patch another patch — — Splinter Review
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.
Attachment #105225 - Flags: superreview+
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.
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.
Comment on attachment 105225 [details] [diff] [review] another patch a=chofmann for 1.0.2
Attachment #105225 - Flags: approval+
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
Closed: 22 years ago
Keywords: mozilla1.0.2+ → fixed1.0.2
Resolution: --- → FIXED
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
Group: security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: