Implement BIO_puts and add callback function support to BIO_puts,gets…

…,ctrl (#1721)

### Issues: 
CryptoAlg-2463

### Description of changes: 
Currently, `BIO_puts` delegates to the public `BIO_write` API, which may
result in unexpected behavior should applications rely on them being
distinguishable (e.g., if a callback has certain logic for when write is
called versus when puts is called). This change implements BIO_puts as
its own function should this method be defined in the application,
otherwise, it defaults to calling the "write" function of the type of
BIO instantiated.

This also expands on the work done in commit
5131684
to add support for `callback_ex`s in `BIO_puts`, `BIO_gets`, and
`BIO_ctrl`. If no custom callback_ex function is defined, the default
behavior remains unchanged.

### Call-outs:
N/A

### Testing:
- A custom BIO is defined to test cases where BIO_puts is defined
- Additional test cases were added which use a mock callback created in
previous testing to test the added callback functionality of BIO_gets,
BIO_puts, and BIO_ctrl

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

---------

Co-authored-by: Sean McGrail <[email protected]>

crypto/bio/bio.c

@@ -212,17 +212,26 @@ int BIO_gets(BIO *bio, char *buf, int len) {
     OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD);
     return -2;
   }
+  int ret = 0;
+  if (HAS_CALLBACK(bio)) {
+    ret = (int)bio->callback_ex(bio, BIO_CB_GETS, buf, len, 0, 0L, 1L, NULL);
+    if (ret <= 0) {
+      return ret;
+    }
+  }
   if (!bio->init) {
     OPENSSL_PUT_ERROR(BIO, BIO_R_UNINITIALIZED);
     return -2;
   }
   if (len <= 0) {
     return 0;
   }
-  int ret = bio->method->bgets(bio, buf, len);
+  ret = bio->method->bgets(bio, buf, len);
   if (ret > 0) {
     bio->num_read += ret;
   }
+  ret = call_bio_callback_with_processed(bio, BIO_CB_GETS | BIO_CB_RETURN, buf,
+                                         len, ret);
   return ret;
 }
 
@@ -299,13 +308,42 @@ int BIO_write_all(BIO *bio, const void *data, size_t len) {
 }
 
 int BIO_puts(BIO *bio, const char *in) {
-  size_t len = strlen(in);
-  if (len > INT_MAX) {
-    // |BIO_write| and the return value both assume the string fits in |int|.
-    OPENSSL_PUT_ERROR(BIO, ERR_R_OVERFLOW);
-    return -1;
+  // Check for bwrites here since we use that if bputs is NULL
+  if (bio == NULL || bio->method == NULL || (bio->method->bwrite == NULL &&
+                                            bio->method->bputs == NULL)) {
+    OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD);
+    return -2;
   }
-  return BIO_write(bio, in, (int)len);
+  int ret = 0;
+  if(HAS_CALLBACK(bio)) {
+    ret = (int)bio->callback_ex(bio, BIO_CB_PUTS, in, 0, 0, 0L, 1L, NULL);
+    if (ret <= 0) {
+      return ret;
+    }
+  }
+
+  if (!bio->init) {
+    OPENSSL_PUT_ERROR(BIO, BIO_R_UNINITIALIZED);
+    return -2;
+  }
+  if (bio->method->bputs != NULL) {
+    ret = bio->method->bputs(bio, in);
+  } else {
+    const size_t len = strlen(in);
+    if (len > INT_MAX) {
+      // |BIO_write| and the return value both assume the string fits in |int|.
+      OPENSSL_PUT_ERROR(BIO, ERR_R_OVERFLOW);
+      return -1;
+    }
+    ret = bio->method->bwrite(bio, in, len);
+  }
+  if (ret > 0) {
+    bio->num_write += ret;
+  }
+  ret = call_bio_callback_with_processed(bio, BIO_CB_PUTS | BIO_CB_RETURN,
+                                          in, 0, ret);
+
+  return ret;
 }
 
 int BIO_flush(BIO *bio) {
@@ -321,8 +359,20 @@ long BIO_ctrl(BIO *bio, int cmd, long larg, void *parg) {
     OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD);
     return -2;
   }
+  long ret = 0;
+  if (HAS_CALLBACK(bio)) {
+    ret = bio->callback_ex(bio, BIO_CB_CTRL, parg, 0, cmd, larg, 1L, NULL);
+    if (ret <= 0) {
+      return ret;
+    }
+  }
 
-  return bio->method->ctrl(bio, cmd, larg, parg);
+  ret = bio->method->ctrl(bio, cmd, larg, parg);
+  if (HAS_CALLBACK(bio)) {
+    ret = bio->callback_ex(bio, BIO_CB_CTRL | BIO_CB_RETURN, parg, 0, cmd, larg,
+                         ret, NULL);
+  }
+  return ret;
 }
 
 char *BIO_ptr_ctrl(BIO *b, int cmd, long larg) {
@@ -838,7 +888,7 @@ void BIO_set_shutdown(BIO *bio, int shutdown) { bio->shutdown = shutdown; }
 int BIO_get_shutdown(BIO *bio) { return bio->shutdown; }
 
 int BIO_meth_set_puts(BIO_METHOD *method, int (*puts)(BIO *, const char *)) {
-  // Ignore the parameter. We implement |BIO_puts| using |BIO_write|.
+  method->bputs = puts;
   return 1;
 }
 

crypto/bio/bio_test.cc

@@ -1095,3 +1095,183 @@ TEST(BIOTest, ReadWriteEx) {
   EXPECT_FALSE(BIO_read_ex(bio.get(), nullptr, 0, &read));
   EXPECT_EQ(read, (size_t)0);
 }
+
+TEST(BIOTest, TestPutsAsWrite) {
+  // By default, |BIO_puts| acts as |BIO_write|.
+  bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+  ASSERT_TRUE(bio);
+
+  // Test basic puts and read  
+  uint8_t buf[32];
+  EXPECT_EQ(12, BIO_puts(bio.get(), "hello world\n"));
+  EXPECT_EQ(12, BIO_read(bio.get(), buf, sizeof(buf)));
+}
+
+namespace {
+// Define custom BIO and BIO_METHODS to test BIO_puts without write
+static int customPuts(BIO *b, const char *in) {
+  return 0;
+}
+static int customNew(BIO *b) {
+  b->init=1;
+  return 1;
+}
+static const BIO_METHOD custom_method = {
+  BIO_TYPE_NONE, "CustomBioMethod", NULL /* write */,
+  NULL,          customPuts,        NULL,
+  NULL,          customNew,         NULL,
+  NULL
+};
+
+static const BIO_METHOD *BIO_cust(void) { return &custom_method; }
+
+TEST(BIOTest, TestCustomPuts) {
+  bssl::UniquePtr<BIO> bio(BIO_new(BIO_cust()));
+  ASSERT_TRUE(bio);
+
+  ASSERT_EQ(0, BIO_puts(bio.get(), "hello world"));
+
+  // Test setting new puts method by creating a new BIO
+  bssl::UniquePtr<BIO_METHOD> method(BIO_meth_new(0, nullptr));
+  ASSERT_TRUE(method);
+  ASSERT_TRUE(BIO_meth_set_create(
+    method.get(), [](BIO *b) -> int {
+      BIO_set_init(b, 1);
+      return 1;
+  }));
+  ASSERT_TRUE(BIO_meth_set_puts(
+    method.get(), [](BIO *b, const char *in) -> int {
+      return 100;
+    }
+  ));
+  bssl::UniquePtr<BIO> bio1(BIO_new(method.get()));
+  ASSERT_TRUE(bio1);
+  ASSERT_TRUE(bio1.get()->method->bputs);
+  ASSERT_FALSE(bio1.get()->method->bwrite);
+  // The new BIO_puts should only return 100
+  ASSERT_EQ(100, BIO_puts(bio1.get(), "hello world"));
+}
+
+TEST(BIOTest, TestPutsNullMethod) {
+  // Create new BIO to test when neither puts nor write is set
+  bssl::UniquePtr<BIO_METHOD> method(BIO_meth_new(0, nullptr));
+  ASSERT_TRUE(method);
+  ASSERT_TRUE(BIO_meth_set_create(
+    method.get(), [](BIO *b) -> int {
+      BIO_set_init(b, 1);
+      return 1;
+  }));
+  bssl::UniquePtr<BIO> bio(BIO_new(method.get()));
+  ASSERT_TRUE(bio);
+
+  ASSERT_FALSE(bio.get()->method->bputs);
+  ASSERT_FALSE(bio.get()->method->bwrite);
+  ASSERT_EQ(-2, BIO_puts(bio.get(), "hello world"));
+}
+} //namespace 
+
+TEST(BIOTest, TestPutsCallbacks) {
+  bio_callback_cleanup();
+  BIO* bio = BIO_new(BIO_s_mem());
+  ASSERT_TRUE(bio);
+
+  BIO_set_callback_ex(bio, bio_cb_ex);
+
+  EXPECT_EQ(TEST_DATA_WRITTEN, BIO_puts(bio, "12345"));
+
+  ASSERT_EQ(param_oper_ex[0], BIO_CB_PUTS);
+  ASSERT_EQ(param_oper_ex[1], BIO_CB_PUTS | BIO_CB_RETURN);
+
+  ASSERT_EQ(param_argp_ex[0], param_argp_ex[1]);
+  ASSERT_EQ(param_ret_ex[0], 1);
+  ASSERT_EQ(param_ret_ex[1], TEST_DATA_WRITTEN);
+
+  // len unused in puts callback
+  ASSERT_FALSE(param_len_ex[0]);
+  ASSERT_FALSE(param_len_ex[1]);
+
+  ASSERT_EQ(param_argi_ex[0], 0);
+  ASSERT_EQ(param_argi_ex[1], 0);
+  ASSERT_EQ(param_argl_ex[0], 0);
+  ASSERT_EQ(param_argl_ex[1], 0);
+
+  ASSERT_EQ(param_processed_ex[0], 0u);
+  ASSERT_EQ(param_processed_ex[1], 5u);
+
+  // The mock should be "full" at this point
+  ASSERT_EQ(test_count_ex, CB_TEST_COUNT);
+
+  bio_callback_cleanup();
+  ASSERT_EQ(BIO_free(bio), 1);
+}
+
+TEST(BIOTest, TestGetsCallback) {
+  bio_callback_cleanup();
+
+  BIO* bio = BIO_new(BIO_s_mem());
+  ASSERT_TRUE(bio);
+  // write data to BIO, then set callback 
+  EXPECT_EQ(TEST_DATA_WRITTEN, BIO_write(bio, "12345", TEST_DATA_WRITTEN));
+  char buf[TEST_BUF_LEN];
+  BIO_set_callback_ex(bio, bio_cb_ex);
+
+  ASSERT_EQ(TEST_DATA_WRITTEN, BIO_gets(bio, buf, sizeof(buf)));
+
+  ASSERT_EQ(param_oper_ex[0], BIO_CB_GETS);
+  ASSERT_EQ(param_oper_ex[1], BIO_CB_GETS | BIO_CB_RETURN);
+
+  ASSERT_EQ(param_argp_ex[0], param_argp_ex[1]);
+  ASSERT_EQ(param_ret_ex[0], 1);
+  ASSERT_EQ(param_ret_ex[1], TEST_DATA_WRITTEN);
+
+  ASSERT_EQ(param_len_ex[0], (size_t)TEST_BUF_LEN);
+  ASSERT_EQ(param_len_ex[1], (size_t)TEST_BUF_LEN);
+
+  ASSERT_EQ(param_argi_ex[0], 0);
+  ASSERT_EQ(param_argi_ex[1], 0);
+  ASSERT_EQ(param_argl_ex[0], 0);
+  ASSERT_EQ(param_argl_ex[1], 0);
+
+  ASSERT_EQ(param_processed_ex[0], 0u);
+  ASSERT_EQ(param_processed_ex[1], 5u);
+
+  ASSERT_EQ(test_count_ex, CB_TEST_COUNT);
+
+  bio_callback_cleanup();
+  ASSERT_EQ(BIO_free(bio), 1);
+}
+
+TEST(BIOTest, TestCtrlCallback) { 
+  bio_callback_cleanup();
+
+  BIO* bio = BIO_new(BIO_s_mem());
+  ASSERT_TRUE(bio);
+  BIO_set_callback_ex(bio, bio_cb_ex);
+
+  char buf[TEST_BUF_LEN];
+  // Test BIO_ctrl. This is not normally called directly so we can use one of
+  // the macros such as BIO_reset to test it
+  ASSERT_EQ(BIO_reset(bio), 1);
+
+  ASSERT_EQ(param_oper_ex[0], BIO_CB_CTRL);
+  ASSERT_EQ(param_oper_ex[1], BIO_CB_CTRL | BIO_CB_RETURN);
+
+  // argi in this case in the cmd sent to the ctrl method
+  ASSERT_EQ(param_argi_ex[0], BIO_CTRL_RESET);
+  ASSERT_EQ(param_argi_ex[1], BIO_CTRL_RESET);
+
+  // BIO_ctrl of a memory bio sets ret to 1 when it calls the reset method
+  ASSERT_EQ(param_ret_ex[0], 1);
+  ASSERT_EQ(param_ret_ex[1], 1);
+
+  // processed is unused in ctrl 
+  ASSERT_EQ(param_processed_ex[0], 0u);
+  ASSERT_EQ(param_processed_ex[1], 0u);
+
+  bio_callback_cleanup();
+  // check that BIO_reset was called correctly
+  ASSERT_EQ(BIO_gets(bio, buf, sizeof(buf)), 0);
+
+  bio_callback_cleanup();
+  ASSERT_EQ(BIO_free(bio), 1);
+}

include/openssl/bio.h

@@ -119,9 +119,12 @@ OPENSSL_EXPORT int BIO_read(BIO *bio, void *data, int len);
 OPENSSL_EXPORT int BIO_read_ex(BIO *bio, void *data, size_t data_len,
                                size_t *read_bytes);
 
-// BIO_gets reads a line from |bio| and writes at most |size| bytes into |buf|.
-// It returns the number of bytes read or a negative number on error. This
-// function's output always includes a trailing NUL byte, so it will read at
+// BIO_gets calls the |bio| |callback_ex| if set with |BIO_CB_GETS|, attempts
+// to read a line from |bio| and write at most |size| bytes into |buf|, then 
+// calls |callback_ex| with |BIO_CB_GETS|+|BIO_CB_RETURN|. If |callback_ex| is
+// set BIO_gets returns the value from calling the |callback_ex|, otherwise
+// |BIO_gets| returns the number of bytes read, or a negative number on error. 
+// This function's output always includes a trailing NUL byte, so it will read at
 // most |size - 1| bytes.
 //
 // If the function read a complete line, the output will include the newline
@@ -146,8 +149,13 @@ OPENSSL_EXPORT int BIO_write_ex(BIO *bio, const void *data, size_t data_len,
 // It returns one if all bytes were successfully written and zero on error.
 OPENSSL_EXPORT int BIO_write_all(BIO *bio, const void *data, size_t len);
 
-// BIO_puts writes a NUL terminated string from |buf| to |bio|. It returns the
-// number of bytes written or a negative number on error.
+// BIO_puts calls the |bio| |callback_ex| if set with |BIO_CB_PUTS|, attempts 
+// to write a NUL terminated string from |buf| to |bio|, then calls
+// |callback_ex| with |BIO_CB_PUTS|+|BIO_CB_RETURN|. If |callback_ex| is set
+// BIO_puts returns the value from calling the |callback_ex|, otherwise
+// |BIO_puts| returns the number of bytes written, or a negative number on 
+// error. Unless the application defines a custom bputs method, this will 
+// delegate to using bwrite.
 OPENSSL_EXPORT int BIO_puts(BIO *bio, const char *buf);
 
 // BIO_flush flushes any buffered output. It returns one on success and zero
@@ -160,8 +168,9 @@ OPENSSL_EXPORT int BIO_flush(BIO *bio);
 // These are generic functions for sending control requests to a BIO. In
 // general one should use the wrapper functions like |BIO_get_close|.
 
-// BIO_ctrl sends the control request |cmd| to |bio|. The |cmd| argument should
-// be one of the |BIO_C_*| values.
+// BIO_ctrl call the |bio| |callback_ex| if set with |BIO_CB_CTRL|, sends the 
+// control request |cmd| to |bio|, then calls |callback_ex| with |BIO_CB_CTRL| 
+// + |BIO_CB_RETURN|. The |cmd| argument should be one of the |BIO_C_*| values.
 OPENSSL_EXPORT long BIO_ctrl(BIO *bio, int cmd, long larg, void *parg);
 
 // BIO_ptr_ctrl acts like |BIO_ctrl| but passes the address of a |void*|
@@ -883,8 +892,8 @@ OPENSSL_EXPORT void BIO_set_shutdown(BIO *bio, int shutdown);
 // BIO_get_shutdown returns the method-specific "shutdown" bit.
 OPENSSL_EXPORT int BIO_get_shutdown(BIO *bio);
 
-// BIO_meth_set_puts returns one. |BIO_puts| is implemented with |BIO_write| in
-// BoringSSL.
+// BIO_meth_set_puts sets the implementation of |BIO_puts| for |method| and 
+// returns 1.
 OPENSSL_EXPORT int BIO_meth_set_puts(BIO_METHOD *method,
                                      int (*puts)(BIO *, const char *));
 
@@ -957,7 +966,6 @@ struct bio_method_st {
   const char *name;
   int (*bwrite)(BIO *, const char *, int);
   int (*bread)(BIO *, char *, int);
-  // TODO(fork): remove bputs.
   int (*bputs)(BIO *, const char *);
   int (*bgets)(BIO *, char *, int);
   long (*ctrl)(BIO *, int, long, void *);
@@ -970,10 +978,14 @@ struct bio_st {
   const BIO_METHOD *method;
   CRYPTO_EX_DATA ex_data;
 
-  // If set, |BIO_read|, |BIO_write|, and |BIO_free| execute |callback_ex|.
+  // If set, |BIO_read|, |BIO_write|, |BIO_free|, |BIO_gets|, |BIO_puts|,
+  // and |BIO_ctrl| execute |callback_ex|.
   // Callbacks are only called with for the following events: |BIO_CB_READ|,
   // |BIO_CB_READ|+|BIO_CB_RETURN|, |BIO_CB_WRITE|,
-  // |BIO_CB_WRITE|+|BIO_CB_RETURN|, and |BIO_CB_FREE|.
+  // |BIO_CB_WRITE|+|BIO_CB_RETURN|, |BIO_CB_PUTS|,
+  // |BIO_CB_PUTS|+|BIO_CB_RETURN|, |BIO_CB_GETS|,
+  // |BIO_CB_GETS|+|BIO_CB_RETURN|, |BIO_CB_CTRL|, 
+  // |BIO_CB_CTRL|+|BIO_CB_RETURN|, and |BIO_CB_FREE|.
   BIO_callback_fn_ex callback_ex;
   // Optional callback argument, only intended for applications use.
   char *cb_arg;