The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 3.11.3

Status

NSS
Libraries
P1
critical
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

({verified1.8.0.7, verified1.8.1})

3.11.2
3.11.3
verified1.8.0.7, verified1.8.1
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(4 attachments, 5 obsolete attachments)

4.70 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
4.03 KB, patch
Details | Diff | Splinter Review
8.02 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
69.64 KB, patch
Christophe Ravel
: review+
Julien Pierre
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

11 years ago
This form of forgery is one of the 9 test cases attached to bug 351079.
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

11 years ago
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()
Please assign this to a real owner (wtc? nelson?)

I'm confused by comment 4, NSS is vulnerable but Thunderbird is not?
Flags: blocking1.8.1?
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7?
Whiteboard: [sg:investigate]
(Reporter)

Comment 8

11 years ago
Created attachment 237388 [details] [diff] [review]
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.
(Reporter)

Comment 9

11 years ago
Also, someone should examine VFY_VerifyDigest because of the XXX
comment in front of it.
Assignee: nobody → nelson
(Assignee)

Comment 10

11 years ago
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.
Severity: normal → blocker
Priority: -- → P1
Whiteboard: [sg:investigate] → [sg:high]
Target Milestone: --- → 3.11.3
(Assignee)

Comment 11

11 years ago
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 {
Attachment #237388 - Flags: superreview?(rrelyea)
Attachment #237388 - Flags: review+

Comment 12

11 years ago
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.
Attachment #237388 - Flags: superreview?(rrelyea) → superreview+

Comment 13

11 years ago
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
Severity: blocker → normal
Priority: P1 → --
Target Milestone: 3.11.3 → ---

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Reporter)

Comment 14

11 years ago
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.
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.7?
Flags: blocking1.8.0.7+
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
Attachment #237388 - Flags: approval1.8.0.7+
(Assignee)

Comment 16

11 years ago
Checked in on NSS_3_11_BRANCH
Severity: normal → critical
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.11.3
(Assignee)

Comment 17

11 years ago
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.
Attachment #237388 - Attachment description: Patch → Patch for NSS_3_11_BRANCH (Checked in)
(Assignee)

Comment 18

11 years ago
Created attachment 237443 [details] [diff] [review]
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.
Attachment #237443 - Flags: superreview?(wtchang)
Attachment #237443 - Flags: review?(rrelyea)
(Reporter)

Comment 19

11 years ago
Created attachment 237536 [details] [diff] [review]
Patch for NSS_3_11_BRANCH  v2 (Checked in)

I renamed a function parameter and added a comment.
There is no real change.
Attachment #237388 - Attachment is obsolete: true
(Assignee)

Comment 20

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

groovy.
Attachment #237536 - Flags: review+

Comment 21

11 years ago
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
Attachment #237443 - Flags: review?(rrelyea) → review+
Created attachment 237546 [details] [diff] [review]
merged to client 1.8.0 branch

Nelson, please give this a once-over to make sure the merge is OK.
Attachment #237546 - Flags: review?
Attachment #237546 - Flags: approval1.8.0.7+
Attachment #237546 - Flags: review?(wtchang)
Attachment #237546 - Flags: review?(nelson)
Attachment #237546 - Flags: review?
Attachment #237388 - Flags: approval1.8.0.7+
Comment on attachment 237388 [details] [diff] [review]
Patch for NSS_3_11_BRANCH (Checked in)

This one works for the 1.8 branch.
Attachment #237388 - Flags: approval1.8.1?
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.
Attachment #237536 - Flags: approval1.8.1?
(Assignee)

Comment 25

11 years ago
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.
Attachment #237546 - Flags: review?(nelson) → review+
Attachment #237388 - Flags: approval1.8.1?
(Reporter)

Comment 26

11 years ago
Created attachment 237556 [details] [diff] [review]
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).
Attachment #237546 - Attachment is obsolete: true
Attachment #237546 - Flags: review?(wtchang)
Keywords: fixed1.8.0.7

Comment 27

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

a=schrep
Attachment #237536 - Flags: approval1.8.1? → approval1.8.1+
Checked in attachment 237536 [details] [diff] [review] to the 1.8 branch for 1.8.1
Keywords: fixed1.8.1
(Reporter)

Comment 29

11 years ago
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).
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
(Reporter)

Comment 30

11 years ago
Created attachment 237878 [details] [diff] [review]
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.
Attachment #237443 - Attachment is obsolete: true
Attachment #237443 - Flags: superreview?(wtchang)
(Assignee)

Comment 31

11 years ago
Created attachment 237879 [details] [diff] [review]
patch for the old NSS_3_3_4_1_BRANCH, v1
Attachment #237879 - Flags: review?(julien.pierre.bugs)

Updated

11 years ago
Attachment #237879 - Flags: review?(julien.pierre.bugs) → review+
(Assignee)

Comment 32

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

r=nelson
Attachment #237878 - Flags: superreview?(rrelyea)
Attachment #237878 - Flags: review+

Comment 33

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

sigh, good catch wan-teh.
r=relyea
Attachment #237878 - Flags: superreview?(rrelyea) → superreview+
(Reporter)

Comment 34

11 years ago
Created attachment 238003 [details] [diff] [review]
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
Attachment #237878 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #238003 - Flags: review+
(Assignee)

Comment 35

11 years ago
Created attachment 238134 [details] [diff] [review]
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.
Attachment #237879 - Attachment is obsolete: true
Attachment #238134 - Flags: superreview?(julien.pierre.bugs)
Attachment #238134 - Flags: review?(christophe.ravel.bugs)

Comment 36

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

Comment 37

11 years ago
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.
Attachment #238134 - Flags: review?(christophe.ravel.bugs) → review+

Updated

11 years ago
Attachment #238134 - Flags: superreview?(julien.pierre.bugs) → superreview+
(Assignee)

Comment 38

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

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Group: security
Related to or inspired by CVE-2006-4340 : unchecked garbage in PKCS v1.5 blocks
You need to log in before you can comment on or make changes to this bug.