Skip to content

undefined is passed as hostname to tls.checkServerIdentity #1489

Closed
Closed
undefined is passed as hostname to tls.checkServerIdentity#1489
Labels
confirmed-bugIssues and PRs for confirmed bugs.tlsIssues and PRs related to the tls subsystem.

Description

@sitegui
sitegui
opened on Apr 21, 2015

Hi all,

When using tls.connect(options) with no options.host I get this:

Hostname/IP doesn't match certificate's altnames: "Host: undefined. is not cert's CN: localhost"

I tried to track this and it seems tls.checkServerIdentity get called with hostname as undefined.

Examining the stack, it gets called by _tls_wrap.js:913 and hostname is defined in _tls_wrap.js:859.

As you can see, hostname is undefined because neither options.servername, options.host nor options.socket are set.

The tls.connect doc says: "If host is omitted, it defaults to localhost", so shouldn't it actually set options.host to "localhost" in this case?

Adding it to defaults may solve this, but I have very little understanding of this part of the code to be sure.

Is it a bug or intended?

Best regards

Activity

  1. sitegui commented on Apr 21, 2015

    @sitegui
    sitegui
    on Apr 21, 2015
    ContributorAuthor

    Code's better than words:

    'use strict'

var tls = require('tls'),
    https = require('https')

https.createServer({
    key: key(),
    cert: cert()
}).listen(8000)

tls.connect({
    port: 8000,
    ca: cert(),
    //host: 'localhost' // <-- comment out and it will work!
}, function () {
    console.log('OK')
    process.exit()
})

function key() {
    return '-----BEGIN RSA PRIVATE KEY-----\n' +
        'MIIEowIBAAKCAQEA5m2YXNXrx4ADNNvwlMai26n5v2xVYnAPQzXZbeBD5JxaHLYB\n' +
        'RqZin/rVKNH209l3pwlgcvZ9jLIdAlyAg13gXlNCcPg2RgEjhFg0t0cf9bYizjnd\n' +
        '4LCIhwDR8fCfx8B3VWoJRmD8dl2kZuemIGzx6Zr/AVi5vpEQOSMB76m2UZZMeIKz\n' +
        'Il/uqkGVGuC83NL1/FGw4v9XqRG3/NxXmNzdRPe1D4vjn6sRoFEQ7qPL0qjmhwWZ\n' +
        'lCKW9ZglwSlochMBEnwue2eKQM87vx4MpujQksVZYd+h7vvVWTUFE06PNCFlNcSU\n' +
        'MgwasDFPqwHDb2XnMkpo+AFjCLdLQJ5qZpe9YQIDAQABAoIBAQC2UK5Ffahgn4Np\n' +
        '9j8Cp6tBW9pTv5ZLHVimF9whmFh/b8nIf6TAznKoG2E+O+osMhr+mWerbiVmBaL4\n' +
        'NFImHkegugWOtoTSnKIKW3PSMz8xPNuLCbPozCQplNeHspfpBvokJZKTEbeOu4aR\n' +
        'OOVzMF+zMkRjP10vTz1jx7QHeOLc5yrhv9PDR348BLMWRYQViqHR5WfKJ+vCyDdW\n' +
        'nPDgaCiu19xZr4tEocRFJ8XtL1r9171nVjAgQXEPBDU65vTfxWrm9AXNNf8fOzb0\n' +
        'UBP0QkS1IS1ryum9g7XD5MItz5WswU5EU4CZfaYMofHNcOW0baHD/UV+MWkJauYn\n' +
        'QZMe7L6FAoGBAPNKMpkutZUVrtJQZGnz0tw8tCtC2NjTCyFGp5o6StU5gP5U65c0\n' +
        'hgADlxFM7U5J+3zkvMCEwxZPhVQspaAaqkxVvKZ6Oc/oRbH24644bRM2+jsbOu/m\n' +
        '8SZCL+7k6lTkIXeWeFVEbHi68I/HI2Vsa/PfAQxDnklLt8g1hOJwd1JDAoGBAPJ3\n' +
        'YebboXSuoZslj6F/YP951tAX0MyAga2hWwThcO7gMqQZUGXVYTUQwGbIBdjtYmNk\n' +
        'pI0OoneHG2w2YSK10som/lYjsIIfLVka9nBceNJylpmuo+rtuaGb3OxrxRcJJL0W\n' +
        'jZKwoVaT9/2IMb+BrcGcfN9s1ImhziOEFAEz3/GLAoGASQcEmSaEKvQPPeITwhoG\n' +
        'OUWfbzzpimwO8zYaKRlGTSqtpaon7YM+ldJ+DhthQBbE/oBKiB9Rz+iexN2B+cUH\n' +
        'SVKTBgW6RMYb5YeOYEVfuFzQT92km05fJHTJnpPoIwM3aIYqKK4ZQUQb4YyM+2zI\n' +
        'GrPdxKinYqjvyZEHClFn/7ECgYBxpKzZZGW3Z8ZNDnzUh/xxoayiWhc+Upj1RaSA\n' +
        'lB23iJOTwF2jbTCji5dyVRwQgarUxS4vAwX5GfUrcg1zFF+Y6k/ZFd88DdrWYcHS\n' +
        'BjWHBbg6jdU8XnHcIk6Y7SYyVtHGYpS2hV0JVE8uoLAYf3JuRadtnPe9Dn6svNIX\n' +
        'gjXbYwKBgDnz3Vg2+MwA7d+DaJ8nazOZlYJVBfLdF9E5MoKg+CFrjbmQm3vWLlg1\n' +
        'twjP8r56/GZuPQ/NHWygX5BXAw+7xCasD5g6Ek6wDVdYzlfE9xZ5H35TP39mYY8R\n' +
        'Vyi2jyjRXvxa9BDSVTPLbxk/KZqpHQOuvvLQ/zLCBUWhBl8pC9I2\n' +
        '-----END RSA PRIVATE KEY-----\n'
}

function cert() {
    return '-----BEGIN CERTIFICATE-----\n' +
        'MIIDNDCCAhwCCQDIfMNgGGDxHjANBgkqhkiG9w0BAQsFADBcMQswCQYDVQQGEwJC\n' +
        'UjESMBAGA1UECBMJU2FvIFBhdWxvMREwDwYDVQQHEwhDYW1waW5hczESMBAGA1UE\n' +
        'ChMJbG9jYWxob3N0MRIwEAYDVQQDEwlsb2NhbGhvc3QwHhcNMTUwNDIxMDkzOTM5\n' +
        'WhcNMjUwNDE4MDkzOTM5WjBcMQswCQYDVQQGEwJCUjESMBAGA1UECBMJU2FvIFBh\n' +
        'dWxvMREwDwYDVQQHEwhDYW1waW5hczESMBAGA1UEChMJbG9jYWxob3N0MRIwEAYD\n' +
        'VQQDEwlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDm\n' +
        'bZhc1evHgAM02/CUxqLbqfm/bFVicA9DNdlt4EPknFoctgFGpmKf+tUo0fbT2Xen\n' +
        'CWBy9n2Msh0CXICDXeBeU0Jw+DZGASOEWDS3Rx/1tiLOOd3gsIiHANHx8J/HwHdV\n' +
        'aglGYPx2XaRm56YgbPHpmv8BWLm+kRA5IwHvqbZRlkx4grMiX+6qQZUa4Lzc0vX8\n' +
        'UbDi/1epEbf83FeY3N1E97UPi+OfqxGgURDuo8vSqOaHBZmUIpb1mCXBKWhyEwES\n' +
        'fC57Z4pAzzu/Hgym6NCSxVlh36Hu+9VZNQUTTo80IWU1xJQyDBqwMU+rAcNvZecy\n' +
        'Smj4AWMIt0tAnmpml71hAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAI4GtZhFqSn/\n' +
        'nZpwWFBAfLPqjo8gvhDuqDsn9HrPrlIiei2ZhMTSwZy34StRD3NRdzQe+KgZNI/c\n' +
        'MQlA6+8KDoWPlXL1Wk730WYfCHmiBtc8hZNCIPcxOukJByPJzxIojSt1Fg4RDrKk\n' +
        'qx4tUb8MHN0l+33GobwSPSAHWbOHiITKxv0lOoAtfXyArXx++K9THbohmdchSSa5\n' +
        'zIfYqp7pWaBgQRCpOyAWIHD/PSYmoIW2dbrr50W5xXQWJNeX4VdrkAjNvGBniBEV\n' +
        'wlig5Vq2YvkaGDo2fGVc++7mnkQl41aEmJ2M1OJYLWke6MOMu535VOmy0TEZu5lw\n' +
        'zIHV0FIGeGE=\n' +
        '-----END CERTIFICATE-----\n'
}
  2. mscdex
    added
    tlsIssues and PRs related to the tls subsystem.
    on Apr 21, 2015
  3. silverwind
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Apr 21, 2015

  4. Fishrock123 commented on Apr 21, 2015

    @Fishrock123
    Fishrock123
    on Apr 21, 2015
    Contributor

    -----BEGIN RSA PRIVATE KEY-----

    Do note this is now public. :)

    Hmm, so, confirmed from that test case, and this patch appears to fix it:

    diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 3e091b0..c1037a7 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -858,7 +858,8 @@ exports.connect = function(/* [port, host], options, cb */) {

   var hostname = options.servername ||
                  options.host ||
-                 options.socket && options.socket._host,
+                 (options.socket && options.socket._host) ||
+                 'localhost',
       NPN = {},
       context = tls.createSecureContext(options);
   tls.convertNPNProtocols(options.NPNProtocols, NPN);

    However, our tests don't appear to specify a host and yet they don't have a problem? I'm kinda confused.

  5. silverwind commented on Apr 21, 2015

    @silverwind
    silverwind
    on Apr 21, 2015
    Contributor

    That cert is self-signed for localhost, so no issue :)

    $ openssl s_client -connect localhost:8000
CONNECTED(00000003)
depth=0 /C=BR/ST=Sao Paulo/L=Campinas/O=localhost/CN=localhost
verify error:num=18:self signed certificate
verify return:1
depth=0 /C=BR/ST=Sao Paulo/L=Campinas/O=localhost/CN=localhost
verify return:1

    I think the suggested fix is spot-on. @sitegui would you like to submit a PR with this patch and your test case added?

  6. silverwind commented on Apr 21, 2015

    @silverwind
    silverwind
    on Apr 21, 2015
    Contributor

    @Fishrock123's patch looks to be safer than adding it to defaults.

  7. Fishrock123 commented on Apr 21, 2015

    @Fishrock123
    Fishrock123
    on Apr 21, 2015
    Contributor

    I think the suggested fix is spot-on. Guilherme Souza would you like to submit a PR with this patch and your test case added?

    Again, see the above, not so sure.

    For the test, I suggest just using the built in certs that are read from fs in other tests.

  8. silverwind commented on Apr 21, 2015

    @silverwind
    silverwind
    on Apr 21, 2015
    Contributor

    Yeah, something is a bit fishy here, it's strange that tls.connect does throw in this case, when the docs for the secureConnect event say

    This event is emitted after a new connection has been successfully handshaked. The listener will be called no matter if the server's certificate was authorized or not.

    Above self-signed cert seems to be authorized by the ca option, I'm not sure the throw is correct in this case.

    cc: @indutny

  9. silverwind commented on Apr 21, 2015

    @silverwind
    silverwind
    on Apr 21, 2015
    Contributor

    @Fishrock123 Regarding existing tests: They mostly use rejectUnauthorized: false which suppresses the cert errors. test-tls-connect being the exception, it connects to an nonexistant server.

    Still, the fact that tls.connect does throw on cert errors puzzles me, maybe the docs aren't up to date?

  10. silverwind commented on Apr 21, 2015

    @silverwind
    silverwind
    on Apr 21, 2015
    Contributor

    Nevermind my error ramblings, I was just confused about how errors were returned here.

    The test_cert.pem our tests use are already signed to localhost, so they're fine to test this with.

  11. sitegui commented on Apr 21, 2015

    @sitegui
    sitegui
    on Apr 21, 2015
    ContributorAuthor

    Thanks for the quick follow up guys.

    @Fishrock123

    Do note this is now public. :)

    It was already, no problem.

    @silverwind

    @sitegui would you like to submit a PR with this patch and your test case added?

    Yes, I'll do this is a moment.

    @silverwind

    @Fishrock123's patch looks to be safer than adding it to defaults

    It seems a better solution indeed, since host must be present only if neither servername nor socket are set.

  12. silverwind
    added a commit that references this issue on Apr 23, 2015

    tls_wrap: use localhost if options.host is empty

    a7d7463

  13. silverwind commented on Apr 23, 2015

    @silverwind
    silverwind
    on Apr 23, 2015
    Contributor

    Fixed in a7d7463

  14. silverwind
    closed this as completedon Apr 23, 2015
  15. Fishrock123
    added a commit that references this issue on Apr 29, 2015

    tls_wrap: use localhost if options.host is empty

    5eea0de
  16. Fishrock123
    added a commit that references this issue on May 14, 2015

    tls_wrap: use localhost if options.host is empty

    71274b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.tlsIssues and PRs related to the tls subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions