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

ssl handshake failure v0.12 or v0.11 #4485

Closed
magicode opened this issue Dec 30, 2015 · 9 comments
Closed

ssl handshake failure v0.12 or v0.11 #4485

magicode opened this issue Dec 30, 2015 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@magicode
Copy link

Test Code

var https = require('https');
//var tls = require("tls");

var url = require("url");

var counter = 0;

function reqHttps(path , callback){
    var parseUrl = url.parse(path);

    counter++;
    if(counter % 1000 == 0) console.log("req:" , counter);

    var options = {};
    options.hostname = parseUrl.hostname;
    options.path = parseUrl.path;
    options.method = 'POST';
    options.secureProtocol = 'TLSv1_method';

    options.agent =  new https.Agent({
        host: options.hostname,
        port: 443,
    });
    var req = https.request(options, function(res) {

        res.on('data', function(d) {

        });

        res.on('end', function(d) {

            callback();
        });
    });

    req.on('error',function(err){
        if((err +'').indexOf('EPROTO') != -1) return callback();
        //if((err +'').indexOf('unable to verify') != -1) return callback();
        console.log('URL: ' + path);
        throw err;
    });

    req.end();

}

var list = [];
list.push('https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_116x41dp.png');
list.push('https://www.flpil.co.il/'); // no support TLSv1_method

var randCounter = 0;


function getUrl(){
    var currId = randCounter++;

    currId = currId % list.length;

    return list[currId];
}


for(var i =0;i<300;i++){
    (function(){
        function next(){
            reqHttps(getUrl(),next);
        }   
        next();
    })();

}

Output in node v0.12 or v.0.11

URL: https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_116x41dp.png
/home/test.js:41
        throw err;
              ^
Error: 140321677469568:error:1409E0E5:SSL routines:SSL3_WRITE_BYTES:ssl handshake failure:../deps/openssl/openssl/ssl/s3_pkt.c:637:

    at Error (native)

Output in node v0.10 , v4 , v5

req: 1000
req: 2000
req: 3000
...
@mscdex mscdex added tls Issues and PRs related to the tls subsystem. v0.12 labels Dec 31, 2015
@indutny
Copy link
Member

indutny commented Jan 1, 2016

May I ask you which particular version of v0.12 you are using? Am I right that you don't expect it to work on v0.10, and it is the version that should be fixed?

@magicode
Copy link
Author

magicode commented Jan 1, 2016

i test by nvm
on version v0.12.9 it's throwing error
on version v0.10.41 it's working great

@indutny
Copy link
Member

indutny commented Jan 3, 2016

Confirmed looking into it.

@indutny indutny added the confirmed-bug Issues with confirmed bugs. label Jan 3, 2016
@indutny
Copy link
Member

indutny commented Jan 3, 2016

It looks like v5 should fail too, but for some reason it does not... Looking into it.

@indutny
Copy link
Member

indutny commented Jan 3, 2016

It actually fails too, it is just emitting EPROTO instead of SSL error. Another problem with v0.12 is that it seems to be emitting SSL error on wrong socket. Will see how it should be fixed.

@indutny
Copy link
Member

indutny commented Jan 3, 2016

@magicode may I ask you to test following patch for v0.12 ?

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index 607f786..fa36a1d 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -56,6 +56,9 @@ using v8::Value;
 size_t TLSCallbacks::error_off_;
 char TLSCallbacks::error_buf_[1024];

+struct ClearErrorOnReturn {
+  ~ClearErrorOnReturn() { ERR_clear_error(); }
+};

 TLSCallbacks::TLSCallbacks(Environment* env,
                            Kind kind,
@@ -451,6 +454,9 @@ void TLSCallbacks::ClearOut() {
   if (eof_)
     return;

+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   HandleScope handle_scope(env()->isolate());
   Context::Scope context_scope(env()->context());

@@ -501,6 +507,9 @@ bool TLSCallbacks::ClearIn() {
   if (!hello_parser_.IsEnded())
     return false;

+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   int written = 0;
   while (clear_in_->Length() > 0) {
     size_t avail = 0;
@@ -590,6 +599,9 @@ int TLSCallbacks::DoWrite(WriteWrap* w,
     return 0;
   }

+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   int written = 0;
   for (i = 0; i < count; i++) {
     written = SSL_write(ssl_, bufs[i].base, bufs[i].len);
@@ -675,6 +687,9 @@ void TLSCallbacks::DoRead(uv_stream_t* handle,


 int TLSCallbacks::DoShutdown(ShutdownWrap* req_wrap, uv_shutdown_cb cb) {
+  ClearErrorOnReturn clear_error_on_return;
+  (void) &clear_error_on_return;  // Silence compiler warning.
+
   if (SSL_shutdown(ssl_) == 0)
     SSL_shutdown(ssl_);
   shutdown_ = true;

Appears to be fixing problem for me. Thanks!

@indutny
Copy link
Member

indutny commented Jan 3, 2016

Should be fixed by #4515

indutny added a commit to indutny/io.js that referenced this issue Jan 4, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit that referenced this issue Jan 4, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Jan 6, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <[email protected]>
@Fishrock123
Copy link
Contributor

Should be fixed in v5.4.0. Please let us know if it isn't.

@magicode
Copy link
Author

magicode commented Jan 7, 2016

I try patch for v0.12. it work well. thanks!

indutny added a commit to indutny/io.js that referenced this issue Jan 14, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this issue Jan 15, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <[email protected]>

v4.x-staging Commit Metadata:
PR-URL: #4709
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 19, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: #4485
PR-URL: #4515
Reviewed-By: Ben Noordhuis <[email protected]>

v4.x-staging Commit Metadata:
PR-URL: #4709
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
Adopt `MarkPopErrorOnReturn` from `node_crypto.cc`, and use it to
clear errors after `SSL_read`/`SSL_write`/`SSL_shutdown` functions.

See: nodejs#4485
PR-URL: nodejs#4515
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
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants