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

Crash using SNICallback over a netSocket #10704

Closed
webertrlz opened this issue Jan 9, 2017 · 1 comment
Closed

Crash using SNICallback over a netSocket #10704

webertrlz opened this issue Jan 9, 2017 · 1 comment
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@webertrlz
Copy link

webertrlz commented Jan 9, 2017

My node app is crashing often when using SNICallback with netSocket and netServer, can't reproduce it yet.

Backtrace:

 _tls_wrap.js:117
   if (ctx.context)
          ^
  TypeError: Cannot read property 'context' of undefined
     at requestOCSP (_tls_wrap.js:117:10)
     at _tls_wrap.js:167:5
     at loadSNI (_tls_wrap.js:88:12)
     at TLSSocket.oncertcb (_tls_wrap.js:164:3)
     at TLSWrap.ssl.oncertcb (_tls_wrap.js:418:39)

Here is a small piece of code just to show what is done:

//options object to use with tls.TLSSocket()
var sslOptions = {
	isServer : true,

	SNICallback : function(hostname, callback){
		callback(null, tls.createSecureContext(fetchDomainCertificate()));
	},

	key : fetchDefaultKey(),
	cert : fetchDefaultCert(),
	ca : fetchDefaultCa(),

	requestCert : false,
	rejectUnauthorized : false,
	requestOCSP : false
}

//create a server using 'net' package
var server = net.createServer(function(_socket){
	var socket = new tlswrapper.tlsWrapper(_socket);
	var lineStream = readline.createInterface(socket, socket);

	lineStream.on('line', function(line){
		if(line == "STARTTLS"){
			//issue the encryption mechanism
			socket.startTLS(sslOptions);
		}
	});

	[...]

}).listen(anyPort);

//see here that I'm using a netServer, not a tlsServer.
sslOptions.server = server;

The tlswrapper attached here is what invokes new tls.TLSSocket(socket, sslOptions), you should take a look at it.

The code above is what is used in production and works for dozens or hundreds of requests (which means a few minutes) until it crashes, but I wasn't able to reproduce it on a test case. I'll keep working on that.

Also, I would like to point that it seems to call that requestOSCP() function even though I set it to false.

It works fine if I use any tlsServer instead of the netServer, by just changing the last line to this:

sslOptions.server = tls.createServer(sslOptions);
@webertrlz
Copy link
Author

Also I would like to point that in _tls_wrap.js:116:

ctx = self.server._sharedCreds;

Since I'm using a netServer, this property doesn't exist, that's why ctx is undefined, causing the crash.

indutny added a commit to indutny/io.js that referenced this issue Jan 9, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: nodejs#10704
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Jan 9, 2017
@sam-github sam-github added the confirmed-bug Issues with confirmed bugs. label Jan 13, 2017
addaleax pushed a commit that referenced this issue Feb 22, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: #10704
PR-URL: #10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell pushed a commit that referenced this issue Mar 7, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: #10704
PR-URL: #10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jasnell pushed a commit that referenced this issue Mar 7, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: #10704
PR-URL: #10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: #10704
PR-URL: #10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
`TLSSocket` should not have a hard dependency on `tls.Server`, since it
may be running without it in cases like `STARTTLS`.

Fix: #10704
PR-URL: #10706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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

3 participants