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

Hi Fraser, That sounds good! I just added the following page to document my "quick test" procedure which I use during development: https://www.dogtagpki.org/wiki/PKI_10.9_Certificate_Transparency btw, the verifySCT is currently enabled, but the failure is ignored. However, you could look in the debug log for "verifySCT" to see relevant debug messages. I'll ask Dinesh to add his more comprehensive testing procedure to the page. thanks!! Christina On Thu, Jun 11, 2020 at 5:58 PM Fraser Tweedale <ftweedal(a)redhat.com&gt; wrote: > Hi Christina, > > I will find a day next week to have a close look. Probably Tuesday > or Wednesday. It will help to have test environment setup > documentation, i.e. how to set up a log server to test with, how to > configure Dogtag, etc. If this stuff is already written then you > just need to tell me where to look :) > > Cheers, > Fraser > > On Thu, Jun 11, 2020 at 05:08:25PM -0700, Christina Fu wrote: > > 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 > > >> > > >> > >