Last Comment Bug 589190 - (CVE-2010-3181) Binary planting potential in nsMIMEInfoWin::LaunchWithFile
(CVE-2010-3181)
: Binary planting potential in nsMIMEInfoWin::LaunchWithFile
Status: RESOLVED FIXED
[sg:critical?] [qa-ntd-191] [qa-ntd-192]
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla2.0b5
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-20 09:16 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2010-11-11 14:11 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.11+
.11-fixed
.14+
.14-fixed


Attachments
Patch (v1) (1.34 KB, patch)
2010-08-20 11:56 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review+
Details | Diff | Review
Patch (v1.1) (1.77 KB, patch)
2010-08-25 11:27 PDT, :Ehsan Akhgari (busy, don't ask for review please)
jst: approval2.0+
dveditz: approval1.9.2.11+
dveditz: approval1.9.1.14+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2010-08-20 09:16:01 PDT
This code <http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/win/nsMIMEInfoWin.cpp#142> needs to use an absolute path.
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-20 11:56:11 PDT
Created attachment 467835 [details] [diff] [review]
Patch (v1)
Comment 2 Benjamin Smedberg [:bsmedberg] 2010-08-25 07:16:38 PDT
Comment on attachment 467835 [details] [diff] [review]
Patch (v1)

Please make rundll32.exe a #define so that the sizeof and the lstrcat don't accidentally end up out of sync. As noted on IRC, I'd prefer that we had a helper function to do this (that we could share to fix LoadLibrary callsites also), but that is not essential.
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-25 11:27:33 PDT
Created attachment 469115 [details] [diff] [review]
Patch (v1.1)

(In reply to comment #2)
> Comment on attachment 467835 [details] [diff] [review]
> Patch (v1)
> 
> Please make rundll32.exe a #define so that the sizeof and the lstrcat don't
> accidentally end up out of sync.

Done.

> As noted on IRC, I'd prefer that we had a
> helper function to do this (that we could share to fix LoadLibrary callsites
> also), but that is not essential.

I'll do that if we decide to patch all the callsites in bug 286382.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-25 19:26:18 PDT
http://hg.mozilla.org/mozilla-central/rev/e94ae58bfd42
Comment 5 Daniel Veditz [:dveditz] 2010-08-27 10:12:49 PDT
Is there really a risk that rundll32.dll won't exist in the windows system directory? If it's always in the path before the current directory there's no "dll planting" risk.
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2010-08-27 12:09:26 PDT
(In reply to comment #5)
> Is there really a risk that rundll32.dll won't exist in the windows system
> directory? If it's always in the path before the current directory there's no
> "dll planting" risk.

The order in which Windows tries to load executables from is even worse than DLLs:

    The directory from which the application loaded.
    The current directory for the parent process.
    The 32-bit Windows system directory. Use the GetSystemDirectory function to get the path of this directory.
    The 16-bit Windows system directory. There is no function that obtains the path of this directory, but it is searched. The name of this directory is System.
    The Windows directory. Use the GetWindowsDirectory function to get the path of this directory.
    The directories that are listed in the PATH environment variable. Note that this function does not search the per-application path specified by the App Paths registry key. To include this per-application path in the search sequence, use the ShellExecute function.

(From http://msdn.microsoft.com/en-us/library/ms682425%28VS.85%29.aspx)

Therefore, if rundll32.exe is present in the current directory, it will be picked up by default no matter whether one exists in the system directory).

(BTW, rundll32 is not a DLL, it's an executable program.)
Comment 7 Daniel Veditz [:dveditz] 2010-08-30 10:22:59 PDT
Comment on attachment 469115 [details] [diff] [review]
Patch (v1.1)

Approved for 1.9.2.10 and 1.9.1.13, a=dveditz for release-drivers
Comment 9 Al Billings [:abillings] 2010-09-09 15:20:51 PDT
Other than code inspection, is there anything for QA to do in regards to verification for this bug on the branches?
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-09 16:09:17 PDT
(In reply to comment #9)
> Other than code inspection, is there anything for QA to do in regards to
> verification for this bug on the branches?

I'm afraid not.
Comment 11 Al Billings [:abillings] 2010-09-21 17:06:16 PDT
All right. Thanks.

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