Reusing TLS sessions causes to `socket.servername` being false

[szmarczak]

• Version: v12.6.0
• Platform: Linux solus 5.1.14-121.current deps: update openssl to 1.0.1j #1 SMP PREEMPT Sun Jun 23 13:57:07 UTC 2019 x86_64 GNU/Linux
• Subsystem: tls, http2

https://runkit.com/szmarczak/5d48640bd3cab4001399357c

The issue happens randomly. Sometimes socket.servername is set to false, sometimes it's a string. I tracked the source down to this C++ code:

node/src/tls_wrap.cc

Lines 991 to 1006 in 0481a7f

	void TLSWrap::GetServername(const FunctionCallbackInfo<Value>& args) {
	Environment* env = Environment::GetCurrent(args);
	TLSWrap* wrap;
	ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
	CHECK_NOT_NULL(wrap->ssl_);
	const char* servername = SSL_get_servername(wrap->ssl_.get(),
	TLSEXT_NAMETYPE_host_name);
	if (servername != nullptr) {
	args.GetReturnValue().Set(OneByteString(env->isolate(), servername));
	} else {
	args.GetReturnValue().Set(false);
	}
	}

servername sometimes is a null pointer. This causes http2session.originSet to look like:

[ 'https://false:35575' ]

[mscdex]

I can't reproduce this with node master, v12.7.0, or v8.x. I can only reproduce it with node v10.x. Also, the runkit link is using node v10.16.0.

[szmarczak]

On my machine the example shows false. I'll try to think of another example.

[szmarczak]

Do:

git clone https://github.com/szmarczak/http2-wrapper
cd http2-wrapper
git checkout bug
npm install
ava test/agent.js -v

Here's what I get every time:

[ 'https://false:36827' ]

[szmarczak]

The issue still occurs on v12.7.0.

[szmarczak]

const http2 = require('http2');

const session = http2.connect('https://google.com', {servername: 'google.com'});
session.socket.once('secureConnect', () => {
    const tlsSession = session.socket.getSession();
    
    session.close();
    session.once('close', () => {
        const secondSession = http2.connect('https://google.com', {servername: 'google.com', session: tlsSession});
        secondSession.socket.once('secureConnect', () => {
            console.log(secondSession.socket.servername);
        });
        secondSession.once('localSettings', () => {
            console.log(secondSession.originSet);
        });
    });
});

Very rarely shows [ 'https://false' ].

[szmarczak]

Any news on this?

[szmarczak]

Hello? Anybody there? It's been a month since the creation of this issue.

The bug breaks the whole originSet functionality.

[mscdex]

/cc @nodejs/crypto

[bnoordhuis]

I suspect it's this line that's at fault:

node/lib/internal/http2/core.js

Line 560 in e585caa

if (socket.servername != null) {

It should probably be something like if (typeof socket.servername === 'string') {

I'm changing the labels because it's not a tls bug: the servername doesn't need to be present, it's optionally sent by the client as an extension in the ClientHello. The http2 code should handle that.

[szmarczak]

@bnoordhuis That would case to originSet being empty. It cannot be empty. There needs to be at least one origin.