tls: server.addContext does not update the context if there was one registered for a given servername wildcard.
- Version: v15.0.0-pre (node master branch)
- Platform: irrelevant
- Subsystem: TLS
What steps will reproduce the bug?
I am having a hard time writing a test that showcases this behavior. Will try to deliver tomorrow. Meanwhile, an explaination with code snippets so I don't forget until morning:
in lib/_tls_wrap.js
, the definition of server.addContext
is pretty concise:
Server.prototype.addContext = function(servername, context) {
if (!servername) {
throw new ERR_TLS_REQUIRED_SERVER_NAME();
}
const re = new RegExp('^' +
servername.replace(/([.^$+?\-\\[\]{}])/g, '\\$1')
.replace(/\*/g, '[^.]*') +
'$');
this._contexts.push([re, tls.createSecureContext(context).context]);
};
As we can see, the new context is pushed to this._contexts
array.
Then, a few lines below, the definition of SNICallback
, a default SNICallback
used when no custom SNICallback
is provided for the server:
function SNICallback(servername, callback) {
const contexts = this.server._contexts;
for (const elem of contexts) {
if (elem[0].test(servername)) {
callback(null, elem[1]);
return;
}
}
callback(null, undefined);
}
We can observe that always the first, 'oldest' matching context will be used, no matter if a new context has been added for a matching servername.
This might be a security threat - the API user might be expecting the updated SecureContext to be used for a given servername after server.addContext
call, but it is not true, because the 'oldest' SecureContext is chosen.
How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior?
Probably the iteration in SNICallback
should be reversed to use the most recent matching SecureContext. Using a dictionary or updating values for already existing wildcards would be a little tricky, but maybe possible.
Proposed solution would then be:
function SNICallback(servername, callback) {
const contexts = this.server._contexts.reverse(); // reverse the contexts
// iterate latest first
for (const elem of contexts) {
if (elem[0].test(servername)) {
callback(null, elem[1]);
return;
}
}
callback(null, undefined);
}
What do you see instead?
The first added SecureContext
that matches servername
is used.
Additional information
servername
should also be case-insensitive.
I am working on it and should soon open a PR with a proposed fix. Please let me know if you agree on the reversed iteration change.