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

Thanks for the testing notes, Christina. Today I set up a local test CT log server using a container image. I plan to document more thoroughly but rough notes at [1]. Now to the issue I found - I have a commit[2] in a private branch. Hopefully the commit message and comments explain it well enough. Is it the last issue? Not sure yet, I finally got it compiled but haven't run the code yet. Tomorrow's adventure. [1] https://github.com/frasertweedale/notes-redhat/blob/master/ct.rst [2] https://github.com/frasertweedale/pki/tree/fix/ct-verify Cheers, Fraser

On Mon, Jun 15, 2020 at 10:45:53AM -0700, Christina Fu wrote: > 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 > > > >> > > > >> > > > >