From 0f745bf9bde4e19dd352f10299d4365a4edaf386 Mon Sep 17 00:00:00 2001
From: Sam Roberts <vieuxtech@gmail.com>
Date: Tue, 12 Mar 2019 12:09:24 -0700
Subject: [PATCH] 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 5fe81c8aff03261.

This is ancient history, start returning the correct cipher version.

PR-URL: https://github.com/nodejs/node/pull/26625
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 doc/api/tls.md                      | 17 ++++++++++----
 lib/_tls_wrap.js                    | 16 ++++++-------
 src/node_crypto.cc                  |  7 +++---
 src/node_crypto.h                   |  2 +-
 test/parallel/test-tls-getcipher.js | 36 ++++++++++++++++++++---------
 test/parallel/test-tls-multi-key.js |  4 ++--
 test/parallel/test-tls-multi-pfx.js |  4 ++--
 7 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/doc/api/tls.md b/doc/api/tls.md
index d4c635963b34e7..fa34711fa81a12 100644
--- 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
index 9f6db7c8de88fd..18f0ca5900ca59 100644
--- 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
index 03ea9c3cf725ca..2776c484d8d938 100644
--- 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
index 1c66b318a2e97d..a0551651fbef94 100644
--- 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
index 37677ada7f411c..f819161f7f4dcc 100644
--- 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
index 8ccc0d32d93757..63b0363327c3ef 100644
--- 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
index 28cd945e3cf3b9..3b0c0591822adc 100644
--- 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'
   }]);
 });