<div dir="ltr"><div dir="ltr"><div>Hi Fraser,</div><div>Thanks for the response! </div><div> 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.</div><div> </div><div>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.</div><div>Finally, nice catch with the missing data length!! I'll add that and go from there.</div><div> </div><div>thanks again!</div><div>Christina </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jun 1, 2020 at 7:31 PM Fraser Tweedale <<a href="mailto:ftweedal@redhat.com" target="_blank">ftweedal@redhat.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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". > <a href="https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L1209" rel="noreferrer" target="_blank">https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L1209</a> > 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? > <a href="https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L951" rel="noreferrer" target="_blank">https://github.com/dogtagpki/pki/blob/master/base/ca/src/com/netscape/ca/CAService.java#L951</a> > 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 </blockquote></div></div>