318618 - Unchecked malloc in CertReader

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Taral]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

http://lxr.mozilla.org/seamonkey/source/xpinstall/src/CertReader.cpp#218

The uncompressed size is passed to malloc() without checking for invalid values. Probably not a big deal (it only crashes the browser), but still a bad idea nonetheless.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Daniel Veditz [:dveditz]]

I don't think this is a security problem -- it could suck all the memory out of the browser though.

This should fix it:

-    if (orgSize == 0)
+    if (orgSize == 0 || orgSize > MAX_SIGNATURE_SIZE)
        return NS_BINDING_ABORTED;

There's a problem in the non-DEFLATED case with the memcpy. If the file is simply STORED csize should equal orgSize, but we don't verify that and go ahead and copy an orgSize amount of data out of the stream (and potentially lots of memory beyond it). That may be where the crash comes in, eventually reading far enough to get an access violation.

I don't think this aspect is exploitable either, if we get that far the malloc succeeded so we're not overwriting anything and VerifySignature should simply return an error on non-signature data. But the above check doesn't entirely solve the problem: we could still read < 32K data that isn't ours and possibly crash with an access violation.

-     else {
+     else if (orgSize == csize) {
        memcpy(orgData, data, orgSize);
      }
+     else
+       err = Z_DATA_ERROR;

Comment 2

[Daniel Veditz [:dveditz]]

Attachment: check read length and limit buffer size

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: avoid memcpy() in the non-compressed case

Here's an alternate version that avoids the useless malloc/memcpy in the uncompressed case (plus a bunch of whitespace cleanup). No real-world difference though since install is a rare action. Maybe the simpler change is better.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 204799 [details] [diff] [review]
avoid memcpy() in the non-compressed case

this is cleaner and style fixup to boot!

Comment 5

[Mike Schroepfer]

Per bug triage meeting Please land in the trunk and re-nominate.

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: -w version of the second patch

Comment 7

[Daniel Veditz [:dveditz]]

This was checked in Friday night, so was in the 20060107 trunk builds

Comment 8

[Daniel Veditz [:dveditz]]

Signed installs can be regression tested here: http://www.mozilla.org/projects/xpinstall/signed/testcases/

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 204799 [details] [diff] [review]
avoid memcpy() in the non-compressed case

a=dveditz for drivers

Comment 10

[Marcia Knous [:marcia]]

verified fixed on the branch using the regression tests noted in Comment 8, using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1.

Comment 11

[Bob Clary [:bc] (inactive)]

I installed dougt's ca cert, gave it rights to install software, restarted and tried to install the signed xpi on the signed xpi install page in 1.8, 1.8.0.1, 1.8.1, 1.9a1 on windows and it blocked the install each time because www.mozilla.org was not whitelisted. Is that correct?

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> it blocked the install each time because www.mozilla.org was not
> whitelisted. Is that correct?

sure, whether signing works or is exploitable is independent of the anti-annoyance install whitelisting. you have to get past the whitelist check to test this bug.