Closed Bug 666607 (CVE-2011-2994) Opened 9 years ago Closed 9 years ago

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

Categories

(Thunderbird :: General, defect)

x86
Windows 7
defect
Not set

Tracking

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

RESOLVED FIXED
Thunderbird 8.0
Tracking Status
blocking-thunderbird3.1 --- .12+
thunderbird3.1 --- .12-fixed
thunderbird6 --- fixed
thunderbird7 --- fixed

People

(Reporter: mitja.kolsek, Assigned: ehsan)

Details

(Whiteboard: [sg:moderate])

Attachments

(3 files, 1 obsolete file)

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
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.
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.
Attached patch Patch (v1) (obsolete) — Splinter Review
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 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?
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.
Attached patch patch v2Splinter Review
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)
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 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
Closed: 9 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+
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+
...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
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.