Bug 666607 (CVE-2011-2994)

Binary planting in address book import from Outlook Express on Windows 7

RESOLVED FIXED in Thunderbird 8.0

Status

Thunderbird
General
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: Mitja Kolsek, Assigned: Away for a while)

Tracking

unspecified
Thunderbird 8.0
x86
Windows 7

Firefox Tracking Flags

(blocking-thunderbird3.1 .12+, thunderbird3.1 .12-fixed, thunderbird6 fixed, thunderbird7 fixed)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 541400 [details]
Vulnerability report ASPR-MOZ-1
(Reporter)

Comment 2

7 years ago
Created attachment 541402 [details]
Files referenced in the report
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.
(Reporter)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
Created attachment 542153 [details] [diff] [review]
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.
Assignee: nobody → ehsan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #542153 - Flags: review?(dbienvenu)

Comment 6

7 years ago
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)) {
Attachment #542153 - Flags: review?(dbienvenu) → review?

Updated

7 years ago
Attachment #542153 - Flags: review? → review?(sid.bugzilla)
Binary planting is critical (run malware) potential impact, but in this case mitigated by being triggered by rarely used functionality
Whiteboard: [sg:moderate]
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.
Attachment #542153 - Flags: review?(sid.bugzilla) → review-
Ugh, Splinter failed miserably. I'm referring to the two _tcsncat instances.
Created attachment 544124 [details] [diff] [review]
patch v2

Here's the patch with my fixes included. I'm fine with either bienvenu or ehsan signing off on this.
Attachment #542153 - Attachment is obsolete: true
Attachment #544124 - Flags: review?(ehsan)
Attachment #544124 - Flags: review?(dbienvenu)

Comment 11

7 years ago
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...
I didn't bother commenting on the code style because the files themselves had no consistent style to speak of.

Comment 13

7 years ago
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!
Attachment #544124 - Flags: review?(dbienvenu) → review+
https://hg.mozilla.org/comm-central/rev/8a25422c9fca

Sorry for the attribution error! No idea how that happened.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 8.0
Comment on attachment 544124 [details] [diff] [review]
patch v2

We probably want to get this into aurora/beta.
Attachment #544124 - Flags: approval-comm-beta?
Attachment #544124 - Flags: approval-comm-aurora?
Note: there were a few changes between v2 and what I checked in, all irc-r=bienvenu...
Attachment #544124 - Flags: review?(ehsan)
Attachment #544124 - Flags: approval-comm-beta?
Attachment #544124 - Flags: approval-comm-beta+
Attachment #544124 - Flags: approval-comm-aurora?
Attachment #544124 - Flags: approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/d7e23e1082cb
https://hg.mozilla.org/releases/comm-beta/rev/c48237b526a7
status-thunderbird6: --- → fixed
status-thunderbird7: --- → fixed
(Assignee)

Comment 18

7 years ago
I was away last week, but just wanted to say that the changes here look good, thanks Sid for taking care of this.
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.
Attachment #544124 - Flags: approval-thunderbird3.1.12+
blocking-thunderbird3.1: --- → .12+
https://hg.mozilla.org/releases/comm-1.9.2/rev/92da70e0e300
status-thunderbird3.1: --- → .12-fixed
...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.
Alias: CVE-2011-2994

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.