Bug 488253 - com.netscape.cmsutil.ocsp.BasicOCSPResponse ASN.1 encoding/decoding is broken
Summary: com.netscape.cmsutil.ocsp.BasicOCSPResponse ASN.1 encoding/decoding is broken
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Dogtag Certificate System
Classification: Retired
Component: Tools - Java
Version: 1.0
Hardware: All
OS: Linux
high
medium
Target Milestone: ---
Assignee: Ade Lee
QA Contact: Chandrasekar Kannan
URL:
Whiteboard:
Depends On:
Blocks: 445047
TreeView+ depends on / blocked
 
Reported: 2009-03-03 15:04 UTC by David Stutzman
Modified: 2015-01-04 23:36 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
: 550331 (view as bug list)
Environment:
https://bugzilla.redhat.com/show_bug.cgi?id=550331
Last Closed: 2012-06-04 20:27:31 UTC
Embargoed:


Attachments (Terms of Use)
a DER encoded com.netscape.cmsutil.ocsp.OCSPResponse (1.08 KB, application/unknown)
2009-03-03 15:04 UTC, David Stutzman
no flags Details
patch for making ASN.1 encoding/decoding more complete (2.53 KB, patch)
2009-12-23 14:35 UTC, David Stutzman
david.k.stutzman2.ctr: review?
Details | Diff
patch including dstudzman fixes and others (17.13 KB, patch)
2010-11-08 04:39 UTC, Ade Lee
alee: review+
Details | Diff

Description David Stutzman 2009-03-03 15:04:57 UTC
Created attachment 333876 [details]
a DER encoded com.netscape.cmsutil.ocsp.OCSPResponse

Description of problem:
The BasicOCSPResponse class has a private field _certs which is an array of Certificates.  It also has the methods getCertsCount() and getCertificateAt(int pos).  If you construct a BasicOCSPResponse by decoding some bytes with this class's template and you call either of the latter methods, you will get a NullPointerException.  The problem is the decoding template completely ignores the optional certificates field of the response and always initializes the object by passing null as the 4th argument of the constructor.

Since I don't have contributor privileges, I will explain in plain words how to fix the class so it supports such responses.  I edited the source file myself and am currently using it in my own OCSP client so it at least works for me performing the following edits:

In the Template inner-class's decode function, after the line:
BIT_STRING bs = (BIT_STRING)seq.elementAt(2);
and before the line:
return new BasicOCSPResponse(rd, alg, bs, null);

the following needs to be done:
1 - get the size of the SEQUENCE "seq", 4 means that the certificates sequence is present and the following needs to be done
2 - get the 4th element of the "seq" object , cast it, and store it in an org.mozilla.jss.asn1.EXPLICIT
3 - get the content of the EXPLICIT, cast it, and store it as a org.mozilla.jss.asn1.SEQUENCE
4 - create an array of org.mozilla.jss.pkix.cert.Certificate that is the same size as the sequence you just created
5 - loop through the SEQUENCE, getting elements and putting them into the certs array, casting as appropriate
6 - call the BasicOCSPResponse constructor, and instead of passing null as the 4th argument, pass the newly created array of Certificates

Steps 2 through 6 should be in an if block depending on the size returned in step 1.

Additionally, I edited the getCertCounts() method to return 0 if _certs is null and I edited getCertificateAt(int pos) to return null if _certs is null as well.

Someone also might want to look at the getBytes() method of this class implemented as part of the com.netscape.cmsutil.ocsp.Response Interface.  It is hardcoded to return null.  This is another method that I called and was suprised to get a null for.  What I *assumed* this method was returning was the bytes of the ResponseData so that I could verify the signature of the response.  I'm not sure what was intended to return but null probably isn't it.

I will attach a DER-encoded OCSP response that these changes can be tested against.

To test:
1- read the bytes from file into a byte[] responseBytes
2- OCSPResponse ocspResponse = (OCSPResponse) ASN1Util.decode(OCSPResponse.getTemplate(), responseBytes);
3- ResponseBytes respBytes = ocspResponse.getResponseBytes();
4- BasicOCSPResponse brep = (BasicOCSPResponse) ASN1Util.decode(BasicOCSPResponse.getTemplate(), respBytes.getResponse().toByteArray());
5- try to get some info about the certs...

Comment 1 David Stutzman 2009-03-03 15:39:13 UTC
If you want, I can file another bug, but I was just trying to encode a BasicOCSPResponse and I get a NullPointerException in this method:

  public void encode(Tag t, OutputStream os) throws IOException {
    os.write(mData);
  }

This is most likely because the decoding Template/Constructor doesn't populate the mData byte[].  It seems this field is intended to store the encoded form of this object.  The 2 encode methods should most likely be implemented differently.

Looking at some existing encoding methods in JSS:
http://mxr.mozilla.org/security/source/security/jss/org/mozilla/jss/pkcs12/PFX.java

The one without the Tag param calls the one that takes a tag passing the one implicitly defined in the class.

This second encode method adds all the pieces to the SEQUENCE and then calls the encode method on that.  The mData field should probably be removed.

Comment 2 David Stutzman 2009-03-04 15:21:47 UTC
fixes for the 2 encoding methods:
for "public void encode(OutputStream os)"
just call the encode method with 2 params passing the static field "TAG" as the first param and the passed in os as the second

for "public void encode(Tag t, OutputStream os)"
perform the following:
1 - create a new SEQUENCE
2 - add the ResponseData field to the SEQUENCE
3 - add the AlgorithmIdentifier field to the SEQUENCE
4 - add the BIT_STRING field to the SEQUENCE
5 - if the _certs field is not null:
    - create a new SEQUENCE
    - loop through the _certs array and add each cert to the SEQUENCE
    - create a new EXPLICIT with a Tag of 0 and the content of the certs SEQUENCE
    - add the certs SEQUENCE to the above sequence
6 - call the encode method on the main SEQUENCE object passing the Tag and OutputStream args from the method params

I did some tests encoding/decoding things and while comparing the ASN.1 noticed the following non-fatal discrepancies in the ResponseData class: 
- My original test response doesn't contain the version (which is optional) in the ResponseData.  When I encode the BasicOCSPResponse (which in turn encodes the ResponseData inside it), it adds it.
- My original test response contains a nextUpdate field in the ResponseData.  When I encode it, it is left out.  I didn't look to see if the ResponseData class keeps track and stores the optional field or not for later use (my guess based on experience with this one is that it ignores it).

Comment 3 David Stutzman 2009-07-10 14:50:32 UTC
OK...in addition to the fixes above, I recently did the following:
ResponseData.java:
- Incorrectly encoded the default version, DER encoding rules specify that if a value is the default, it should be omitted
- support in the class for response extensions is lacking
SingleResponse.java:
- Decoding template ignores the optional values of nextUpdate and singleExtensions
- support in the whole class for singleExtension is lacking

Comment 5 David Stutzman 2009-12-23 14:35:30 UTC
Created attachment 380035 [details]
patch for making ASN.1 encoding/decoding more complete

Comment 6 David Stutzman 2009-12-23 14:40:19 UTC
Let me know if the format and such that I used for this is appropriate and I'll continue to make patches for the rest of the com.netscape.cmsutil.ocsp package files that I fixed up.  Also, should I file a bug per file or just toss them all into this bug?

Comment 7 Dmitri Pal 2009-12-23 17:33:34 UTC
David,

Thank you for the patch! Please continue. We will review and come back if there any suggestions about the patch or corrections that should be made.
Due to the holiday season our response might be a bit slow. 

It makes sense to have a bug per issue and a patch per bug. This way the issues can be reproduced and then the patch can be applied to verify that the issue is addressed.

Comment 8 Ade Lee 2010-11-08 04:39:35 UTC
Created attachment 458575 [details]
patch including dstudzman fixes and others

patch includes fixes for this bug and for :

https://bugzilla.redhat.com/show_bug.cgi?id=551410
https://bugzilla.redhat.com/show_bug.cgi?id=550331

Tested and proved to work with dstutz's ocspresponse data.
Also tested with NSS (ocsp enabled), and firefox OCSP checking.

awnuk, please review

Comment 9 Andrew Wnuk 2010-11-08 21:32:34 UTC
attachment 458575 [details]  +awnuk   (with small correction that we have discussed)

Comment 10 Ade Lee 2010-11-09 05:36:15 UTC
8.1:

[vakwetu@goofy-vm4 util]$ svn ci -m "BZ488253, BZ551410, BZ550331 - oscp asn1 encoding fixes, including code provided by David Studzman"
Sending        util/src/com/netscape/cmsutil/ocsp/BasicOCSPResponse.java
Sending        util/src/com/netscape/cmsutil/ocsp/ResponseData.java
Sending        util/src/com/netscape/cmsutil/ocsp/TBSRequest.java
Transmitting file data ...
Committed revision 1481.

tip:

Not yet committed -- some kind of network issue with svn ..

Comment 11 Ade Lee 2010-11-09 05:48:54 UTC
tip: 

[vakwetu@dhcp231-121 util]$  svn ci -m "BZ488253, BZ551410, BZ550331 - oscp asn1 encoding fixes, including code provided by David Studzman"
Sending        util/src/com/netscape/cmsutil/ocsp/BasicOCSPResponse.java
Sending        util/src/com/netscape/cmsutil/ocsp/ResponseData.java
Sending        util/src/com/netscape/cmsutil/ocsp/TBSRequest.java
Transmitting file data ...
Committed revision 1482.

Comment 12 Ade Lee 2010-11-18 21:29:21 UTC
Comment on attachment 458575 [details]
patch including dstudzman fixes and others

reviewed by awnuk


Note You need to log in before you can comment on or make changes to this bug.