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

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 > >