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.

Sign up for GitHub

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

Jump to bottom

OCSP signature verification discards OpenSSL error information #395

Open
p-mongo opened this issue Aug 20, 2020 · 5 comments
Open

OCSP signature verification discards OpenSSL error information #395

p-mongo opened this issue Aug 20, 2020 · 5 comments

Comments

@p-mongo
Copy link

p-mongo commented Aug 20, 2020 •
edited
Loading

In https://github.com/ruby/openssl/blob/master/ext/openssl/ossl_ocsp.c#L428-L429, ossl_ocspreq_verify does:

    if (result <= 0)
	ossl_clear_error();

return result > 0 ? Qtrue : Qfalse;

If the result is not successful, the error information is cleared. This makes it not available in OpenSSL.errors for inspection by the caller.

Response verification has the same issue.

To get this information, one needs to enable OpenSSL.debug prior to calling the verify method.

Example missing information:

(byebug) resp.verify([ca_cert], store)
Result: 0
false
(byebug) OpenSSL.errors 
[]
(byebug) resp.verify([ca_cert], store)
Result: 0
false
(byebug) OpenSSL.errors 
[]
(byebug) OpenSSL.debug=true
true
(byebug) resp.verify([ca_cert], store)
(byebug):1: warning: error on stack: error:27069065:OCSP routines:OCSP_basic_verify:certificate verify error (Verify error:self signed certificate)
Result: 0
false
(byebug) store.add_cert(ca_cert)
#<OpenSSL::X509::Store:0x00005630c0470120 @verify_callback=nil, @error=nil, @error_string=nil, @chain=nil, @time=nil>
(byebug) resp.verify [],store
(byebug):1: warning: error on stack: error:27069076:OCSP routines:OCSP_basic_verify:signer certificate not found
Result: 0
false
(byebug) resp.verify [ca_cert],store
Result: 1
true

Without the debug information, figuring out why verification failed is impractical.

One way of solving this is to provide a method like verify! which would raise an exception if verification fails, including the openssl error information into the method.
@p-mongo p-mongo changed the title OCSP signature verification discards OpenSSL error information OCSP response signature verification discards OpenSSL error information Aug 20, 2020
@p-mongo p-mongo changed the title OCSP response signature verification discards OpenSSL error information OCSP signature verification discards OpenSSL error information Aug 20, 2020
@rhenium
Copy link
Member

rhenium commented Aug 21, 2020

Somewhat related: #312

@rhenium
Copy link
Member

rhenium commented Aug 21, 2020

If the result is not successful, the error information is cleared. This makes it not available in OpenSSL.errors for inspection by the caller.

This is expected because OpenSSL.errors is meant for debugging this library, for issues like https://bugs.ruby-lang.org/issues/7215. If it ever returns a non-empty array, then there is a missing ossl_clear_error() call somewhere which is a bug that needs to be fixed.

Response verification has the same issue.

Actually, all verify methods in this library have the same issue. In my opinion, this is a design flaw and they could raise an exception on verification failure by default, but we can't fix it.

I don't like the name verify! as it is not "more dangerous" than the current verify. I can't come up with a better one, however.

@p-mongo
Copy link
Author

p-mongo commented Aug 27, 2020

but we can't fix it

What is preventing adding new methods that raise exceptions?

@rhenium
Copy link
Member

rhenium commented Aug 29, 2020

Nothing. It is just that we can't change the behavior of existing methods and verify! is not a good fit for the new name.

Two other easy ways I can think of:

  • obj.verify(..., exception: true) - similarly to IO#read_nonblock
  • Let ossl_clear_error() store the error information in thread/fiber-local storage and repurpose OpenSSL.errors to return it (given that nothing uses OpenSSL.errors for the current purpose, apart from Ruby/OpenSSL's regression test)

@p-mongo
Copy link
Author

p-mongo commented Aug 30, 2020

I am happy to contribute an implementation. I don't understand what the conclusion/decision is in this ticket, if that is clarified I can look into typing the code up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rhenium @p-mongo