Issue 1988993002: Check self-signed certificate names and signatures

dadrian

Description was changed from ========== Check self-signed certificate names and signatures Add unit tests for ...

4 years, 7 months ago (2016-05-20 17:40:47 UTC) #1

dadrian

dadrian@google.com changed reviewers: + estark@chromium.org, svaldez@chromium.org

4 years, 7 months ago (2016-05-20 17:40:47 UTC) #2

dadrian

Adding estark@ and svaldez@ as initial reviewers, per request from estark@

4 years, 7 months ago (2016-05-20 17:42:03 UTC) #3

svaldez

Some comments from a first pass. https://codereview.chromium.org/1988993002/diff/260001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/260001/net/cert/x509_certificate_ios.cc#newcode475 net/cert/x509_certificate_ios.cc:475: if (X509_check_issued(cert.get(), cert.get()) ...

4 years, 7 months ago (2016-05-20 17:54:53 UTC) #4

estark

Thanks! Totally didn't think about how gnarly the test data was going to be when ...

4 years, 7 months ago (2016-05-20 18:37:53 UTC) #5

dadrian

https://codereview.chromium.org/1988993002/diff/260001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/260001/net/cert/x509_certificate_ios.cc#newcode471 net/cert/x509_certificate_ios.cc:471: if ((X509_verify(cert.get(), scoped_key.get())) != 1) { On 2016/05/20 18:37:53, ...

4 years, 7 months ago (2016-05-20 19:01:22 UTC) #6

estark

lgtm with one more nit Ryan Sleevi's probably the best owner to review this, so ...

4 years, 7 months ago (2016-05-24 03:40:17 UTC) #8

svaldez

https://codereview.chromium.org/1988993002/diff/300001/net/cert/x509_certificate_nss.cc File net/cert/x509_certificate_nss.cc (right): https://codereview.chromium.org/1988993002/diff/300001/net/cert/x509_certificate_nss.cc#newcode294 net/cert/x509_certificate_nss.cc:294: return c == SECComparison::SECEqual; nit: You could almost just ...

4 years, 7 months ago (2016-05-24 14:18:01 UTC) #9

dadrian

dadrian@google.com changed reviewers: + rsleevi@chromium.org

4 years, 7 months ago (2016-05-25 16:38:52 UTC) #10

dadrian

rsleevi@chromium.org: Please review changes to how self-signed certificates are identified.

4 years, 7 months ago (2016-05-25 16:38:53 UTC) #11

Ryan Sleevi

Emily: Curious your thoughts about doing the munging native (e.g. no need to generate independent ...

4 years, 7 months ago (2016-05-26 07:57:09 UTC) #12

estark

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc#newcode473 net/cert/x509_certificate_ios.cc:473: // NOTE: x509_check_issued returns X509_V_OK in case of success ...

4 years, 7 months ago (2016-05-26 16:26:51 UTC) #13

Ryan Sleevi

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh File net/data/ssl/scripts/generate-bad-self-signed.sh (right): https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh#newcode62 net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER -outform PEM -out out/self-signed-invalid-sig.pem ...

4 years, 7 months ago (2016-05-26 16:34:56 UTC) #14

Ryan Sleevi

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc#newcode473 net/cert/x509_certificate_ios.cc:473: // NOTE: x509_check_issued returns X509_V_OK in case of success ...

4 years, 7 months ago (2016-05-26 16:36:42 UTC) #15

dadrian

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh File net/data/ssl/scripts/generate-bad-self-signed.sh (right): https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh#newcode62 net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER -outform PEM -out out/self-signed-invalid-sig.pem ...

4 years, 7 months ago (2016-05-26 18:41:36 UTC) #16

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-bad-self-signed.sh (right):

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER
-outform PEM -out out/self-signed-invalid-sig.pem
On 2016/05/26 16:34:56, Ryan Sleevi (OOO til 6-6) wrote:
> On 2016/05/26 16:26:51, estark wrote:
>  > Ahh, I didn't think of that, that does seem better. So you're suggesting
read
> in
> > a certificate, mess with the signature, and then use
> > X509Certificate::CreateFromBytes()? When you say "we know exactly where the
> > signature is", do you mean hardcode the byte offset? Or is there a nicer
way?
> 
> Could use the ASN.1 parser we have and just skip to the offset. On the con
side,
> that's more work, and more native, on the plus side, it means less bash
> scripting, less coupling to OpenSSL's text form, and less coreutils
> dependencies. Mostly I mention it because when I wrote a lot of the original
> bash scripts, I was using them in Windows in msys (because I'm weird like that
> and because depot-tools includes msys)
> 
> I was thinking something like
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/asn1_util...
> in the _unittest.cc file to munge the data, and then CreateFromBytes.
> 
> My own gut take is that it'd be more maintainable, but I'm also looking to see
> if there's disagreement, or if it's more work than I'm thinking, since it's
just
> more of a gut take.

I'm less familiar with Chrome development, so take what I say with a grain of
salt. However, in my experience outside of Chrome, having the actual
certificates for various error conditions written to disk, rather than
"ephemeral" certificates generated programmatically during the life of a test,
has always been more useful. For one, certificate files are easier to grok since
you can send them through `openssl x509 -text` or similar. Beyond that, since
it's physically impossible to write certificate-handling code correctly, when
you inevitably find bugs, they're easier to share across various parts of code
that touch certificates, and also easier to verify for (in)correctness.

estark

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc#newcode473 net/cert/x509_certificate_ios.cc:473: // NOTE: x509_check_issued returns X509_V_OK in case of success ...

4 years, 7 months ago (2016-05-27 00:59:13 UTC) #17

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certific...
File net/cert/x509_certificate_ios.cc (right):

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certific...
net/cert/x509_certificate_ios.cc:473: // NOTE: x509_check_issued returns
X509_V_OK in case of success
On 2016/05/26 16:36:41, Ryan Sleevi (OOO til 6-6) wrote:
> On 2016/05/26 16:26:51, estark wrote:
> > On 2016/05/26 07:57:08, Ryan Sleevi wrote:
> > > You don't need this NOTE (X509_verify() was the weird API contract)
> > 
> > FWIW, I think BoringSSL actually just returns 0/1 from X509_verify. (...for
> now.
> > that might change.) So line 470 isn't exactly accurate anymore. Maybe it
> should
> > be changed to "Historically, OpenSSL's X509_verify returned..." or something
> > like that?
> 
> If that's the case (and the API is fixed), we should remove both NOTEs. The
NOTE
> then was just "OpenSSL APIs are dumb" - if this was fixed in David's BoringSSL
> burning run, then we should just "if (!X509_Verify())" and be done with it
(and
> remove the NOTE)

I was thinking that we shouldn't remove the comment entirely because there's a
possibility that the BoringSSL API might change back because a caller in CT was
kind of depending on the weird -1/0 return values
(https://bugs.chromium.org/p/boringssl/issues/detail?id=18). Buuut, if and when
we change it back to -1/0/1, we're going to have to audit all the callers to
make sure that everything makes sense, so we can always change it back then. 
So, removing it and doing `if !X509_verify()` SGTM.

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-bad-self-signed.sh (right):

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER
-outform PEM -out out/self-signed-invalid-sig.pem
On 2016/05/26 18:41:36, dadrian wrote:
> On 2016/05/26 16:34:56, Ryan Sleevi (OOO til 6-6) wrote:
> > On 2016/05/26 16:26:51, estark wrote:
> >  > Ahh, I didn't think of that, that does seem better. So you're suggesting
> read
> > in
> > > a certificate, mess with the signature, and then use
> > > X509Certificate::CreateFromBytes()? When you say "we know exactly where
the
> > > signature is", do you mean hardcode the byte offset? Or is there a nicer
> way?
> > 
> > Could use the ASN.1 parser we have and just skip to the offset. On the con
> side,
> > that's more work, and more native, on the plus side, it means less bash
> > scripting, less coupling to OpenSSL's text form, and less coreutils
> > dependencies. Mostly I mention it because when I wrote a lot of the original
> > bash scripts, I was using them in Windows in msys (because I'm weird like
that
> > and because depot-tools includes msys)
> > 
> > I was thinking something like
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/asn1_util...
> > in the _unittest.cc file to munge the data, and then CreateFromBytes.
> > 
> > My own gut take is that it'd be more maintainable, but I'm also looking to
see
> > if there's disagreement, or if it's more work than I'm thinking, since it's
> just
> > more of a gut take.
> 
> I'm less familiar with Chrome development, so take what I say with a grain of
> salt. However, in my experience outside of Chrome, having the actual
> certificates for various error conditions written to disk, rather than
> "ephemeral" certificates generated programmatically during the life of a test,
> has always been more useful. For one, certificate files are easier to grok
since
> you can send them through `openssl x509 -text` or similar. Beyond that, since
> it's physically impossible to write certificate-handling code correctly, when
> you inevitably find bugs, they're easier to share across various parts of code
> that touch certificates, and also easier to verify for (in)correctness.

Hrm, I am struggling to come up with a strong opinion either way. The openssl
bash stuff is pretty gross -- no offense, David :) -- but I buy the argument
that it's handy to have the actual certificate file around to poke at.

dadrian

https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc File net/cert/x509_certificate_ios.cc (right): https://codereview.chromium.org/1988993002/diff/320001/net/cert/x509_certificate_ios.cc#newcode473 net/cert/x509_certificate_ios.cc:473: // NOTE: x509_check_issued returns X509_V_OK in case of success ...

4 years, 7 months ago (2016-05-27 01:05:03 UTC) #18

dadrian

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh File net/data/ssl/scripts/generate-bad-self-signed.sh (right): https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/generate-bad-self-signed.sh#newcode62 net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER -outform PEM -out out/self-signed-invalid-sig.pem ...

4 years, 7 months ago (2016-05-27 01:22:57 UTC) #19

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
File net/data/ssl/scripts/generate-bad-self-signed.sh (right):

https://codereview.chromium.org/1988993002/diff/320001/net/data/ssl/scripts/g...
net/data/ssl/scripts/generate-bad-self-signed.sh:62: | openssl x509 -inform DER
-outform PEM -out out/self-signed-invalid-sig.pem
On 2016/05/27 00:59:13, estark wrote:
> On 2016/05/26 18:41:36, dadrian wrote:
> > On 2016/05/26 16:34:56, Ryan Sleevi (OOO til 6-6) wrote:
> > > On 2016/05/26 16:26:51, estark wrote:
> > >  > Ahh, I didn't think of that, that does seem better. So you're
suggesting
> > read
> > > in
> > > > a certificate, mess with the signature, and then use
> > > > X509Certificate::CreateFromBytes()? When you say "we know exactly where
> the
> > > > signature is", do you mean hardcode the byte offset? Or is there a nicer
> > way?
> > > 
> > > Could use the ASN.1 parser we have and just skip to the offset. On the con
> > side,
> > > that's more work, and more native, on the plus side, it means less bash
> > > scripting, less coupling to OpenSSL's text form, and less coreutils
> > > dependencies. Mostly I mention it because when I wrote a lot of the
original
> > > bash scripts, I was using them in Windows in msys (because I'm weird like
> that
> > > and because depot-tools includes msys)
> > > 
> > > I was thinking something like
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/asn1_util...
> > > in the _unittest.cc file to munge the data, and then CreateFromBytes.
> > > 
> > > My own gut take is that it'd be more maintainable, but I'm also looking to
> see
> > > if there's disagreement, or if it's more work than I'm thinking, since
it's
> > just
> > > more of a gut take.
> > 
> > I'm less familiar with Chrome development, so take what I say with a grain
of
> > salt. However, in my experience outside of Chrome, having the actual
> > certificates for various error conditions written to disk, rather than
> > "ephemeral" certificates generated programmatically during the life of a
test,
> > has always been more useful. For one, certificate files are easier to grok
> since
> > you can send them through `openssl x509 -text` or similar. Beyond that,
since
> > it's physically impossible to write certificate-handling code correctly,
when
> > you inevitably find bugs, they're easier to share across various parts of
code
> > that touch certificates, and also easier to verify for (in)correctness.
> 
> Hrm, I am struggling to come up with a strong opinion either way. The openssl
> bash stuff is pretty gross -- no offense, David :) -- but I buy the argument
> that it's handy to have the actual certificate file around to poke at.

OpenSSL anything is pretty gross. I'd argue that most people interacting with
the test certificates wouldn't have to touch it, though.

Ryan Sleevi

rsleevi@chromium.org changed reviewers: + eroman@chromium.org

4 years, 7 months ago (2016-05-27 04:28:30 UTC) #20

Ryan Sleevi

Going OOO, so I'm going to defer the test bit to eroman, since he's looked ...

4 years, 7 months ago (2016-05-27 04:28:31 UTC) #21

eroman

- I like that there are unit-tests - I like that the tests are expressed ...

4 years, 7 months ago (2016-05-27 07:59:37 UTC) #22

dadrian

On 2016/05/27 07:59:37, eroman wrote: > - I like that there are unit-tests > > ...

4 years, 6 months ago (2016-05-27 18:48:15 UTC) #23

eroman

On Fri, May 27, 2016 at 11:48 AM, <dadrian@google.com> wrote: > On 2016/05/27 07:59:37, eroman ...

4 years, 6 months ago (2016-05-27 18:51:55 UTC) #24

On Fri, May 27, 2016 at 11:48 AM, <dadrian@google.com> wrote:

> On 2016/05/27 07:59:37, eroman wrote:
> > - I like that there are unit-tests
> >
> > - I like that the tests are expressed as certificate files rather than
> requiring
> > C++ manipulations at test time.
> >
> > - ... But I don't like the bash script
> >
> >
> > I am good with this change provided we can clean up the mechanism for
> mutating
> > the signature a bit.
> >
> > How about just flipping a byte relative to the *end* of the file -- say
> the
> 4th
> > byte from the end. The signature is the final field in the certificate,
> so
> > corrupting it is just a matter of change something near the end of the
> DER.
> >
> > This should address some of Ryan's concern, which I think was largely
> based on
> > the sorcery of using OpenSSL's text format, sed, and xxd to achieve that
> > manipulation.
> >
> > On a personal note, I find the OpenSSL .cnf approach and command line
> flag
> > incantations to be extremely obtuse. I am also a hater of bash scripts.
> When I
> > was dealing with this to generate certificate chain test data, the only
> thing
> > that felt sane was to write simple python scripts, and wrap the OpenSSL
> magic
> > and generate appropriate .cnf file. For example:
> >
> >
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/verify_ce...
> >
> >
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/verify_ce...
> >
> > I think it worked pretty well, and would be in favor (separate from this
> CL of
> > course) to use that approach and remove our dependence on bash
> all-together.
>
> Unfortunately, I don't think there's a very nice way to modify specific
> byte
> offsets in bash, but I gave it a shot. I don't think this is necessarily
> any
> cleaner to read, but at least it doesn't require xxd or depend on the
> format of
> `openssl -text`
>

I think that is an improvement.
LGTM.


>
> https://codereview.chromium.org/1988993002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

dadrian

The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org, estark@chromium.org Link to ...

4 years, 6 months ago (2016-05-27 19:01:33 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988993002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988993002/380001

4 years, 6 months ago (2016-05-27 19:02:10 UTC) #28

commit-bot: I haz the power

Description was changed from ========== Check self-signed certificate names and signatures Add unit tests for ...

4 years, 6 months ago (2016-05-27 20:11:29 UTC) #29

commit-bot: I haz the power

Description was changed from ========== Check self-signed certificate names and signatures Add unit tests for ...

4 years, 6 months ago (2016-05-27 20:14:32 UTC) #31

commit-bot: I haz the power

4 years, 6 months ago (2016-05-27 20:14:34 UTC) #32

Message was sent while issue was closed.

Patchset 20 (id:??) landed as
https://crrev.com/da42899529b15a0fa89a072c6e46c22c11292514
Cr-Commit-Position: refs/heads/master@{#396548}

Issue 1988993002: Check self-signed certificate names and signatures (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fixed test certs #

Patch Set 3 : iOS implementation first try #

Patch Set 4 : Reorder self signed tests #

Patch Set 5 : Update net.gypi for self-signed test certs #

Patch Set 6 : Document test case cert generation #

Patch Set 7 : forgot to update net.gypi #

Patch Set 8 : First attempt at windows #

Patch Set 9 : Fix windows build #

Patch Set 10 : Fix OpenSSL build #

Patch Set 11 : OpenSSL fixes again #

Patch Set 12 : Windows build fixes #

Patch Set 13 : Windows bug fixes #

Patch Set 14 : More Windows bugfixes #

Patch Set 15 : Address initial comments #

Patch Set 16 : Bugfixes for changes from initial comments #

Patch Set 17 : Formatting nits #

Patch Set 18 : Address comments from rsleevi@ #

Patch Set 19 : Fix compilation on Windows #

Patch Set 20 : Remove dependency on openssl -text format #

Messages