Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master #1775

richsalz · 2016-10-24T17:09:47Z

To be ported to 1.1.0 and 1.0.2 Fails right now on master, needs to be investigated.

FdaSilvaYY · 2016-10-24T18:21:41Z

Error is pretty clear ;

test/crltest.c:40:5: error: string length '1253' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:64:5: error: string length '1298' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:77:5: error: string length '629' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:91:5: error: string length '658' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:105:5: error: string length '666' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:123:5: error: string length '654' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:140:5: error: string length '658' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

test/crltest.c:158:5: error: string length '682' is greater than the length '509' ISO C90 compilers are required to support [-Werror=overlength-strings]

richsalz · 2016-10-25T18:37:29Z

No, that's a build error which I did not see and which I will fix later. The run-time error is this:

CRL with unknown critical extension (2) was accepted.

davidben · 2016-10-25T21:25:50Z

Please consider any work from BoringSSL included here to be covered by our existing CLA with OpenSSL.

I think the reason the test doesn't pass is because the fix didn't get applied to master (or 1.1.0), so the test is working. :-)

davidben · 2016-10-25T22:49:49Z

By the way, in case you need to generate more of these or whatever, here's the private key corresponding to kCRLTestRoot:
https://boringssl.googlesource.com/boringssl/+/f9f312af61f9ba87896736620d1e4e568c4442bd
(That probably should have been in a comment. I'll go fix that...)

Also, shameless plug, but der-ascii is really handy for generating these kinds of things:
https://github.com/google/der-ascii
You can see notes on how I fixed the signatures in samples/certificates.md (documented because I can never remember the incant for signing things)

richsalz · 2016-10-26T16:40:55Z

yes, right i forgot to push the commit with the fix. (appropriate smiley)
the test gets picked to 1.0.2
everything here goes to 1.1.0 and master.
no longer a WIP.
ping @levitte who asked me about this off-line :)

kroeckx · 2016-10-27T19:33:33Z

You know that there are problems reported by travis, like "crltest: corrupted double-linked list"?

richsalz · 2016-10-27T22:30:53Z

No I did not notice the travis problems.

richsalz · 2016-10-28T14:40:31Z

Added a commit taht should fix this. Thanks for pointing me to the problem.

davidben · 2016-10-28T15:03:02Z

test/crltest.c

+    retcode = X509_verify_cert(ctx);
+    /* Unset the CA and CRL stacks so they don't get free'd by us. */
+    X509_STORE_CTX_trusted_stack(ctx, NULL);
+    X509_STORE_CTX_set0_crls(ctx, NULL);


I'm not seeing where either of these fields are freed by X509_STORE_CTX_free, though perhaps I'm just not seeing it.

Anyway, this doesn't make sense. One would expect that either both X509_STORE_CTX_set0_crls and X509_STORE_CTX_free free the existing value or neither to.

That is, if ctx owns crls as a result of line 235, then ctx is responsible for freeing it in both set0_crls and free. If ctx doesn't own crls, then ctx should not free it in either function. (At least from inspection, it seems to be the latter.)

asan is complaining, so let's see what happens. :)

doh, of course i wasn't making sense. :( backed it out. need to reproduce locally.

richsalz · 2016-10-30T15:47:33Z

found and fixed. pushing new version.
unclear if i will backport this to 1.0.2

snhenson · 2016-11-06T21:47:45Z

You can avoid the PEM concatentation by using DER format and calling the relevant d2i function directly.

levitte

Looks good to me at this point

richsalz · 2016-11-28T20:43:34Z

ping @levitte to re-review. uses the test framework. for master only.

richsalz · 2016-12-12T16:18:54Z

ping @levitte please reconfirm since it uses the test harness now. updated/rebased commit pushed.

levitte · 2016-12-13T17:17:20Z

test/crltest.c

+
+    if (!X509_STORE_CTX_init(ctx, store, leaf, NULL))
+        goto err;
+    X509_STORE_CTX_trusted_stack(ctx, roots);


Change to:

X509_STORE_CTX_set0_trusted_stack(ctx, roots);

(it will fix the Travis build issue)

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc

richsalz · 2016-12-13T17:25:04Z

updated commit pushed.

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1775)

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1775) (cherry picked from commit 2b40699)

richsalz · 2016-12-14T17:49:42Z

2b40699 in master, 0baae1c in 1.1.0

FdaSilvaYY · 2017-02-17T22:22:04Z

This fix was not pushed into 1.0.2 branch, who still has the issue too.
Any reason ?

richsalz · 2017-02-18T04:14:16Z

It was; see 3ade92e

richsalz added branch: master Merge to master branch branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch 1.1.0 labels Oct 24, 2016

richsalz self-assigned this Oct 24, 2016

richsalz added branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch and removed branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch labels Oct 26, 2016

richsalz changed the title ~~WIP Port CRL test from boring crypto/x509/x509_test.cc~~ Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master Oct 26, 2016

davidben reviewed Oct 28, 2016

View reviewed changes

kaduk mentioned this pull request Oct 28, 2016

Selftest to unittest #1789

Closed

3 tasks

levitte approved these changes Nov 9, 2016

View reviewed changes

richsalz removed branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch 1.1.0 labels Nov 28, 2016

richsalz mentioned this pull request Nov 29, 2016

TLSv1.3: Use TLSv1.3 style nonce construction (new approach) #1947

Closed

2 tasks

levitte requested changes Dec 13, 2016

View reviewed changes

CRL critical extension bugfix

96a15dd

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc

levitte approved these changes Dec 14, 2016

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Dec 14, 2016

levitte pushed a commit that referenced this pull request Dec 14, 2016

CRL critical extension bugfix

2b40699

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1775)

levitte pushed a commit that referenced this pull request Dec 14, 2016

CRL critical extension bugfix

0baae1c

More importantly, port CRL test from boringSSL crypto/x509/x509_test.cc Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #1775) (cherry picked from commit 2b40699)

richsalz closed this Dec 14, 2016

richsalz deleted the add-crltest branch December 22, 2016 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master #1775

Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master #1775

richsalz commented Oct 24, 2016

FdaSilvaYY commented Oct 24, 2016

richsalz commented Oct 25, 2016

davidben commented Oct 25, 2016

davidben commented Oct 25, 2016

richsalz commented Oct 26, 2016

kroeckx commented Oct 27, 2016

richsalz commented Oct 27, 2016

richsalz commented Oct 28, 2016

davidben Oct 28, 2016

richsalz Oct 28, 2016

richsalz Oct 28, 2016

richsalz commented Oct 30, 2016

snhenson commented Nov 6, 2016

levitte left a comment

richsalz commented Nov 28, 2016

richsalz commented Dec 12, 2016

levitte Dec 13, 2016

levitte Dec 13, 2016

richsalz commented Dec 13, 2016

richsalz commented Dec 14, 2016

FdaSilvaYY commented Feb 17, 2017 •

edited

richsalz commented Feb 18, 2017

Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master #1775

Port CRL test from boring crypto/x509/x509_test.cc; add fix to 1.1.0/master #1775

Conversation

richsalz commented Oct 24, 2016

FdaSilvaYY commented Oct 24, 2016

richsalz commented Oct 25, 2016

davidben commented Oct 25, 2016

davidben commented Oct 25, 2016

richsalz commented Oct 26, 2016

kroeckx commented Oct 27, 2016

richsalz commented Oct 27, 2016

richsalz commented Oct 28, 2016

davidben Oct 28, 2016

Choose a reason for hiding this comment

richsalz Oct 28, 2016

Choose a reason for hiding this comment

richsalz Oct 28, 2016

Choose a reason for hiding this comment

richsalz commented Oct 30, 2016

snhenson commented Nov 6, 2016

levitte left a comment

Choose a reason for hiding this comment

richsalz commented Nov 28, 2016

richsalz commented Dec 12, 2016

levitte Dec 13, 2016

Choose a reason for hiding this comment

levitte Dec 13, 2016

Choose a reason for hiding this comment

richsalz commented Dec 13, 2016

richsalz commented Dec 14, 2016

FdaSilvaYY commented Feb 17, 2017 • edited

richsalz commented Feb 18, 2017

FdaSilvaYY commented Feb 17, 2017 •

edited