Last Comment Bug 688882 - Investigate stack buffer overflow in nsLocalFile::EnsureShortPath
: Investigate stack buffer overflow in nsLocalFile::EnsureShortPath
Status: RESOLVED FIXED
[sg:nse]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 6 Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-23 14:39 PDT by Brandon Sterne (:bsterne)
Modified: 2011-09-29 15:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Buffer overflow fix (1.49 KB, patch)
2011-09-26 10:50 PDT, Brian R. Bondy [:bbondy]
benjamin: review+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2011-09-23 14:39:04 PDT
Fabian Yamaguchi of Recurity Labs on behalf of BSI (German Federal Office for Information Security) reported the following to security@mozilla.org:

Method: nsLocalFile::EnsureShortPath

Component:
The bug is contained in the nsLocalFile Implementation for Windows.

Summary:
The method nsLocalFile::EnsureShortPath contains a call to GetShortPathName, which potentially overflows a stack­based buffer.

Overview:
The method nsLocalFile::EnsureShortPath reads a path name into a local stack­buffer by using the Win32 API function GetShortPathNameW. This function expects the number of wide­characters that the destination buffer can hold as its last argument. In EnsureShortPath, this third parameter has mistakenly been chosen to contain the number of bytes the buffer consists of, which is twice as much as the number of wide­characters it can hold. Therefore, it may be possible to trigger a stack­ based buffer­overflow.

Actual Results:
A local stack­buffer can potentially be overflowed.

Expected Results:
The buffer cannot be overflowed.

Additional Information:
The vulnerable code is the following.
void nsLocalFile::EnsureShortPath()
{
[...]
    WCHAR thisshort[MAX_PATH];
    DWORD thisr = ::GetShortPathNameW(mWorkingPath.get(), thisshort,
                                      sizeof(thisshort));
[...]
}
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-09-26 09:16:51 PDT
I don't think this is actually a security problem because we never use \\?\ style paths, so they cannot be longer than MAX_PATH. But we should still fix the code.
Comment 2 Brian R. Bondy [:bbondy] 2011-09-26 10:50:15 PDT
Created attachment 562479 [details] [diff] [review]
Buffer overflow fix
Comment 3 Brian R. Bondy [:bbondy] 2011-09-28 11:03:00 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d0fbb1c0ab14
Comment 4 Brian R. Bondy [:bbondy] 2011-09-29 06:10:12 PDT
Pushed to inbound: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/c872ba5d7b05
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 14:30:20 PDT
https://hg.mozilla.org/mozilla-central/rev/4ca939ac46fa

I accidentally pushed this patch to mozilla-central while it was living on inbound.  On the next merge, it will be merged.  Sorry for the mess!
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-29 15:48:43 PDT
Merged from inbound: https://hg.mozilla.org/mozilla-central/rev/c872ba5d7b05

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