[Pki-devel] Certificate Transparency SCT signature verification?

[Christina Fu]

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 at redhat.com> 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/CAService.java#L1209
> > 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/CAService.java#L951
> >
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20200602/88d0554e/attachment.htm>