386351 - SignerInfo class inserts wrong version # into the resulting structure

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect, P1)

Description

[David Stutzman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: JSS 4.2

The CMS rfc (http://www.faqs.org/rfcs/rfc3852.html) says the following about SignerInfo version:
     "version is the syntax version number.  If the SignerIdentifier is
      the CHOICE issuerAndSerialNumber, then the version MUST be 1.  If
      the SignerIdentifier is subjectKeyIdentifier, then the version
      MUST be 3."

I have constructed a SignerInfo object passing in a SignerIdentifier as the first argument that was constructed with an IssuerAndSerialNumber and the resulting structure has a version of 3 when it should have a version of 1 according to the spec.

Reproducible: Always

Steps to Reproduce:
1. construct an IssuerAndSerialNumber
2. construct a SignerIdentifier using the IssuerAndSerialNumber you just created
3. construct a SignerInfo using the SignerIdentifier you just created
4. call getVersion() on the resulting SignerInfo object and get a 3
Actual Results:  
SignerInfo.getVersion() returns a 3

Expected Results:  
SignerInfo.getVersion() returns a 1

I haven't tested what version you get if you make a SignerIdentifier with a SubjectKeyIdentifier.

Comment 1

[David Stutzman]

Attachment: 386351.patch

Fix versioning of SignerInfo to match CMS spec.

Tested with constructing a SignerInfo with a SignerIdentifier that is an IASN and calling SignerInfo.getVersion() which returns 1 now and constructing with a SignerIdentifier that is a SKI yields version 3.  Also encoded and decoded the objects and correct versions persist.

Comment 2

[Christina Fu]

Comment on attachment 8942262 [details] [diff] [review]
386351.patch

Review of attachment 8942262 [details] [diff] [review]:
-----------------------------------------------------------------

logic looks reasonable.  My only suggestion is to add null check for signerIdentifier.  I see the existing code doesn't do much of that, but since you are dereferencing it, it would be a safer practice.

Comment 3

[David Stutzman]

Attachment: 386351.patch

Added null-check for signerIdentifier param as noted from code review.

Comment 4

[Christina Fu]

Comment on attachment 8942656 [details] [diff] [review]
386351.patch

Review of attachment 8942656 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Dave, thanks for the update.  I have trouble applying the patch to the latest tip.  Did you make sure that you did a pull before creating the patch?  Please let me know.  thanks.

Comment 5

[David Stutzman]

Attachment: 386351.patch

Sorry, working on an offline box.  New patch against current tip (8746a3fc7478)

Comment 6

[Christina Fu]

pushed:
https://hg.mozilla.org/projects/jss/rev/1d858c6d4626b625bb671426e6899d98c2f5bb2e