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

crypto: fix VerifyCallback in case of verify error #2064

Closed
wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Jun 26, 2015

3beb880 has a bug in VerifyCallback when preverify is 1 and the cert chain has an verify error. If the error is UNABLE_TO_GET_ISSUER_CERT_LOCALLY, it leads an assertion error in finding rootCA.
The whitelist check should be made only when the cert chain has no verify error with X509_V_OK.

Fixes: #2061

CI of https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/87/ are green except one failure of test-debug-port-from-cmdline.js on win2012r2. It's not related with this fix.

make test-internetare fine and the result of a test shown by the issue reporter of #2061 is below.

$ ./iojs -e "require('tls').connect(443, '143.116.116.84');"
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: certificate has expired
    at Error (native)
    at TLSSocket.<anonymous> (_tls_wrap.js:965:38)
    at emitNone (events.js:67:13)
    at TLSSocket.emit (events.js:166:7)
    at TLSSocket._finishInit (_tls_wrap.js:546:8)

The error code is different because UNABLE_TO_GET_ISSUER_CERT_LOCALLY is overwritten by CERT_HAS_EXPIRED according to the order of verify checks.

R= @bnoordhuis

3beb880 has a bug in VerifyCallback
when preverify is 1 and the cert chain has an verify error. If the
error is UNABLE_TO_GET_ISSUER_CERT_LOCALLY, it leads an assertion
error in finding rootCA.
The whitelist check should be made only when the cert chain has no
verify error with X509_V_OK.

Fixes: nodejs#2061
@shigeki shigeki added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. labels Jun 26, 2015
@bnoordhuis
Copy link
Member

LGTM

shigeki pushed a commit that referenced this pull request Jun 27, 2015
3beb880 has a bug in VerifyCallback
when preverify is 1 and the cert chain has an verify error. If the
error is UNABLE_TO_GET_ISSUER_CERT_LOCALLY, it leads an assertion
error in finding rootCA.
The whitelist check should be made only when the cert chain has no
verify error with X509_V_OK.

Fixes: #2061
PR-URL: #2064
Reviewed-By: Ben Noordhuis <[email protected]>
@shigeki
Copy link
Contributor Author

shigeki commented Jun 27, 2015

Thanks for revewing, Ben. Landed in 9e890fe.

@shigeki shigeki closed this Jun 27, 2015
@rvagg rvagg mentioned this pull request Jun 30, 2015
mscdex pushed a commit to mscdex/io.js that referenced this pull request Jul 9, 2015
3beb880 has a bug in VerifyCallback
when preverify is 1 and the cert chain has an verify error. If the
error is UNABLE_TO_GET_ISSUER_CERT_LOCALLY, it leads an assertion
error in finding rootCA.
The whitelist check should be made only when the cert chain has no
verify error with X509_V_OK.

Fixes: nodejs#2061
PR-URL: nodejs#2064
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.3.1 introduced an assertion exception
2 participants