Investigate stack buffer overflow in nsDirectoryService::GetCurrentProcessDirectory

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bsterne, Assigned: bbondy)

Tracking

6 Branch
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Fabian Yamaguchi of Recurity Labs on behalf of BSI (German Federal Office for Information Security) reported the following to security@mozilla.org:

Method: nsDirectoryService::GetCurrentProcessDirectory

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

Summary:
The method nsDirectoryService::GetCurrentProcessDirectory reads a module filename into a local stack­buffer by using the Win32 API function GetModuleFilenameW. This function expects the number of wide­characters that the destination buffer can hold as its last argument. In GetCurrentProcessDirectory, 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:
nsresult
nsDirectoryService::GetCurrentProcessDirectory(nsILocalFile** aFile) {
  [..]
#ifdef XP_WIN
    PRUnichar buf[MAX_PATH];
    if ( ::GetModuleFileNameW(0, buf, sizeof(buf)) )
    {
      // chop off the executable name by finding the rightmost backslash
      PRUnichar* lastSlash = wcsrchr(buf, L'\\');
      if (lastSlash)
        *(lastSlash + 1) = L'\0';
      localFile->InitWithPath(nsDependentString(buf));
      *aFile = localFile;
      return NS_OK;
    }
[..]
}

Comment 1

6 years ago
Ehsan, is it possible for an external link to force Firefox to have an attacker-controlled CWD?
Assignee: nobody → netzen

Comment 2

6 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Ehsan, is it possible for an external link to force Firefox to have an
> attacker-controlled CWD?

By external link, you mean URL links activated through other applications using ShellExecute?  Then, yes.  But I assume that's going to need the user to perform some action beyond usual browsing actions, so, sg:moderate?
Whiteboard: [sg:moderate]

Comment 3

6 years ago
Created attachment 562487 [details] [diff] [review]
Patch (v1)
Assignee: netzen → ehsan
Status: NEW → ASSIGNED
Attachment #562487 - Flags: review?(benjamin)
(Assignee)

Comment 4

6 years ago
Created attachment 562494 [details] [diff] [review]
Patch v2

Sorry I also did a patch, this one also checks for a return value of GetLastError() != ERROR_INSUFFICIENT_BUFFER which if it was not big enough would lead to a crash in the first patch.
Assignee: ehsan → netzen
Attachment #562487 - Attachment is obsolete: true
Attachment #562487 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #562494 - Flags: review?(ehsan)

Comment 5

6 years ago
Comment on attachment 562494 [details] [diff] [review]
Patch v2

If you look at the docs for this function <http://msdn.microsoft.com/en-us/library/windows/desktop/ms683197%28v=vs.85%29.aspx>, it will truncate the path to the module if the size of the buffer is too small.  The tricky part is that the truncated buffer is _not_ null terminated on Windows 2K/XP.  Which means that you need to set bug[MAX_PATH] to '\0'.

Other than that, this patch has the extremely common Windows bug of calling an API and then immediately calling GetLastError and assuming that the error code is coming from that function.  Win32 functions which finish successfully don't clear out the error code, so the error code returned from GetLastError is not necessarily coming from the Win32 API called right before it.  The way you usually prevent against that is calling SetLastError(ERROR_SUCCESS) right before calling the API which is supposed to set the error code.
Attachment #562494 - Flags: review?(ehsan) → review-
(Assignee)

Comment 6

6 years ago
>  it will truncate the path...

Yup you're right about truncating, but I think we don't want to use the truncated path.

> this patch has the extremely common Windows bug 

Most Win32 APIs will correctly set the last error, but some will not.  This Win32 API in particular does set the success code.  You can verify it with the following code.  If you would like me to resubmit a patch though that uses only the return code please let me know. 

  WCHAR exePath[MAX_PATH+1];
  SetLastError(1024);
  assert(GetLastError() == 1024);
  if (!::GetModuleFileNameW(0, exePath, 1024))
    return 1;

  DWORD dw2 = GetLastError();
  assert(GetLastError() == ERROR_SUCCESS);
(Assignee)

Comment 7

6 years ago
> !::GetModuleFileNameW(0, exePath, ***MAX_PATH+1***))
(Assignee)

Comment 8

6 years ago
Comment on attachment 562494 [details] [diff] [review]
Patch v2

As per last 2 comments.
Attachment #562494 - Flags: review- → review?(ehsan)

Comment 9

6 years ago
Comment on attachment 562494 [details] [diff] [review]
Patch v2

As discussed with Brian on IRC, we might be able to use _get_pgmptr here.  He'll investigate whether that's feasible or not.
Attachment #562494 - Flags: review?(ehsan)
(Assignee)

Comment 10

6 years ago
Created attachment 564108 [details] [diff] [review]
Patch v3

I still have to try it on mingw builds before pushing it, but here is the patch that works with normal VS builds.  I haven't had a chance to get a mingw setup together yet.
Attachment #562494 - Attachment is obsolete: true
Attachment #564108 - Flags: review?(ehsan)

Comment 11

6 years ago
Comment on attachment 564108 [details] [diff] [review]
Patch v3

As a sanity check, please initialize pathBuffer to NULL.  r=me with that, although I really like us to make sure not to break mingw here.
Attachment #564108 - Flags: review?(ehsan) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 565393 [details] [diff] [review]
Patch v4

Last one was r+ but you asked that I r= you again so did so :)

I will not push this by the way until I try on mingw. I'm having some trouble (or error after error after error...) with building it on mingw/cygwin and don't have a lot of time until next week to keep figuring out why.  I need to get it up though for another bug as well.
Attachment #564108 - Attachment is obsolete: true
Attachment #565393 - Flags: review?(ehsan)

Comment 13

6 years ago
Comment on attachment 565393 [details] [diff] [review]
Patch v4

I actually didn't need to look at it again, but anyways, r=me.  :-)
Attachment #565393 - Flags: review?(ehsan) → review+

Comment 14

6 years ago
bbondy, ping?
(Assignee)

Comment 15

6 years ago
There's a good chance I wont' get a chance to try this with a mingw dev environment until after silent update lands (which will be within 1-2 weeks).  But if I do get a chance I'll update here.  It's still on my todo list.
(Assignee)

Comment 16

6 years ago
mingw does not have _get_pgmptr nor _get_wpgmptr so we'll have to go back to the method of the last patch.  I'll resubmit for review a new patch today. 

These functions are declared on msvc in stdlib.h.  mingw's stdlib.h does not have these methods nor does any header in cygwin / mingw.
(Assignee)

Comment 17

6 years ago
By the way it does have the globals _pgmptr and __wpgmptr but the whole method doesn't sem very portable so going back to GetModuleFileNameW
(Assignee)

Comment 18

6 years ago
Created attachment 573409 [details] [diff] [review]
Patch v5
Attachment #565393 - Attachment is obsolete: true
Attachment #573409 - Flags: review?(ehsan)

Comment 19

6 years ago
Comment on attachment 573409 [details] [diff] [review]
Patch v5

Use mozilla::ArrayLength.  r=me with that.
Attachment #573409 - Flags: review?(ehsan) → review+
(Assignee)

Comment 20

6 years ago
Created attachment 577139 [details] [diff] [review]
Patch v6

6 patches is way too much for this task :)
Attachment #573409 - Attachment is obsolete: true
Attachment #577139 - Flags: review?(ehsan)

Comment 21

6 years ago
Comment on attachment 577139 [details] [diff] [review]
Patch v6

Sorry about being extremely strict on reviews.. :-)
Attachment #577139 - Flags: review?(ehsan) → review+
(Assignee)

Comment 22

6 years ago
np it's a good thing :), it was mostly because of the detour on the _get_pgmptr/mingw problem.
(Assignee)

Comment 23

6 years ago
Pushed to try win32 only, no talos:
https://tbpl.mozilla.org/?tree=Try&rev=afa2b083848d

Will push to inbound once that succeeds.
(Assignee)

Comment 24

6 years ago
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4e299e8ee35
Target Milestone: --- → mozilla11

Comment 25

6 years ago
https://hg.mozilla.org/mozilla-central/rev/b4e299e8ee35
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.