Re: [Pki-devel] Certificate Transparency SCT signature verification?

HI Fraser, verifySCT still fails. I still think the fact the rfc does not require the signed object to accompany the signature presents undue challenge to the party that needs to verify the signature. Although I understand that this is v1, and the issue would not be present in v2 since there will not be poison extension ;-/. I'd appreciate it if you could find time to take a closer look. Here is my latest attempt: https://github.com/dogtagpki/pki/pull/440 Since it's a patch against the latest code, for a full view, it would be easier if you just apply the patch and read from "(Certificate Transparency)" in base/ca/src/com/netscape/ca/CAService.java This patch would require JSS change at: https://github.com/dogtagpki/jss/pull/575 Code still requires some refinement but I wish to address the verification issue before cleaning things up. Of course I still let verifySCT returns success for now just so people could still play with CT. Much appreciated! Christina On Tue, Jun 2, 2020 at 3:05 PM Christina Fu <cfu(a)redhat.com&gt; wrote: > Hi Fraser, > Thanks for the response! > Regarding the poison extension, yes I was aware that it needed to be > removed so the code already had it removed. It was the order of things > left inside tbsCert that I was concerned about since I used the existing > delete method provided for the Extension class, which I wasn't sure if it'd > preserve the order of the remaining extensions. Thanks for confirming my > suspicion. I will double-check the order. > > Also thanks for the input on how to handle failed CT log communication > v.s. response verification failure. I will address them separately as > suggested. > Finally, nice catch with the missing data length!! I'll add that and go > from there. > > thanks again! > Christina > > On Mon, Jun 1, 2020 at 7:31 PM Fraser Tweedale <ftweedal(a)redhat.com&gt; > wrote: > >> Hi Christina, >> >> Adding pki-devel@ for wider audience. Comments below. >> >> On Mon, Jun 01, 2020 at 06:28:42PM -0700, Christina Fu wrote: >> > Hi Fraser, >> > Do you know how the signature returned in the SCT response could be >> > verified by the CA? >> > My thought is that the CA should somehow verify the CT response after >> > sending the add-pre-chain request and before signing the cert. Since >> log >> > inclusion verification would not be feasible right after the request >> (the >> > SCT response is supposed to be just a "promise, according to the rfc), >> I >> > ruled that out and intend to stay with just the following two >> verifications >> > on the response itself: >> > >> > - checking if log id (CT log signer public key hash) returned in the >> CT >> > response is correct >> > - this I have no problem verifying >> > - Verifying if the signature returned in the CT response is >> correct >> > - this I can't seem to get it working. >> > >> > I put the verification work above in the method "verifySCT". >> > >> https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/... >> > What I am wondering is how this can be done properly. Since the >> tbsCert is >> > not to contain the poison extension, the poison extension needs to be >> > removed by the CT server before it signs. What if the order of the >> > extensions contained in the tbsCert gets changed in the process? >> > >> The poison extension must be removed, and care must be taken to keep >> everything else in the same order, and reserialise the parts in >> exactly the same way. >> >> > It seems that the response should contain the tbsCert that it signs >> (which >> > isn't per the rfc) or I am not sure how the CA could verify the >> signature. >> > >> The response does not contain the TBCCertificate. Both sides (log >> and client) remove the poison extension (and change nothing else), >> then both sides can sign the same data. >> >> > So the question now is if I should just leave out the check, unless you >> > have other suggestions. >> > >> I do think we should verify the signature, to ensure the message was >> actually received by the log server we wanted and not an impostor. >> >> > Of course, I also could have missed something in my code. >> > >> The binary format is complex and it's easy to miss something. After >> you implement removal of the poison extension, if it is still not >> working I will go over the code with a fine-tooth comb. >> >> I copied some of the code and left comments below, too. Comments >> begin with `!!'. I think I found one bug and a couple of possible >> improvements. >> >> > One last question, currently in the code, if verifySCT fails, I just >> > "continue" to process for next CT log. Should this just bail out all >> > together or it's fine to continue? Or could this be a choice by the >> admin. >> > What do you think and why? >> > >> https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/... >> > >> My line of thinking is this: >> >> - we should tolerate communication errors with log (perhaps >> enqueuing the cert for a retry later) >> >> - but (assuming we implement it correctly) verifySCT failure is >> indicative of something wrong with the log (e.g. key changed); it >> is not a communication error and can be treated differently. >> >> - I think it's OK to fail hard. Admins will likely want to know if >> something is wrong with CT logging. >> >> - But in case we make a mistake, or an org needs issuance to >> continue despite CT log misbehaviour, there should be a config >> knob to allow this condition to be ignored. "In case of >> emergency..." >> >> >> > >> > thanks, >> > Christina >> >> boolean verifySCT(CTResponse response, byte[] tbsCert, String >> logPublicKey) >> throws Exception { >> >> /* ... SNIP ... */ >> >> byte[] extensions = new byte[] {0, 0}; >> !! although no extensions have been defined I think we should we take >> extensions from the CT response to future-proof this code. i.e. >> decode the base64 data from the "extensions" field, and prepend the >> length. >> >> // piece them together >> >> int data_len = version.length + signature_type.length + >> timestamp.length + entry_type.length + >> issuer_key_hash.length + tbsCert.length + >> extensions.length; >> >> logger.debug(method + " data_len = "+ data_len); >> ByteArrayOutputStream ostream = new ByteArrayOutputStream(); >> >> ostream.write(version); >> ostream.write(signature_type); >> ostream.write(timestamp); >> >> ostream.write(entry_type); >> ostream.write(issuer_key_hash); >> ostream.write(tbsCert); >> !! I believe you need to prepend the length of tbsCert as a >> THREE-byte length field, because its definition is >> `opaque TBSCertificate<1..2^24-1>;' >> >> ostream.write(extensions); >> >> byte[] data = ostream.toByteArray(); >> >> // Now, verify the signature >> // Note: this part currently does not work; see method comment >> above >> >> // cfu ToDo: interpret the alg bytes later; hardcode for now >> Signature signer = Signature.getInstance("SHA256withEC", >> "Mozilla-JSS"); >> signer.initVerify(log_pubKey); >> signer.update(data); >> !! We could call signer.update() multiple times instead of making an >> intermediate ByteArrayOutputStream. I do not care about the >> performance, just whatever might simplify the routine. >> >> if (!signer.verify(signature)) { >> logger.error(method + "failed to verify SCT signature; pass >> for now"); >> // this method is not yet working; Let this pass for now >> // return false; >> } else { >> logger.debug(method + "SCT signature verified successfully"); >> } >> logger.debug("verifySCT ends"); >> >> return true; >> } >> >> >> >> Cheers, >> Fraser >> >>