613670 - PBKDF2 iteration count sometimes treated as negative

Status: RESOLVED DUPLICATE of bug 572289

(NSS :: Libraries, defect)

Description

[Greg Hudson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.44 Safari/534.7
Build Identifier: 3.12.6

If you perform a PBKDF2 key derivation using PK11_CreatePBEV2AlgorithmID() and PK11_PBEKeyGen() with an iteration count of 32768 (or any number of other integers), it may fail because the iteration count becomes negative in the process of encoding it into ASN.1 and back.

The iteration count is encoded at pk11pbe.c:547 using
SEC_ASN1EncodeInteger().  This function treats any integer with the high bit set in its first byte as negative, so for instance it encodes 32768 as {128, 0} (which means -32768).  When that string is decoded at pk11pbe.c:818 with DER_GetInteger(), the iteration count is set to a negative number, which leads to a failure.

A targeted solution to this bug is to use 
SEC_ASN1EncodeUnsignedInteger for the iteration count, since there are no reasonable negative iteration counts (even though the iteration count is commonly held in a signed field).  However, I'd also question whether a caller would ever want the current behavior of SEC_ASN1EncodeInteger(), and whether every other call site of that function is also a bug nest.  More consistent behavior might come from telling sec_asn1e_integer() whether the quantity being passed to it is non-negative, instead of unsigned, and have SEC_ASN1EncodeInteger() pass (value >= 0) instead of FALSE for that flag.


Reproducible: Always

Comment 1

[Robert Relyea]

Attachment: Decode the int's as unsigned values in the pbe's

Comment 2

[Wan-Teh Chang]

Comment on attachment 502959 [details] [diff] [review]
Decode the int's as unsigned values in the pbe's

r=wtc.

Comment 3

[Wan-Teh Chang]

Comment on attachment 502959 [details] [diff] [review]
Decode the int's as unsigned values in the pbe's

On second thought, I prefer Greg's suggested change to
SEC_ASN1EncodeInteger.  I believe "unsigned" actually
refers to the C integer type of the input ('unsigned long')
as opposed to the DER encoded output.  This opinion is
supported by the check at the beginning of DER_GetUInteger
that disallows a most significant bit of 1:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/dersubr.c&rev=1.6&mark=243,250-254#238
This shows the most significant bit in an DER encoded
integer is always the sign bit.

This patch will create an inconsistency between encoding
and decoding because we will be calling SEC_ASN1EncodeUnsignedInteger
and DER_GetInteger (as opposed to DER_GetUInteger).

Comment 4

[Wan-Teh Chang]

This problem with SEC_ASN1EncodeInteger turns out to be a known bug.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,  As you've discovered, DER encoded integers are always signed. 
Whenever you see the word signed or unsigned used in out APIs in the 
context of ASN INTEGERs, it refers to the UNencoded/DEcoded value.  
It tells the encoder/decoder whether the high order bit of the first byte 
of unencoded input, or decoded output is a sign bit or not.  Mostly this
tells the encoder whether or not to add a leading zero byte (when necessary)
to keep the encoded value positive.  It also tells the decoder whether or not 
to eliminate any leading zero bytes from the decoded output.