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

tls: server.addContext does not update the context if there was one registered for a given servername wildcard. #34110

Closed
mkrawczuk opened this issue Jun 29, 2020 · 0 comments · Fixed by #36072
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@mkrawczuk
Copy link
Contributor

mkrawczuk commented Jun 29, 2020

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
2 participants