666607 - (CVE-2011-2994) Binary planting in address book import from Outlook Express on Windows 7

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Mitja Kolsek]

User-Agent:       Mozilla/5.0 (Windows NT 5.1) AppleWebKit/534.30 (KHTML, like Gecko) Chrome/12.0.742.100 Safari/534.30
Build Identifier: 3.1.10, 3.1.11

There is a Binary Planting vulnerability in Mozilla Thunderbird 3.1.11, allowing a remote, possibly Internet-based attacker to deploy malicious dynamic-link library (DLL) to a user's Windows machine and have it launched in the context of that user. In particular, Mozilla's Thunderbird.exe tries to load  %CommonProgramFiles%\System\wab32.dll when a user tries to import an Outlook Express address book on Windows 7, but failing to find it in the search path provides an opportunity for the attacker to plant a malicious DLL in the current working directory.

More details in the attached report.

Reproducible: Always

Comment 1

[Mitja Kolsek]

Attachment: Vulnerability report ASPR-MOZ-1

Comment 2

[Mitja Kolsek]

Attachment: Files referenced in the report

Comment 3

[Ludovic Hirlimann [:Usul]]

I fail to see how this is so different than bug 666608 ?
Will do some testing later but probably not today nor this week-end.

Comment 4

[Mitja Kolsek]

While both bugs represent the same type of vulnerability (binary planting, also called DLL hijacking or DLL preloading) and are both in the "import" functionality they have to be addressed separately. Bug 666608 seems to be in Microsoft's code actually (although can be mitigated by Mozilla), while this one is clearly in Mozilla's.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Here's what this patch does: if the WAB_DLL_PATH_KEY key exists in the registry and its value is a REG_EXPAND_SZ type, we call ExpandEnvironmentStrings to get an absolute path.  Otherwise, we try to load the DLL explicitly from the system directory.

Please note that I haven't built with this patch myself, so I'd appreciate if you could do a bit of smoke testing on it.

Comment 6

[David :Bienvenu]

Comment on attachment 542153 [details] [diff] [review]
Patch (v1)

I'm going to switch review request to Sid after I cc him

but, this can be changed to 

else if (...)

+    } else {
+        if (GetSystemDirectory(wabDLLPath, MAX_PATH)) {

Comment 7

[Daniel Veditz [:dveditz]]

Binary planting is critical (run malware) potential impact, but in this case mitigated by being triggered by rarely used functionality

Comment 8

[Rain [:rain1] (inactive)]

Comment on attachment 542153 [details] [diff] [review]
Patch (v1)

Review of attachment 542153 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.

tchar.h needs to be included both places for the patch to work.

::: mailnews/addrbook/src/nsWabAddressBook.cpp
@@ -68,0 +68,8 @@
> > +        if (keyType == REG_EXPAND_SZ) {
> > +            // Expand the environment variables
> > +            DWORD bufferSize = ExpandEnvironmentStrings(wabDLLPath, NULL, 0);
> > +            if (bufferSize && bufferSize < MAX_PATH) {
NaN more ...

... else we should error out, it seems to me.

@@ -68,0 +68,13 @@
> > +        if (keyType == REG_EXPAND_SZ) {
> > +            // Expand the environment variables
> > +            DWORD bufferSize = ExpandEnvironmentStrings(wabDLLPath, NULL, 0);
> > +            if (bufferSize && bufferSize < MAX_PATH) {
NaN more ...

This should be MAX_PATH - dllNameLength - 1, right? The count doesn't include the null terminator.

::: mailnews/import/oexpress/WabObject.cpp
@@ +140,5 @@
> +                if (bufferSize && bufferSize < MAX_PATH) {
> +                    TCHAR tmp [MAX_PATH];
> +                    ExpandEnvironmentStrings(szWABDllPath, tmp, bufferSize);
> +                    _tcscpy(szWABDllPath, tmp);
> +                }

We should error out here too.

@@ -137,0 +137,13 @@
> > +            if (dwType == REG_EXPAND_SZ) {
> > +                // Expand the environment variables
> > +                DWORD bufferSize = ExpandEnvironmentStrings(szWABDllPath, NULL, 0);
> > +                if (bufferSize && bufferSize < MAX_PATH) {
NaN more ...

And add a - 1 here, I think.

Comment 9

[Rain [:rain1] (inactive)]

Ugh, Splinter failed miserably. I'm referring to the two _tcsncat instances.

Comment 10

[Rain [:rain1] (inactive)]

Attachment: patch v2

Here's the patch with my fixes included. I'm fine with either bienvenu or ehsan signing off on this.

Comment 11

[David :Bienvenu]

thx for this, Sid. I haven't tried the patch yet, but one nit - the } else { lines seem to be wrongly indented, and else should be on its own line...

Comment 12

[Rain [:rain1] (inactive)]

I didn't bother commenting on the code style because the files themselves had no consistent style to speak of.

Comment 13

[David :Bienvenu]

Comment on attachment 544124 [details] [diff] [review]
patch v2

While you're here, can you remove the spaces only line after this line:
    HKEY keyHandle = NULL ;
    

and fix those else braces.

Thx very much for fixing this!

Comment 14

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/comm-central/rev/8a25422c9fca

Sorry for the attribution error! No idea how that happened.

Comment 15

[Rain [:rain1] (inactive)]

Comment on attachment 544124 [details] [diff] [review]
patch v2

We probably want to get this into aurora/beta.

Comment 16

[Rain [:rain1] (inactive)]

Note: there were a few changes between v2 and what I checked in, all irc-r=bienvenu...

Comment 17

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/releases/comm-aurora/rev/d7e23e1082cb
https://hg.mozilla.org/releases/comm-beta/rev/c48237b526a7

Comment 18

[(no longer active)]

I was away last week, but just wanted to say that the changes here look good, thanks Sid for taking care of this.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 544124 [details] [diff] [review]
patch v2

I'm assuming this will probably apply & work on 3.1.12, and we need it there as well. So a=Standard8 for that.

Comment 20

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/releases/comm-1.9.2/rev/92da70e0e300

Comment 21

[Justin Wood (:Callek)]

...Just for the record, I tried to reproduce in SeaMonkey on SeaMonkey 2.2 (win7 x64, sea binary x86), and could not. That said this patch was mailnews core so should be safe for future anyway.