Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

@DDvO
Copy link
Contributor

DDvO commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 9, 2017

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

@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Aug 10, 2017

CLA on file, removing tag.

@dot-asm

This comment has been minimized.

Copy link
Contributor

dot-asm commented Aug 10, 2017

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...

@DDvO DDvO force-pushed the siemens:fix-OCSP_basic_verify branch from 7d87f88 to e2b0593 Aug 10, 2017
…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 DDvO force-pushed the siemens:fix-OCSP_basic_verify branch from e2b0593 to 20aaac5 Aug 10, 2017
@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 10, 2017

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

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 16, 2017

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.
my $expected_exit = shift;
if (!looks_like_number($expected_exit)) {

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Aug 16, 2017

Member

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

@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 16, 2017

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

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Aug 16, 2017

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

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 16, 2017

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

This comment has been minimized.

Copy link
Member

mattcaswell commented Aug 16, 2017

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.

Copy link
Contributor

richsalz left a comment

One minor change, one comment

@@ -73,6 +73,8 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs,
goto f_err;
}
}
} else if (certs) {

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 16, 2017

Contributor

Please add != NULL explicitly.

@@ -29,6 +29,10 @@ sub test_ocsp {
my $title = shift;
my $inputfile = shift;
my $CAfile = shift;
my $untrusted = shift;

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 16, 2017

Contributor

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

@DDvO

This comment has been minimized.

Copy link
Contributor Author

DDvO commented Aug 16, 2017

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

Copy link
Contributor

richsalz left a comment

i'll squash/merge.

levitte pushed a commit that referenced this pull request Aug 16, 2017
…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 <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #4124)
levitte pushed a commit that referenced this pull request Aug 16, 2017
…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 <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #4124)

(cherry picked from commit 121738d)
@richsalz

This comment has been minimized.

Copy link
Contributor

richsalz commented Aug 16, 2017

merged to 1.1.0 and master; thanks!

@richsalz richsalz closed this Aug 16, 2017
pracj3am added a commit to cdn77/openssl that referenced this pull request Aug 22, 2017
…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 <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from openssl#4124)

(cherry picked from commit 121738d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.