tls: return correct version from getCipher()

OpenSSL 1.0.0 returned incorrect version information. OpenSSL 1.1.0 fixed this, but returning the correct information broke our tests, so was considered semver-major. Because of this, the version was hard-coded to the OpenSSL 1.0.0 (incorrect) string in 5fe81c8. This is ancient history, start returning the correct cipher version. PR-URL: #26625 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Mar 18, 2019 · 0f745bf · 0f745bf
1 parent 1ceb6d8
commit 0f745bf
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 32 deletions.
diff --git a/doc/api/tls.md b/doc/api/tls.md
@@ -717,18 +717,25 @@ socket has been destroyed, `null` will be returned.
 ### tlsSocket.getCipher()
 <!-- YAML
 added: v0.11.4
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/26625
+    description: Return the minimum cipher version, instead of a fixed string
+      (`'TLSv1/SSLv3'`).
 -->
 
 * Returns: {Object}
+  * `name` {string} The name of the cipher suite.
+  * `version` {string} The minimum TLS protocol version supported by this cipher
+    suite.
 
-Returns an object representing the cipher name. The `version` key is a legacy
-field which always contains the value `'TLSv1/SSLv3'`.
+Returns an object containing information on the negotiated cipher suite.
 
 For example: `{ name: 'AES256-SHA', version: 'TLSv1/SSLv3' }`.
 
-See `SSL_CIPHER_get_name()` in
-<https://www.openssl.org/docs/man1.1.0/ssl/SSL_CIPHER_get_name.html> for more
-information.
+See
+[OpenSSL](https://www.openssl.org/docs/man1.1.1/man3/SSL_CIPHER_get_name.html)
+for more information.
 
 ### tlsSocket.getEphemeralKeyInfo()
 <!-- YAML

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
@@ -737,18 +737,18 @@ function makeSocketMethodProxy(name) {
 }
 
 [
-  'getFinished', 'getPeerFinished', 'getSession', 'isSessionReused',
-  'getEphemeralKeyInfo', 'getProtocol', 'getTLSTicket'
+  'getCipher',
+  'getEphemeralKeyInfo',
+  'getFinished',
+  'getPeerFinished',
+  'getProtocol',
+  'getSession',
+  'getTLSTicket',
+  'isSessionReused',
 ].forEach((method) => {
   TLSSocket.prototype[method] = makeSocketMethodProxy(method);
 });
 
-TLSSocket.prototype.getCipher = function(err) {
-  if (this._handle)
-    return this._handle.getCurrentCipher();
-  return null;
-};
-
 // TODO: support anonymous (nocert) and PSK
 
 

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -1464,7 +1464,7 @@ void SSLWrap<Base>::AddMethods(Environment* env, Local<FunctionTemplate> t) {
   env->SetProtoMethod(t, "loadSession", LoadSession);
   env->SetProtoMethodNoSideEffect(t, "isSessionReused", IsSessionReused);
   env->SetProtoMethodNoSideEffect(t, "verifyError", VerifyError);
-  env->SetProtoMethodNoSideEffect(t, "getCurrentCipher", GetCurrentCipher);
+  env->SetProtoMethodNoSideEffect(t, "getCipher", GetCipher);
   env->SetProtoMethod(t, "endParser", EndParser);
   env->SetProtoMethod(t, "certCbDone", CertCbDone);
   env->SetProtoMethod(t, "renegotiate", Renegotiate);
@@ -2357,7 +2357,7 @@ void SSLWrap<Base>::VerifyError(const FunctionCallbackInfo<Value>& args) {
 
 
 template <class Base>
-void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
+void SSLWrap<Base>::GetCipher(const FunctionCallbackInfo<Value>& args) {
   Base* w;
   ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
   Environment* env = w->ssl_env();
@@ -2371,8 +2371,9 @@ void SSLWrap<Base>::GetCurrentCipher(const FunctionCallbackInfo<Value>& args) {
   const char* cipher_name = SSL_CIPHER_get_name(c);
   info->Set(context, env->name_string(),
             OneByteString(args.GetIsolate(), cipher_name)).FromJust();
+  const char* cipher_version = SSL_CIPHER_get_version(c);
   info->Set(context, env->version_string(),
-            OneByteString(args.GetIsolate(), "TLSv1/SSLv3")).FromJust();
+            OneByteString(args.GetIsolate(), cipher_version)).FromJust();
   args.GetReturnValue().Set(info);
 }
 

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -281,7 +281,7 @@ class SSLWrap {
   static void LoadSession(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void IsSessionReused(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void GetCurrentCipher(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void GetCipher(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void EndParser(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void CertCbDone(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Renegotiate(const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/test/parallel/test-tls-getcipher.js b/test/parallel/test-tls-getcipher.js
@@ -30,28 +30,42 @@ const tls = require('tls');
 // Import fixtures directly from its module
 const fixtures = require('../common/fixtures');
 
-const cipher_list = ['AES128-SHA256', 'AES256-SHA256'];
-const cipher_version_pattern = /TLS|SSL/;
 const options = {
   key: fixtures.readKey('agent2-key.pem'),
   cert: fixtures.readKey('agent2-cert.pem'),
-  ciphers: cipher_list.join(':'),
   honorCipherOrder: true
 };
 
-const server = tls.createServer(options, common.mustCall());
+let clients = 0;
+const server = tls.createServer(options, common.mustCall(() => {
+  if (--clients === 0)
+    server.close();
+}, 2));
 
 server.listen(0, '127.0.0.1', common.mustCall(function() {
-  const client = tls.connect({
+  clients++;
+  tls.connect({
     host: '127.0.0.1',
     port: this.address().port,
-    ciphers: cipher_list.join(':'),
+    ciphers: 'AES128-SHA256',
     rejectUnauthorized: false
   }, common.mustCall(function() {
-    const cipher = client.getCipher();
-    assert.strictEqual(cipher.name, cipher_list[0]);
-    assert(cipher_version_pattern.test(cipher.version));
-    client.end();
-    server.close();
+    const cipher = this.getCipher();
+    assert.strictEqual(cipher.name, 'AES128-SHA256');
+    assert.strictEqual(cipher.version, 'TLSv1.2');
+    this.end();
+  }));
+
+  clients++;
+  tls.connect({
+    host: '127.0.0.1',
+    port: this.address().port,
+    cipher: 'ECDHE-RSA-AES128-GCM-SHA256',
+    rejectUnauthorized: false
+  }, common.mustCall(function() {
+    const cipher = this.getCipher();
+    assert.strictEqual(cipher.name, 'ECDHE-RSA-AES128-GCM-SHA256');
+    assert.strictEqual(cipher.version, 'TLSv1.2');
+    this.end();
   }));
 }));
diff --git a/test/parallel/test-tls-multi-key.js b/test/parallel/test-tls-multi-key.js
@@ -157,7 +157,7 @@ function test(options) {
     }, common.mustCall(function() {
       assert.deepStrictEqual(ecdsa.getCipher(), {
         name: 'ECDHE-ECDSA-AES256-GCM-SHA384',
-        version: 'TLSv1/SSLv3'
+        version: 'TLSv1.2'
       });
       assert.strictEqual(ecdsa.getPeerCertificate().subject.CN, eccCN);
       // XXX(sam) certs don't currently include EC key info, so depend on
@@ -177,7 +177,7 @@ function test(options) {
     }, common.mustCall(function() {
       assert.deepStrictEqual(rsa.getCipher(), {
         name: 'ECDHE-RSA-AES256-GCM-SHA384',
-        version: 'TLSv1/SSLv3'
+        version: 'TLSv1.2'
       });
       assert.strictEqual(rsa.getPeerCertificate().subject.CN, rsaCN);
       assert(rsa.getPeerCertificate().exponent, 'cert for an RSA key');

diff --git a/test/parallel/test-tls-multi-pfx.js b/test/parallel/test-tls-multi-pfx.js
@@ -42,9 +42,9 @@ const server = tls.createServer(options, function(conn) {
 process.on('exit', function() {
   assert.deepStrictEqual(ciphers, [{
     name: 'ECDHE-ECDSA-AES256-GCM-SHA384',
-    version: 'TLSv1/SSLv3'
+    version: 'TLSv1.2'
   }, {
     name: 'ECDHE-RSA-AES256-GCM-SHA384',
-    version: 'TLSv1/SSLv3'
+    version: 'TLSv1.2'
   }]);
 });