Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
tls: veryify server's identity
  • Loading branch information
indutny committed Jul 19, 2012
1 parent 53716eb commit eb2ca10
Show file tree
Hide file tree
Showing 4 changed files with 306 additions and 13 deletions.
14 changes: 6 additions & 8 deletions lib/http.js
Expand Up @@ -1123,13 +1123,11 @@ Agent.prototype.removeSocket = function(s, name, host, port, localAddress) {
}
if (this.requests[name] && this.requests[name].length) {
// If we have pending requests and a socket gets closed a new one
this.createSocket(
name,
host,
port,
localAddress,
this.requests[name][0]
).emit('free');
this.createSocket(name,
host,
port,
localAddress,
this.requests[name][0]).emit('free');
}
};

Expand All @@ -1141,7 +1139,7 @@ function ClientRequest(options, cb) {
var self = this;
OutgoingMessage.call(self);

this.options = options;
this.options = util._extend({}, options);
self.agent = options.agent === undefined ? globalAgent : options.agent;

var defaultPort = options.defaultPort || 80;
Expand Down
111 changes: 108 additions & 3 deletions lib/tls.js
Expand Up @@ -22,6 +22,7 @@
var crypto = require('crypto');
var util = require('util');
var net = require('net');
var url = require('url');
var events = require('events');
var Stream = require('stream');
var END_OF_FILE = 42;
Expand Down Expand Up @@ -77,6 +78,99 @@ function convertNPNProtocols(NPNProtocols, out) {
}
}


function checkServerIdentity(host, cert) {
// Create regexp to much hostnames
function regexpify(host, wildcards) {
// Add trailing dot (make hostnames uniform)
if (!/\.$/.test(host)) host += '.';

// Host names with less than one dots are considered too broad,
// and should not be allowed
if (!/^.+\..+$/.test(host)) return /$./;

// The same applies to hostname with more than one wildcard,
// if hostname has wildcard when wildcards are not allowed,
// or if there are less than two dots after wildcard (i.e. *.com or *d.com)
if (/\*.*\*/.test(host) || !wildcards && /\*/.test(host) ||
/\*/.test(host) && !/\*.*\..+\..+/.test(host)) {
return /$./;

This comment has been minimized.

Copy link
@jerem

jerem Jan 14, 2013

Did you mean /.$/ ? IMHO /$./ has no sense. Do you want a pull request?

This comment has been minimized.

Copy link
@indutny

indutny Jan 14, 2013

Author Member

No, I mean /$./. Which obviously is a regexp that pattern could never be satisfied.

This comment has been minimized.

Copy link
@jerem

jerem Jan 14, 2013

Strange, I have a case where host = "*.dropbox.com.", wildcards = false and the following test https://github.com/joyent/node/blob/master/lib/tls.js#L170 fails with host = 'api-content.dropbox.com.'

Looks like there is a problem somewhere. Any idea?

This comment has been minimized.

Copy link
@indutny

indutny Jan 14, 2013

Author Member

Can you create a reproducible test case and file an issue?

This comment has been minimized.

Copy link
@indutny

indutny Jan 14, 2013

Author Member

btw, wildcards = false - means that wildcards are not allowed in this certificate section.

This comment has been minimized.

Copy link
@indutny

indutny Jan 14, 2013

Author Member

This comment has been minimized.

Copy link
@jerem

jerem Jan 14, 2013

Here is the issue with the code snippet: #4592

}

// Replace wildcard chars with regexp's wildcard and
// escape all characters that have special meaning in regexps
// (i.e. '.', '[', '{', '*', and others)
var re = host.replace(
/\*([a-z0-9\\-_\.])|[\.,\-\\\^\$+?*\[\]\(\):!\|{}]/g,
function(all, sub) {
if (sub) return '[a-z0-9\\-_]*' + (sub === '-' ? '\\-' : sub);
return '\\' + all;
}
);

return new RegExp('^' + re + '$', 'i');
}

var dnsNames = [],
uriNames = [],
ips = [],
valid = false;

// There're several names to perform check against:
// CN and altnames in certificate extension
// (DNS names, IP addresses, and URIs)
//
// Walk through altnames and generate lists of those names
if (cert.subjectaltname) {
cert.subjectaltname.split(/, /g).forEach(function(altname) {
if (/^DNS:/.test(altname)) {
dnsNames.push(altname.slice(4));
} else if (/^IP Address:/.test(altname)) {
ips.push(altname.slice(11));
} else if (/^URI:/.test(altname)) {
var uri = url.parse(altname.slice(4));
if (uri) uriNames.push(uri.hostname);
}
});
}

// If hostname is an IP address, it should be present in the list of IP
// addresses.
if (net.isIP(host)) {
valid = ips.some(function(ip) {
return ip === host;
});
} else {
// Transform hostname to canonical form
if (!/\.$/.test(host)) host += '.';

// Otherwise check all DNS/URI records from certificate
// (with allowed wildcards)
dnsNames = dnsNames.map(function(name) {
return regexpify(name, true);
});

// Wildcards ain't allowed in URI names
uriNames = uriNames.map(function(name) {
return regexpify(name, false);
});

dnsNames = dnsNames.concat(uriNames);

// And only after check if hostname matches CN
// (because CN is deprecated, but should be used for compatiblity anyway)
dnsNames.push(regexpify(cert.subject.CN, false));

valid = dnsNames.some(function(re) {
return re.test(host);
});
}

return valid;
};
exports.checkServerIdentity = checkServerIdentity;


// Base class of both CleartextStream and EncryptedStream
function CryptoStream(pair) {
Stream.call(this);
Expand Down Expand Up @@ -1118,11 +1212,12 @@ exports.connect = function(/* [port, host], options, cb */) {
var sslcontext = crypto.createCredentials(options);

convertNPNProtocols(options.NPNProtocols, this);
var pair = new SecurePair(sslcontext, false, true,
var hostname = options.servername || options.host,
pair = new SecurePair(sslcontext, false, true,
options.rejectUnauthorized === true ? true : false,
{
NPNProtocols: this.NPNProtocols,
servername: options.servername || options.host
servername: hostname
});

if (options.session) {
Expand All @@ -1147,9 +1242,19 @@ exports.connect = function(/* [port, host], options, cb */) {

cleartext.npnProtocol = pair.npnProtocol;

// Verify that server's identity matches it's certificate's names
if (!verifyError) {
var validCert = checkServerIdentity(hostname,
pair.cleartext.getPeerCertificate());
if (!validCert) {
verifyError = new Error('Hostname/IP doesn\'t match certificate\'s ' +
'altnames');
}
}

if (verifyError) {
cleartext.authorized = false;
cleartext.authorizationError = verifyError;
cleartext.authorizationError = verifyError.message;

if (pair._rejectUnauthorized) {
cleartext.emit('error', verifyError);
Expand Down
5 changes: 3 additions & 2 deletions src/node_crypto.cc
Expand Up @@ -1547,7 +1547,8 @@ Handle<Value> Connection::VerifyError(const Arguments& args) {
// We requested a certificate and they did not send us one.
// Definitely an error.
// XXX is this the right error message?
return scope.Close(String::New("UNABLE_TO_GET_ISSUER_CERT"));
return scope.Close(Exception::Error(
String::New("UNABLE_TO_GET_ISSUER_CERT")));
}
X509_free(peer_cert);

Expand Down Expand Up @@ -1673,7 +1674,7 @@ Handle<Value> Connection::VerifyError(const Arguments& args) {
break;
}

return scope.Close(s);
return scope.Close(Exception::Error(s));
}


Expand Down
189 changes: 189 additions & 0 deletions test/simple/test-tls-check-server-identity.js
@@ -0,0 +1,189 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

var common = require('../common');
var assert = require('assert');
var util = require('util');
var tls = require('tls');

var tests = [
// Basic CN handling
{ host: 'a.com', cert: { subject: { CN: 'a.com' } }, result: true },
{ host: 'a.com', cert: { subject: { CN: 'A.COM' } }, result: true },
{ host: 'a.com', cert: { subject: { CN: 'b.com' } }, result: false },
{ host: 'a.com', cert: { subject: { CN: 'a.com.' } }, result: true },

// No wildcards in CN
{ host: 'b.a.com', cert: { subject: { CN: '*.a.com' } }, result: false },

// DNS names and CN
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*',
subject: { CN: 'b.com' }
},
result: false
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.com',
subject: { CN: 'b.com' }
},
result: false
},
{
host: 'a.co.uk', cert: {
subjectaltname: 'DNS:*.co.uk',
subject: { CN: 'b.com' }
},
result: true
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'a.com' }
},
result: true
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'b.com' }
},
result: false
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:a.com',
subject: { CN: 'b.com' }
},
result: true
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:A.COM',
subject: { CN: 'b.com' }
},
result: true
},

// DNS names
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: false
},
{
host: 'b.a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: true
},
{
host: 'c.b.a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: false
},
{
host: 'b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: true
},
{
host: 'a-cb.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: true
},
{
host: 'a.b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: false
},
// Mutliple DNS names
{
host: 'a.b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com, DNS:a.b.a.com',
subject: {}
},
result: true
},
// URI names
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://a.b.a.com/',
subject: {}
},
result: true
},
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://*.b.a.com/',
subject: {}
},
result: false
},
// IP addresses
{
host: 'a.b.a.com', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: false
},
{
host: '127.0.0.1', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: true
},
{
host: '127.0.0.2', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: false
},
{
host: '127.0.0.1', cert: {
subjectaltname: 'DNS:a.com',
subject: {}
},
result: false
},
];

tests.forEach(function(test, i) {
assert.equal(tls.checkServerIdentity(test.host, test.cert),
test.result,
'Test#' + i + ' failed: ' + util.inspect(test));
});

0 comments on commit eb2ca10

Please sign in to comment.