[Pki-devel] Certificate Transparency SCT signature verification?

[Fraser Tweedale]

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