From 20e4ce62badbdfdea8d44d6d612585ca7efaa060 Mon Sep 17 00:00:00 2001
From: Adam Langley <agl@google.com>
Date: Fri, 5 Feb 2016 11:22:49 -0800
Subject: [PATCH] crypto: have fixed NodeBIOs return EOF

Prior to this change, the NodeBIO objects used to wrap fixed data had
`num` equal to -1. This caused them to return -1 and set the retry flags
when they ran out of data. Since the data is fixed, that's incorrect.
Instead they should return zero to signal EOF.

This change adds a new, static function, NodeBIO::NewFixed to create a
BIO that wraps fixed data and which returns zero when exhausted.

The practical impact of this is limited since most (all?) the parsing
functions that these BIOs get passed to consider any return value less
than one to be EOF and ignore the retry flags anyway.

PR-URL: https://github.com/nodejs/node/pull/5105
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
---
 src/node_crypto.cc     | 26 ++++++--------------------
 src/node_crypto_bio.cc | 16 ++++++++++++++++
 src/node_crypto_bio.h  |  4 ++++
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 6fdf95a59dc75d..5fe3632b109fa7 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -416,29 +416,18 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
 // Takes a string or buffer and loads it into a BIO.
 // Caller responsible for BIO_free_all-ing the returned object.
 static BIO* LoadBIO(Environment* env, Local<Value> v) {
-  BIO* bio = NodeBIO::New();
-  if (!bio)
-    return nullptr;
-
   HandleScope scope(env->isolate());
 
-  int r = -1;
-
   if (v->IsString()) {
     const node::Utf8Value s(env->isolate(), v);
-    r = BIO_write(bio, *s, s.length());
-  } else if (Buffer::HasInstance(v)) {
-    char* buffer_data = Buffer::Data(v);
-    size_t buffer_length = Buffer::Length(v);
-    r = BIO_write(bio, buffer_data, buffer_length);
+    return NodeBIO::NewFixed(*s, s.length());
   }
 
-  if (r <= 0) {
-    BIO_free_all(bio);
-    return nullptr;
+  if (Buffer::HasInstance(v)) {
+    return NodeBIO::NewFixed(Buffer::Data(v), Buffer::Length(v));
   }
 
-  return bio;
+  return nullptr;
 }
 
 
@@ -761,15 +750,12 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
     root_cert_store = X509_STORE_new();
 
     for (size_t i = 0; i < ARRAY_SIZE(root_certs); i++) {
-      BIO* bp = NodeBIO::New();
-
-      if (!BIO_write(bp, root_certs[i], strlen(root_certs[i]))) {
-        BIO_free_all(bp);
+      BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
+      if (bp == nullptr) {
         return;
       }
 
       X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
-
       if (x509 == nullptr) {
         BIO_free_all(bp);
         return;
diff --git a/src/node_crypto_bio.cc b/src/node_crypto_bio.cc
index ca0d0ba97ecfef..d155eaf79a6f9c 100644
--- a/src/node_crypto_bio.cc
+++ b/src/node_crypto_bio.cc
@@ -2,6 +2,7 @@
 #include "openssl/bio.h"
 #include "util.h"
 #include "util-inl.h"
+#include <limits.h>
 #include <string.h>
 
 namespace node {
@@ -27,6 +28,21 @@ BIO* NodeBIO::New() {
 }
 
 
+BIO* NodeBIO::NewFixed(const char* data, size_t len) {
+  BIO* bio = New();
+
+  if (bio == nullptr ||
+      len > INT_MAX ||
+      BIO_write(bio, data, len) != static_cast<int>(len) ||
+      BIO_set_mem_eof_return(bio, 0) != 1) {
+    BIO_free(bio);
+    return nullptr;
+  }
+
+  return bio;
+}
+
+
 void NodeBIO::AssignEnvironment(Environment* env) {
   env_ = env;
 }
diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h
index c4f2923675e9e2..eb247b7f51ace7 100644
--- a/src/node_crypto_bio.h
+++ b/src/node_crypto_bio.h
@@ -23,6 +23,10 @@ class NodeBIO {
 
   static BIO* New();
 
+  // NewFixed takes a copy of `len` bytes from `data` and returns a BIO that,
+  // when read from, returns those bytes followed by EOF.
+  static BIO* NewFixed(const char* data, size_t len);
+
   void AssignEnvironment(Environment* env);
 
   // Move read head to next buffer if needed