Fix OCSP_basic_verify() cert chain construction in case bs->certs is NULL by DDvO · Pull Request #4124 · openssl/openssl

DDvO · 2017-08-09T16:25:27Z

Now the certs arg is not any more neglected when building the signer cert chain.
Added case to test/recipes/80-test_ocsp.t proving fix for 3-level CA hierarchy.

See also http://rt.openssl.org/Ticket/Display.html?id=4620

It seems that OCSP_basic_verify(bs, certs, st, flags) unfortunately is
not documented, but from its code it becomes clear that the "certs"
parameter is meant to be a set of untrusted certificates, which is first
used (together with bs->certs) to determine the signer cert of the OCSP
response "bs" and then is partly(!) used to construct the chain of certs
towards a trusted (root) cert in the store passed in the "st" parameter.

As long as the OCSP response pointed to by "bs" includes a non-NULL
bs->certs field, OCSP_basic_verify() takes the union of any certs in the
"certs" parameter and in bs->certs as untrusted certs for chain
construction, but if bs->certs is NULL, i.e. when the OCSP responder did
not include any certs its response, for some reason OCSP_basic_verify()
does not take "certs" but bs->certs, which corresponds to the empty set.

This fix takes "certs" as the set of untrusted certs in case bs->certs is NULL.

Checklist

tests are added or updated

DDvO · 2017-08-09T18:41:58Z

The automatic Travis CI build given above failed due to an unrelated memory leak in test/sslapitest.c
See #4127 for more detail.

richsalz · 2017-08-10T12:25:20Z

CLA on file, removing tag.

dot-asm · 2017-08-10T13:41:02Z

This is unreview-able. It looks like partial re-base. Unfortunately I don't have an advice how to do it right, as it always worked for me as I expected...

…NULL Now the certs arg is not any more neglected when building the signer cert chain. Added case to test/recipes/80-test_ocsp.t proving fix for 3-level CA hierarchy. See also http://rt.openssl.org/Ticket/Display.html?id=4620

DDvO · 2017-08-10T14:33:29Z

Oops, I had made a mistake when rebasing, such that my commit went somewhat below the head.
Thanks for the comment. Now it should be fine.

DDvO · 2017-08-16T04:34:41Z

For some reason, the cla-check failure tag appears still present, although Rich has cleared the need-cla label.
Anything (else) needed for reviewing this pull request?

The actual code change of the crypto library for the fix is tiny: 2 new lines.
Maybe some explanations on the extension of the OCSP tests are due:

The new test case "NON-DELEGATED; 3-level CA hierarchy" has been designed to fail without the fix and to pass after it has been applied, demonstrating the effectiveness of the fix.
I tried to stick with the available test data and scripts as much as possible:
I was glad to find an existing OCSP test response (ND1.ors) with empty certs field and a corresponding signer cert (ND1_Issuer_ICA.pem) for which I could extend the cert chain to contain three elements (in this case by adding a cross-certification cert, resulting in the new file ND1_Issuer_ICA-Cross.pem, and a new root cert file ND1_Cross_Root.pem).
Unfortunately, the existing Perl subroutine test_ocsp() did not distinguish between trusted and untrusted certs and used the CAfile parameter for both. I could have added a new parameter for untrusted certs to all test cases, but I did not want to touch them (and clean up the cert arguments for them accordingly). Instead I added a case distinction allowing for an optional untrusted parameter, so far used only by the new test case.

mattcaswell · 2017-08-16T07:45:31Z

test/recipes/80-test_ocsp.t

    my $CAfile = shift;
+    my $untrusted = $CAfile;
    my $expected_exit = shift;
+    if (!looks_like_number($expected_exit)) {


This looks quite horrible. Can we not just add the extra argument to the other tests (perhaps with "" meaning "use CAfile")?

DDvO · 2017-08-16T09:51:05Z

@mattcaswell thanks for the first review.
I've simplified test_ocsp as you suggested.

mattcaswell · 2017-08-16T10:01:17Z

This applies cleanly to master and 1.1.0. I suspect the issue is present in 1.0.2 as well, but this PR won't apply cleanly to that branch due to the test. It may be appropriate to have a second PR which just makes the main change without the test for 1.0.2.

DDvO · 2017-08-16T10:10:41Z

I haven't tried with 1.0.2.
Could it make sense to do a version distinction inside the test?
Otherwise, how to do a PR with just the fix specifically for 1.0.2?

mattcaswell · 2017-08-16T10:17:47Z

The test uses the new test framework which isn't available in 1.0.2, so couldn't be back ported without additional work. There may be an equivalent test in the old 1.0.2 way of doing things that you could amend - I've not checked - but it's probably not worth the effort. To do a PR to fix specifically 1.0.2, just create a new branch based on OpenSSL_1_0_2-stable and make the relevant change. Then create the PR in the normal way, but instead of master (the default) select OpenSSL_1_0_2-stable as the target branch.

richsalz

One minor change, one comment

richsalz · 2017-08-16T11:39:10Z

crypto/ocsp/ocsp_vfy.c

                    goto f_err;
                }
            }
+        } else if (certs) {


Please add != NULL explicitly.

richsalz · 2017-08-16T11:40:12Z

test/recipes/80-test_ocsp.t

    my $title = shift;
    my $inputfile = shift;
    my $CAfile = shift;
+    my $untrusted = shift;


this could be shift || $CAfile if you wanted. Or not. :)

DDvO · 2017-08-16T17:16:51Z

Re-formulated the certs check as requested.
Did not do the change regarding 'shift' since AFAICS this would work for the last parameter only.

richsalz

i'll squash/merge.

…NULL Now the certs arg is not any more neglected when building the signer cert chain. Added case to test/recipes/80-test_ocsp.t proving fix for 3-level CA hierarchy. See also http://rt.openssl.org/Ticket/Display.html?id=4620 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #4124)

…NULL Now the certs arg is not any more neglected when building the signer cert chain. Added case to test/recipes/80-test_ocsp.t proving fix for 3-level CA hierarchy. See also http://rt.openssl.org/Ticket/Display.html?id=4620 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #4124) (cherry picked from commit 121738d)

richsalz · 2017-08-16T18:37:03Z

merged to 1.1.0 and master; thanks!

…NULL Now the certs arg is not any more neglected when building the signer cert chain. Added case to test/recipes/80-test_ocsp.t proving fix for 3-level CA hierarchy. See also http://rt.openssl.org/Ticket/Display.html?id=4620 Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from openssl#4124) (cherry picked from commit 121738d)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Aug 9, 2017

richsalz removed the hold: cla required The contributor needs to submit a license agreement label Aug 10, 2017

DDvO force-pushed the fix-OCSP_basic_verify branch from 7d87f88 to e2b0593 Compare August 10, 2017 14:25

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Aug 10, 2017

DDvO force-pushed the fix-OCSP_basic_verify branch from e2b0593 to 20aaac5 Compare August 10, 2017 14:29

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Aug 10, 2017

richsalz removed the hold: cla required The contributor needs to submit a license agreement label Aug 13, 2017

mattcaswell reviewed Aug 16, 2017

View reviewed changes

simplified test_ocsp

d396743

mattcaswell added 1.1.0 branch: master Applies to master branch labels Aug 16, 2017

mattcaswell approved these changes Aug 16, 2017

View reviewed changes

mattcaswell added the approval: review pending This pull request needs review by a committer label Aug 16, 2017

richsalz suggested changes Aug 16, 2017

View reviewed changes

re-formulated certs check using '!= NULL'

a1ef0ff

richsalz approved these changes Aug 16, 2017

View reviewed changes

richsalz added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 16, 2017

richsalz closed this Aug 16, 2017

DDvO mentioned this pull request Aug 17, 2017

Fix OCSP_basic_verify() cert chain construction in case bs->certs is NULL #4183

Closed

DDvO mentioned this pull request Jul 31, 2018

Certificate Management Protocol (CMP, RFC 4210) extension #6811

Closed

2 tasks

Uh oh!

Conversation

DDvO commented Aug 9, 2017

Checklist

Uh oh!

DDvO commented Aug 9, 2017

Uh oh!

richsalz commented Aug 10, 2017

Uh oh!

dot-asm commented Aug 10, 2017

Uh oh!

DDvO commented Aug 10, 2017

Uh oh!

DDvO commented Aug 16, 2017

Uh oh!

mattcaswell Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

DDvO commented Aug 16, 2017

Uh oh!

mattcaswell commented Aug 16, 2017

Uh oh!

DDvO commented Aug 16, 2017

Uh oh!

mattcaswell commented Aug 16, 2017

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

richsalz Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

richsalz Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

DDvO commented Aug 16, 2017

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

richsalz commented Aug 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants