Red Hat Bugzilla – Bug 488253
com.netscape.cmsutil.ocsp.BasicOCSPResponse ASN.1 encoding/decoding is broken
Last modified: 2015-01-04 18:36:54 EST
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...
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.
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).
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
Created attachment 380035 [details] patch for making ASN.1 encoding/decoding more complete
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?
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.
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
attachment 458575 [details] +awnuk (with small correction that we have discussed)
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 ..
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 on attachment 458575 [details] patch including dstudzman fixes and others reviewed by awnuk