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

2020-06-16 Thread Fraser Tweedale
On Wed, Jun 17, 2020 at 12:59:57AM +1000, Fraser Tweedale wrote:
> 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
> 

I went down the garden path on this one.  It turns out that JSS/NSS
*does* use the DER-encoded ECDSA-Sig-Value as the signature format.
(I wrote a small test program to check this).  So most of the code I
wrote yesterday is wrong.

Analysis continues.

Thanks,
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  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  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 
> > > > > 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 

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

2020-06-16 Thread Fraser Tweedale
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  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  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 
> > > > 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".
> > > >> >
> > > >>

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

2020-06-15 Thread Christina Fu
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  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  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 
> > > 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 

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

2020-06-11 Thread Fraser Tweedale
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  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 
> > 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 

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

2020-06-11 Thread Christina Fu
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  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 
> 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 

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

2020-06-02 Thread 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  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 + 

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

2020-06-01 Thread 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");