Skip to content

Commit a22a2d8

Browse files
maxtacoindutny
authored andcommitted
tls: stop NodeBIO::Gets from reading off end of buffer
NodeBIO::Gets was reading off the end of a buffer if it didn't find a "\n" before the EOF. This behavior was causing X509 certificates passed to `https.Agent` via the "ca" option to be silently discarded. It also was causing improper parsing of certs and keys passed to https.Agent, but those problems were worked around in cdde9a3. Backed out workaround in `lib/crypto.js` from ccde9a3, which now isn't needed. But keep the test introduced in that commit, which tests properly for this bug. This bug was first introduced in a58f93f Gist containing test code, bisection log, and notes: https://gist.github.com/maxtaco/9211605
1 parent b5f9779 commit a22a2d8

File tree

2 files changed

+5
-18
lines changed

2 files changed

+5
-18
lines changed

lib/crypto.js

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,6 @@ function Credentials(secureProtocol, flags, context) {
8080

8181
exports.Credentials = Credentials;
8282

83-
function addNewline(buf) {
84-
var last = buf[buf.length - 1];
85-
var isBuf = Buffer.isBuffer(buf);
86-
87-
if (!isBuf && !util.isString(buf))
88-
throw new Error('Certificate should be of type Buffer or string');
89-
90-
if (isBuf ? last !== 10 : last !== '\n')
91-
return buf.toString().trim() + '\n';
92-
else
93-
return buf;
94-
}
9583

9684
exports.createCredentials = function(options, context) {
9785
if (!options) options = {};
@@ -103,15 +91,14 @@ exports.createCredentials = function(options, context) {
10391
if (context) return c;
10492

10593
if (options.key) {
106-
var key = addNewline(options.key);
10794
if (options.passphrase) {
108-
c.context.setKey(key, options.passphrase);
95+
c.context.setKey(options.key, options.passphrase);
10996
} else {
110-
c.context.setKey(key);
97+
c.context.setKey(options.key);
11198
}
11299
}
113100

114-
if (options.cert) c.context.setCert(addNewline(options.cert));
101+
if (options.cert) c.context.setCert(options.cert);
115102

116103
if (options.ciphers) c.context.setCiphers(options.ciphers);
117104

src/node_crypto_bio.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,8 @@ int NodeBIO::Gets(BIO* bio, char* out, int size) {
145145

146146
int i = nbio->IndexOf('\n', size);
147147

148-
// Include '\n'
149-
if (i < size)
148+
// Include '\n', if it's there. If not, don't read off the end.
149+
if (i < size && i >= 0 && static_cast<size_t>(i) < nbio->Length())
150150
i++;
151151

152152
// Shift `i` a bit to NULL-terminate string later

0 commit comments

Comments
 (0)