351848 - DecryptSigBlock does not check the message digest's length is correct

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

The callers of DecryptSigBlock pass a buffer of size
HASH_LENGTH_MAX to ensure it is large enough to hold
any message digest.  But DecryptSigBlock only checks
that the message digest can fit in the buffer.  It doesn't
check that the message digest's length is correct for the
message digest algorithm, and it doesn't return the length
to the callers, so the callers can't check that, either.

So for MD5, one can append up to 48 bytes of arbitrary data
after the MD5 hash, and for SHA-1, up to 44 bytes.  However,
the length octet in the ASN.1 encoding of the hash must
match the length.  Because of the smaller number of bytes to
play with and the constraint on the length octet, it may be
impossible to implement an attack similar to bug 350640.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This form of forgery is one of the 9 test cases attached to bug 351079.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

It's e) garbage appended to the hash, inside the DER encoding 
  that is: header, DER(Algorithm OID, algorithm parameters, hash+garbage)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

oh, wait, are you suggesting something like this:

   header, DER(Algorithm OID, algorithm parameters, hash, garbage)

where the garbage is inside the sequence that includes the hash, but 
comes after the hash?

Comment 4

[Wan-Teh Chang]

It is

  header, DER(Algorithm OID, algorithm parameters, hash+garbage)

NSS 3.11.3 release candidate has this bug.

I also tried

  header, DER(Algorithm OID, algorithm parameters, hash, garbage)

Thunderbird says the signature is invalid.  I believe it fails in
the decoding of DigestInfo but didn't verify it in a debugger or
with printf statements.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

We have a forged cert for that test case.  
3.11.3 has been tested against it (certutil -V and vfychain)
and detects it.  So I conclude that one of these must be true:
a) test forgery is defective.  (I will investigate that)
b) you're testing a different code path than I have been testing
   (I've been testing CERT_VerifyCert).

Comment 6

[Wan-Teh Chang]

I tested email signature verification in Thunderbird.  This is the
code path (the line numbers may be slightly off because of the
printf statements and other changes I added):

1. Decoding of DigestInfo:

DecryptSigBlock(int * 0x0309fdfc, unsigned char * 0x027862ac, unsigned int 64, SECKEYPublicKeyStr * 0x0389ef00, const SECItemStr * 0x03759eb0, char * 0x0373dfa0) line 107
vfy_CreateContextPrivate(const SECKEYPublicKeyStr * 0x02d361d0, const SECItemStr * 0x03759eb0, int 20, const SECItemStr * 0x00000000, void * 0x0373dfa0) line 354 + 33 bytes
vfy_VerifyDataPrivate(const unsigned char * 0x038a17f8, int 461, const SECKEYPublicKeyStr * 0x02d361d0, const SECItemStr * 0x03759eb0, int 20, const SECItemStr * 0x00000000, void * 0x0373dfa0) line 602 + 25 bytes
VFY_VerifyData(unsigned char * 0x038a17f8, int 461, SECKEYPublicKeyStr * 0x02d361d0, SECItemStr * 0x03759eb0, int 20, void * 0x0373dfa0) line 621 + 31 bytes
NSS_CMSSignerInfo_Verify(NSSCMSSignerInfoStr * 0x03759e68, SECItemStr * 0x0375a4b8, SECItemStr * 0x00303894) line 469 + 38 bytes
NSS_CMSSignedData_VerifySignerInfo(NSSCMSSignedDataStr * 0x0276a228, int 0, NSSTrustDomainStr * 0x027452e0, int 4) line 699 + 17 bytes
THUNDERBIRD! 007ec136()
THUNDERBIRD! 007ec03a()
THUNDERBIRD! 007ef870()
THUNDERBIRD! 007ef9d6()
25e814ec()

2. PORT_Memcmp call that compares the decrypted hash with the alleged hash:

VFY_EndWithSignature(VFYContextStr * 0x027862a0, SECItemStr * 0x00000000) line 521
VFY_End(VFYContextStr * 0x027862a0) line 536 + 11 bytes
vfy_VerifyDataPrivate(const unsigned char * 0x038a17f8, int 461, const SECKEYPublicKeyStr * 0x02d361d0, const SECItemStr * 0x03759eb0, int 20, const SECItemStr * 0x00000000, void * 0x0373dfa0) line 610 + 9 bytes
VFY_VerifyData(unsigned char * 0x038a17f8, int 461, SECKEYPublicKeyStr * 0x02d361d0, SECItemStr * 0x03759eb0, int 20, void * 0x0373dfa0) line 621 + 31 bytes
NSS_CMSSignerInfo_Verify(NSSCMSSignerInfoStr * 0x03759e68, SECItemStr * 0x0375a4b8, SECItemStr * 0x00303894) line 469 + 38 bytes
NSS_CMSSignedData_VerifySignerInfo(NSSCMSSignedDataStr * 0x0276a228, int 0, NSSTrustDomainStr * 0x027452e0, int 4) line 699 + 17 bytes
THUNDERBIRD! 007ec136()
THUNDERBIRD! 007ec03a()
THUNDERBIRD! 007ef870()
THUNDERBIRD! 007ef9d6()
25e814ec()

Comment 7

[Daniel Veditz [:dveditz]]

Please assign this to a real owner (wtc? nelson?)

I'm confused by comment 4, NSS is vulnerable but Thunderbird is not?

Comment 8

[Wan-Teh Chang]

Attachment: Patch for NSS_3_11_BRANCH (Checked in)

This is one way to fix it.  I won't be able to work on
this bug this week.  Feel free to use this patch or write
your own patch.  If you use this patch, feel free to change
identifier names and indentation, etc.  I didn't have time
to work on coding style.

Comment 9

[Wan-Teh Chang]

Also, someone should examine VFY_VerifyDigest because of the XXX
comment in front of it.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

This bug, together with bug 350640 and bug 351079, are all vulnerabilities to 
malformed PKCS#1 v1.5 RSA signatures.  There are several code paths through
NSS that check these signatures.  One of them was fixed for bug 351079.
This bug reports another path that is vulnerable to one of the same cases as 
shown in that bug.  

It appears that the difference is that this path is used to verify
signatures on things other than certificates, whereas the other path was
used to verify signatures on certificates.  

I consider this to be part of the same vulnerability.  If NSS accepts the 
malformed signature, NSS is vulnerable to it, regardless of what kind of 
data (cert or other) is being signed.  Perhaps we should reopen bug 351079 
and make this bug a dup of that one.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

r=nelsonb.  Bob please SR.

There is one minor (nit) change I would make to this patch, but in 
the interest of time, I will refrain from changing it.
In the patch segment below, instead of the 3 "+" lines, I would have
coded this "*" line:

> 	    if (sig) {
> 		SECOidTag hashid = SEC_OID_UNKNOWN;
>-	    	rv = DecryptSigBlock(&hashid, cx->u.buffer,
>+		unsigned int digestlen = 0;
>+	    	rv = DecryptSigBlock(&hashid, cx->u.buffer, &digestlen,
>*              rv = DecryptSigBlock(&hashid, cx->u.buffer, &cx->rsadigestlen,
> 			HASH_LENGTH_MAX, cx->key, sig, (char*)wincx);
> 		cx->alg = hashid;
>+		cx->rsadigestlen = digestlen;
> 	    } else {

Comment 12

[Robert Relyea]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

r+=relyea

The u.buffer data really calls out to be a secitem which points to the u.buffer space. That should also simplify the DSA and ECDSA verification code.

Comment 13

[Robert Relyea]

I believe the issue here is that the forged signature case uses more data than the biggest HASH we support, so it is being detected. The reduced size means that it will likely be harder to construct a perfect cube in the space we allow, but 40 bytes is enough to make me worried. Especially if one starts trying to attack 1024 bit keys (which may need less 'garbage' than 2048 or 3072 bit keys).

Anyway I concur this should be fixed as part of the other signing issues.

bob

Comment 14

[Wan-Teh Chang]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

> 	    if (sig) {
> 		SECOidTag hashid = SEC_OID_UNKNOWN;
>-	    	rv = DecryptSigBlock(&hashid, cx->u.buffer,
>+		unsigned int digestlen = 0;
>+	    	rv = DecryptSigBlock(&hashid, cx->u.buffer, &digestlen,
> 			HASH_LENGTH_MAX, cx->key, sig, (char*)wincx);
> 		cx->alg = hashid;
>+		cx->rsadigestlen = digestlen;
> 	    } else {

The use of the local variable 'digestlen' is intended to
bring your attention to the similar use of 'hashid' in
the original code.

Feel free to eliminate 'digestlen' and pass &cx->rsadigestlen
directly, but please also eliminate 'hashid' and pass &cx->alg
directly.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

approved for the 1.8.0 branch, a=dveditz for drivers

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Checked in on NSS_3_11_BRANCH

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

Now the owners of the mozilla branches need to port this to their branches.  

lib/cryptohi/secvfy.c; new revision: 1.16.2.4; previous revision: 1.16.2.3

I will work on preparing a patch for the trunk next.
This file is VERY different on the trunk.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch for trunk

Since this file is already SO different between trunk and branch,
this patch for the trunk differs significantly from the branch patch.
I took the liberty of renaming one variable to avoid ambiguity, 
and I also took Wan-Teh's suggestion about the hashid variable.

This patch is not blocking any other mozilla branches.

Comment 19

[Wan-Teh Chang]

Attachment: Patch for NSS_3_11_BRANCH v2 (Checked in)

I renamed a function parameter and added a comment.
There is no real change.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237536 [details] [diff] [review]
Patch for NSS_3_11_BRANCH  v2 (Checked in)

groovy.

Comment 21

[Robert Relyea]

Comment on attachment 237443 [details] [diff] [review]
Patch for trunk

r+ = rrelyea.
The patch explains why hashid was separate, and handles the new semantic correctly.

bob

Comment 22

[Daniel Veditz [:dveditz]]

Attachment: merged to client 1.8.0 branch

Nelson, please give this a once-over to make sure the merge is OK.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

This one works for the 1.8 branch.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 237536 [details] [diff] [review]
Patch for NSS_3_11_BRANCH  v2 (Checked in)

ought to take this too just in case we have to merge something else in here later on.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237546 [details] [diff] [review]
merged to client 1.8.0 branch

I'm giving this a qualified r+ for the 1.8.0 branch.

I believe this patch accurately carries the change from 
the NSS_3_11_BRANCH back to this older branch.

However, because there are other preceeding changes 
from the NSS_3_11_BRANCH that have not been carried back
I cannot state with any certainty that, with this patch
applied, the 1.8.0 branch will be as free of vulnerabilities
as the 3_11_BRANCH is now.

Comment 26

[Wan-Teh Chang]

Attachment: merged to client 1.8.0 branch v2

Dan, your patch is correct.  I renamed the rsadigestlen field of
VFYContext because 'digestlen' looks nicer on the MOZILLA_1_8_0_BRANCH
(and NSS_3_10_BRANCH).

Comment 27

[Mike Schroepfer]

Comment on attachment 237536 [details] [diff] [review]
Patch for NSS_3_11_BRANCH  v2 (Checked in)

a=schrep

Comment 28

[Daniel Veditz [:dveditz]]

Checked in attachment 237536 [details] [diff] [review] to the 1.8 branch for 1.8.1

Comment 29

[Wan-Teh Chang]

Verified with Thunderbird 1.5.0.7 rc6 (compared with 1.5.0.5,
which has the bug).

Verified with Thunderbird 2 alpha 1 20060911 (compared with
build 20060910, which has the bug).

Comment 30

[Wan-Teh Chang]

Attachment: Patch for trunk v2

I added my new comment for digestlen and renamed the 'len'
parameter of DecryptSigBlock.

You missed the change to one PORT_Memcmp call.  If it's
intentional, please remove that change from this patch.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for the old NSS_3_3_4_1_BRANCH, v1

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 237878 [details] [diff] [review]
Patch for trunk v2

r=nelson

Comment 33

[Robert Relyea]

Comment on attachment 237878 [details] [diff] [review]
Patch for trunk v2

sigh, good catch wan-teh.
r=relyea

Comment 34

[Wan-Teh Chang]

Attachment: Patch for trunk v3 (as checked in)

I made two additional changes to vfy_CreateContext.
1. I removed a break statement that's no longer necessary.
2. I fixed a leak of 'cx' on an error return.  Coverity
   should be able to catch this leak, but Coverity was only
   run on some NSS branch used by Mozilla, and this code is
   new on the trunk only.

Please review these two changes.  I've checked in this patch
on the NSS trunk for NSS 3.12.

Checking in secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.20; previous revision: 1.19
done

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS_3_3_4_1_BRANCH, v2

This revision fixes a problem due to the added declaration of QuickDER
coming from a different revision than the source to QuickDER.  The MSVC 
compiler didn't complain, but Sun's compiler rightly called it an error.

Comment 36

[Christophe Ravel]

The build is passing on Solaris now. I am going to run some tests with all.sh.

Comment 37

[Christophe Ravel]

Comment on attachment 238134 [details] [diff] [review]
patch for NSS_3_3_4_1_BRANCH, v2

Build and tests are passing on Solaris with this patch.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 238134 [details] [diff] [review]
patch for NSS_3_3_4_1_BRANCH, v2

Fix committed on the NSS_3_3_4_1_BRANCH. 

cmd/lib/SECerrs.h;           new : 1.1.148.1;     previous : 1.1
lib/cryptohi/secvfy.c;       new : 1.5.54.1;      previous : 1.5
lib/softoken/pkcs11c.c;      new : 1.13.2.2.2.2;  previous : 1.13.2.2.2.1
lib/util/manifest.mn;        new : 1.4.108.1;     previous : 1.4
lib/util/quickder.c;         new : 1.23.20.1;     previous : (added)
lib/util/secasn1.h;          new : 1.4.56.1;      previous : 1.4
lib/util/secasn1t.h;         new : 1.4.48.1.46.2; previous : 1.4.48.1.46.1
lib/util/seccomon.h;         new : 1.2.118.1;     previous : 1.2
lib/util/secdig.c;           new : 1.2.200.1;     previous : 1.2
lib/util/secerr.h;           new : 1.3.64.1;      previous : 1.3
tests/qa_stat;               new : 1.21.50.1;     previous : 1.21
tests/cipher/cipher.sh;      new : 1.5.124.1;     previous : 1.5
tests/cipher/performance.sh; new : 1.2.220.1;     previous : 1.2
tests/ssl/ssl.sh;            new : 1.38.52.2;     previous : 1.38.52.1

Is this bug ready to be marked resolved/fixed?

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Related to or inspired by CVE-2006-4340 : unchecked garbage in PKCS v1.5 blocks