830781 - org.mozilla.jss.pkix.primitive.AlgorithmIdentifier decode/encode process alters original data

Status: RESOLVED WONTFIX

(JSS Graveyard :: Library, defect)

Description

[David Stutzman]

If you have an AlgorithmIdentifier in encoded form such as:
SEQUENCE {
    OBJECT IDENTIFIER '1.2.3.4'
}

decode it into an org.mozilla.jss.pkix.primitive.AlgorithmIdentifier object then encode that back into bytes you will end up with:
SEQUENCE {
    OBJECT IDENTIFIER '1.2.3.4'
    NULL
}

This is due to the decoding template in AlgorithmIdentifier class which takes the optional second element of the decoded sequence (which would be a normal Java null when presented with encoded form of my first example) and calls the 2 parameter AlgorithmIdentifier constructor (http://mxr.mozilla.org/security/source/security/jss/org/mozilla/jss/pkix/primitive/AlgorithmIdentifier.java#103).   Doing this will turn the java null into a JSS PKIX NULL object (http://mxr.mozilla.org/security/source/security/jss/org/mozilla/jss/pkix/primitive/AlgorithmIdentifier.java#43) and add it to the sequence to be encoded should that method then be called.

Apparently this code has been like this since first checked in many years ago and as such would affect all versions of Mozilla's JSS.  If you are working with DER encoded data such as this you end up altering it slightly if you decode/re-encode pieces of it and could end up invalidating signatures on signed data since it has been modified.

I'm not on development machine to generate a valid patch but a fix would be as simple as checking the actual value of seq.elementAt(1) in the decode method (http://mxr.mozilla.org/security/source/security/jss/org/mozilla/jss/pkix/primitive/AlgorithmIdentifier.java#105) and if the value is a Java null, meaning nothing at all was in the encoded form in the first place, then call the 1 arg constructor, else call the 2 arg constructor.

A simple test case is to construct an AlgorithmIdentifier with the 1 arg constructor and encode it to a byte[].  Decode the byte[] back into AlgorithmIdentifier object and encode to a second byte[]. Compare the resulting byte[] to the original.  You will see the second byte array has an additional ASN.1 encoded NULL (hex bytes: 05 00) in it.

Comment 1

[David Stutzman]

Attachment: 830781.patch

Comment 2

[David Stutzman]

Patch fixes naive decoding.

Tested with constructing an AlgorithmIdentifier using the single parameter constructor and the encoded form stays the same if the object is encoded/decoded/encoded.  An object constructed with the two parameter version of the constructor stays the same as well (as it did before).

The following test case can be used to check before/after:
AlgorithmIdentifier constructed = new AlgorithmIdentifier(new OBJECT_IDENTIFIER("1.2.3.4"));
ByteArrayOutputStream baos = new ByteArrayOutputStream();
constructed.encode(baos);
byte[] algIdBeforeBytes = baos.toByteArray();
AlgorithmIdentifier decoded = (AlgorithmIdentifier) ASN1Util.decode(AlgorithmIdentifier.getTemplate(), algIdBeforeBytes);
baos = new ByteArrayOutputStream();
decoded.encode(baos);
byte[] algIdAfterBytes = baos.toByteArray();
assertThat(Arrays.areEqual(algIdBeforeBytes, algIdAfterBytes), is(true));

Comment 3

[BMO Automation]

JSS development has moved from the Mozilla community to the Dogtag PKI community. Please re-file this bug at https://github.com/dogtagpki/jss if it is still relevant. Thank you!