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: fix session resumption check #2312

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -584,17 +584,6 @@ TLSSocket.prototype._start = function() {
this._handle.start();
};

TLSSocket.prototype._isSessionResumed = function _isSessionResumed(session) {
if (!session)
return false;

var next = this.getSession();
if (!next)
return false;

return next.equals(session);
};

TLSSocket.prototype.setServername = function(name) {
this._handle.setServername(name);
};
Expand Down Expand Up @@ -1011,7 +1000,7 @@ exports.connect = function(/* [port, host], options, cb */) {

// Verify that server's identity matches it's certificate's names
// Unless server has resumed our existing session
if (!verifyError && !socket._isSessionResumed(options.session)) {
if (!verifyError && !socket.isSessionReused()) {
var cert = socket.getPeerCertificate();
verifyError = options.checkServerIdentity(hostname, cert);
}
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ namespace node {
V(syscall_string, "syscall") \
V(tick_callback_string, "_tickCallback") \
V(tick_domain_cb_string, "_tickDomainCallback") \
V(ticketkeycallback_string, "onticketkeycallback") \
V(timeout_string, "timeout") \
V(times_string, "times") \
V(timestamp_string, "timestamp") \
Expand Down
104 changes: 104 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,23 @@ void SecureContext::Initialize(Environment* env, Handle<Object> target) {
env->SetProtoMethod(t, "getTicketKeys", SecureContext::GetTicketKeys);
env->SetProtoMethod(t, "setTicketKeys", SecureContext::SetTicketKeys);
env->SetProtoMethod(t, "setFreeListLength", SecureContext::SetFreeListLength);
env->SetProtoMethod(t,
"enableTicketKeyCallback",
SecureContext::EnableTicketKeyCallback);
env->SetProtoMethod(t, "getCertificate", SecureContext::GetCertificate<true>);
env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate<false>);

t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyReturnIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyReturnIndex));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyHMACIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyHMACIndex));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyAESIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyAESIndex));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyNameIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyNameIndex));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyIVIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyIVIndex));

t->PrototypeTemplate()->SetAccessor(
FIXED_ONE_BYTE_STRING(env->isolate(), "_external"),
CtxGetter,
Expand Down Expand Up @@ -378,6 +392,7 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
}

sc->ctx_ = SSL_CTX_new(method);
SSL_CTX_set_app_data(sc->ctx_, sc);

// Disable SSLv2 in the case when method == SSLv23_method() and the
// cipher list contains SSLv2 ciphers (not the default, should be rare.)
Expand Down Expand Up @@ -982,6 +997,95 @@ void SecureContext::SetFreeListLength(const FunctionCallbackInfo<Value>& args) {
}


void SecureContext::EnableTicketKeyCallback(
const FunctionCallbackInfo<Value>& args) {
SecureContext* wrap = Unwrap<SecureContext>(args.Holder());

SSL_CTX_set_tlsext_ticket_key_cb(wrap->ctx_, TicketKeyCallback);
}


int SecureContext::TicketKeyCallback(SSL* ssl,
unsigned char* name,
unsigned char* iv,
EVP_CIPHER_CTX* ectx,
HMAC_CTX* hctx,
int enc) {
static const int kTicketPartSize = 16;

SecureContext* sc = static_cast<SecureContext*>(
SSL_CTX_get_app_data(ssl->ctx));

Environment* env = sc->env();
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());

Local<Value> argv[] = {
Buffer::New(env,
reinterpret_cast<char*>(name),
kTicketPartSize).ToLocalChecked(),
Buffer::New(env,
reinterpret_cast<char*>(iv),
kTicketPartSize).ToLocalChecked(),
Boolean::New(env->isolate(), enc != 0)
};
Local<Value> ret = node::MakeCallback(env,
sc->object(),
env->ticketkeycallback_string(),
ARRAY_SIZE(argv),
argv);
Local<Array> arr = ret.As<Array>();

int r = arr->Get(kTicketKeyReturnIndex)->Int32Value();
if (r < 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should care r=0 case. Is it better return 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are -1, 0, 1, 2 return values. It is not better to return 0, it is just use-case dependent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=0 means

       0   This indicates that it was not possible to set/retrieve a session
           ticket and the SSL/TLS session will continue by by negiotationing a
           set of cryptographic parameters or using the alternate SSL/TLS
           resumption mechanism, session ids.

So when enableTicketKeyCallback is returned with r=0, does it cause issues in iniitalizing the following HMAC, EVP_Encrypt/EVP_Decrypt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, OpenSSL is using them when r = 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried,

diff --git a/test/parallel/test-https-resume-after-renew.js b/test/parallel/test-https-resume-after-renew.js
index 23626cc..25e1e80 100644
--- a/test/parallel/test-https-resume-after-renew.js
+++ b/test/parallel/test-https-resume-after-renew.js
@@ -21,6 +21,7 @@ hmac.fill('H');

 server._sharedCreds.context.enableTicketKeyCallback();
 server._sharedCreds.context.onticketkeycallback = function(name, iv, enc) {
+/*
   if (enc) {
     var newName = new Buffer(16);
     var newIV = crypto.randomBytes(16);
@@ -31,6 +32,8 @@ server._sharedCreds.context.onticketkeycallback = function(name, iv, enc) {
   }

   return [ 1, hmac, aes, newName, newIV ];
+*/
+  return [0];
 };

causes

ohtsu@ubuntu:~/github/io.js$ ./iojs test/parallel/test-https-resume-after-renew.js
iojs: ../src/node_buffer.cc:196: size_t node::Buffer::Length(v8::Handle<v8::Value>): Assertion `val->IsObject()' failed.
Aborted (core dumped)

Is is better to avoid this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Sorry, this is buffer error. Not related crypt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny return [0, hmac, aes, name, iv]; has no error. Never mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. This function will be provided in core, and core will be responsible for setting return array right.

return r;

Local<Value> hmac = arr->Get(kTicketKeyHMACIndex);
Local<Value> aes = arr->Get(kTicketKeyAESIndex);
if (Buffer::Length(aes) != kTicketPartSize)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why hmac length is not checked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be any.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length 0 of

diff --git a/test/parallel/test-https-resume-after-renew.js b/test/parallel/test-https-resume-after-renew.js
index 23626cc..6a1d6b5 100644
--- a/test/parallel/test-https-resume-after-renew.js
+++ b/test/parallel/test-https-resume-after-renew.js
@@ -16,7 +16,7 @@ var server = https.createServer(options, function(req, res) {

 var aes = new Buffer(16);
 aes.fill('S');
-var hmac = new Buffer(16);
+var hmac = new Buffer(0);
 hmac.fill('H');

 server._sharedCreds.context.enableTicketKeyCallback();

cases

$ ./iojs test/parallel/test-https-resume-after-renew.js
events.js:141
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at TLSSocket.onHangUp (_tls_wrap.js:1031:19)
    at TLSSocket.g (events.js:260:16)
    at emitNone (events.js:72:20)
    at TLSSocket.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:889:12)
    at doNTCallback2 (node.js:429:9)
    at process._tickCallback (node.js:343:17)

Off course we can check it in js land. Do you think it is not needed neither?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, checking it won't yield any other result :)

return -1;

if (enc) {
Local<Value> name_val = arr->Get(kTicketKeyNameIndex);
Local<Value> iv_val = arr->Get(kTicketKeyIVIndex);

if (Buffer::Length(name_val) != kTicketPartSize ||
Buffer::Length(iv_val) != kTicketPartSize) {
return -1;
}

memcpy(name, Buffer::Data(name_val), kTicketPartSize);
memcpy(iv, Buffer::Data(iv_val), kTicketPartSize);
}

HMAC_Init_ex(hctx,
Buffer::Data(hmac),
Buffer::Length(hmac),
EVP_sha256(),
nullptr);

const unsigned char* aes_key =
reinterpret_cast<unsigned char*>(Buffer::Data(aes));
if (enc) {
EVP_EncryptInit_ex(ectx,
EVP_aes_128_cbc(),
nullptr,
aes_key,
iv);
} else {
EVP_DecryptInit_ex(ectx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check ticket name to confirm the key and iv are right one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shigeki I think this could be done in JS land? We are passing the name to it anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have my preference to have

return [ r, name, aes, iv, hmac];

Should I better to wait until you have complete JS land?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, this isn't going to happen in this PR.

EVP_aes_128_cbc(),
nullptr,
aes_key,
iv);
}

return r;
}




void SecureContext::CtxGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
HandleScope scope(info.GetIsolate());
Expand Down
16 changes: 16 additions & 0 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ class SecureContext : public BaseObject {

static const int kMaxSessionSize = 10 * 1024;

// See TicketKeyCallback
static const int kTicketKeyReturnIndex = 0;
static const int kTicketKeyHMACIndex = 1;
static const int kTicketKeyAESIndex = 2;
static const int kTicketKeyNameIndex = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think key name index is number 1 and it is mandate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the indexes in array that should be returned by JS callback, not indexes of inputs.

static const int kTicketKeyIVIndex = 4;

protected:
static const int64_t kExternalSize = sizeof(SSL_CTX);

Expand All @@ -92,12 +99,21 @@ class SecureContext : public BaseObject {
static void SetTicketKeys(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetFreeListLength(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void EnableTicketKeyCallback(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void CtxGetter(v8::Local<v8::String> property,
const v8::PropertyCallbackInfo<v8::Value>& info);

template <bool primary>
static void GetCertificate(const v8::FunctionCallbackInfo<v8::Value>& args);

static int TicketKeyCallback(SSL* ssl,
unsigned char* name,
unsigned char* iv,
EVP_CIPHER_CTX* ectx,
HMAC_CTX* hctx,
int enc);

SecureContext(Environment* env, v8::Local<v8::Object> wrap)
: BaseObject(env, wrap),
ca_store_(nullptr),
Expand Down
56 changes: 56 additions & 0 deletions test/parallel/test-https-resume-after-renew.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';
var common = require('../common');
var fs = require('fs');
var https = require('https');
var crypto = require('crypto');

var options = {
key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem'),
ca: fs.readFileSync(common.fixturesDir + '/keys/ca1-cert.pem')
};

var server = https.createServer(options, function(req, res) {
res.end('hello');
});

var aes = new Buffer(16);
aes.fill('S');
var hmac = new Buffer(16);
hmac.fill('H');

server._sharedCreds.context.enableTicketKeyCallback();
server._sharedCreds.context.onticketkeycallback = function(name, iv, enc) {
if (enc) {
var newName = new Buffer(16);
var newIV = crypto.randomBytes(16);
newName.fill('A');
} else {
// Renew
return [ 2, hmac, aes ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to add ticket name even in rekey?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not updated in case of decryption, and this is the test. I'd say no point in doing it here.

}

return [ 1, hmac, aes, newName, newIV ];
};

server.listen(common.PORT, function() {
var addr = this.address();

function doReq(callback) {
https.request({
method: 'GET',
port: addr.port,
servername: 'agent1',
ca: options.ca
}, function(res) {
res.resume();
res.once('end', callback);
}).end();
}

doReq(function() {
doReq(function() {
server.close();
});
});
});