Fix bio callback backward compatibility

Don't pass a pointer to uninitialized processed value
for BIO_CB_READ and BIO_CB_WRITE

Check the correct cmd code in BIO_callback_ctrl

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5516)

diff --git a/crypto/bio/bio_lib.c b/crypto/bio/bio_lib.c
index 557d609cd101869aa99313be5c11cb7f87656c73..95eef7d4bf5b85da2a2ba46f0887fd2577bea64d 100644 (file)
--- a/crypto/bio/bio_lib.c
+++ b/crypto/bio/bio_lib.c
@@ -34,9 +34,8 @@ static long bio_call_callback(BIO *b, int oper, const char *argp, size_t len,
     long ret;
     int bareoper;
 
-    if (b->callback_ex != NULL) {
+    if (b->callback_ex != NULL)
         return b->callback_ex(b, oper, argp, len, argi, argl, inret, processed);
-    }
 
     /* Strip off any BIO_CB_RETURN flag */
     bareoper = oper & ~BIO_CB_RETURN;
@@ -51,17 +50,17 @@ static long bio_call_callback(BIO *b, int oper, const char *argp, size_t len,
             return -1;
 
         argi = (int)len;
+    }
 
-        if (inret && (oper & BIO_CB_RETURN)) {
-            if (*processed > INT_MAX)
-                return -1;
-            inret = *processed;
-        }
+    if (inret && (oper & BIO_CB_RETURN) && bareoper != BIO_CB_CTRL) {
+        if (*processed > INT_MAX)
+            return -1;
+        inret = *processed;
     }
 
     ret = b->callback(b, oper, argp, argi, argl, inret);
 
-    if (ret >= 0 && (HAS_LEN_OPER(bareoper) || bareoper == BIO_CB_PUTS)) {
+    if (ret >= 0 && (oper & BIO_CB_RETURN) && bareoper != BIO_CB_CTRL) {
         *processed = (size_t)ret;
         ret = 1;
     }
@@ -260,7 +259,7 @@ static int bio_read_intern(BIO *b, void *data, size_t dlen, size_t *readbytes)
 
     if ((b->callback != NULL || b->callback_ex != NULL) &&
         ((ret = (int)bio_call_callback(b, BIO_CB_READ, data, dlen, 0, 0L, 1L,
-                                       readbytes)) <= 0))
+                                       NULL)) <= 0))
         return ret;
 
     if (!b->init) {
@@ -333,7 +332,7 @@ static int bio_write_intern(BIO *b, const void *data, size_t dlen,
 
     if ((b->callback != NULL || b->callback_ex != NULL) &&
         ((ret = (int)bio_call_callback(b, BIO_CB_WRITE, data, dlen, 0, 0L, 1L,
-                                       written)) <= 0))
+                                       NULL)) <= 0))
         return ret;
 
     if (!b->init) {
@@ -542,7 +541,8 @@ long BIO_callback_ctrl(BIO *b, int cmd, BIO_info_cb *fp)
     if (b == NULL)
         return 0;
 
-    if ((b->method == NULL) || (b->method->callback_ctrl == NULL)) {
+    if ((b->method == NULL) || (b->method->callback_ctrl == NULL)
+            || (cmd != BIO_CTRL_SET_CALLBACK)) {
         BIOerr(BIO_F_BIO_CALLBACK_CTRL, BIO_R_UNSUPPORTED_METHOD);
         return -2;
     }

diff --git a/doc/man3/BIO_set_callback.pod b/doc/man3/BIO_set_callback.pod
index 71a041ebf545c44232bb452e70eba406d2e105e5..0a9b6edb656a72d42c70eb506e3f245847fc9277 100644 (file)
--- a/doc/man3/BIO_set_callback.pod
+++ b/doc/man3/BIO_set_callback.pod
@@ -114,7 +114,7 @@ is called before the free operation.
 
 =item B<BIO_read_ex(b, data, dlen, readbytes)>
 
- callback_ex(b, BIO_CB_READ, data, dlen, 0, 0L, 1L, readbytes)
+ callback_ex(b, BIO_CB_READ, data, dlen, 0, 0L, 1L, NULL)
 
 or
 
@@ -123,7 +123,7 @@ or
 is called before the read and
 
  callback_ex(b, BIO_CB_READ | BIO_CB_RETURN, data, dlen, 0, 0L, retvalue,
-             readbytes)
+             &readbytes)
 
 or
 
@@ -133,7 +133,7 @@ after.
 
 =item B<BIO_write(b, data, dlen, written)>
 
- callback_ex(b, BIO_CB_WRITE, data, dlen, 0, 0L, 1L, written)
+ callback_ex(b, BIO_CB_WRITE, data, dlen, 0, 0L, 1L, NULL)
 
 or
 
@@ -142,7 +142,7 @@ or
 is called before the write and
 
  callback_ex(b, BIO_CB_WRITE | BIO_CB_RETURN, data, dlen, 0, 0L, retvalue,
-             written)
+             &written)
 
 or
 
@@ -161,7 +161,7 @@ or
 is called before the operation and
 
  callback_ex(b, BIO_CB_GETS | BIO_CB_RETURN, buf, size, 0, 0L, retvalue,
-             readbytes)
+             &readbytes)
 
 or
 
@@ -179,11 +179,11 @@ or
 
 is called before the operation and
 
- callback_ex(b, BIO_CB_PUTS | BIO_CB_RETURN, buf, 0, 0, 0L, retvalue, written)
+ callback_ex(b, BIO_CB_PUTS | BIO_CB_RETURN, buf, 0, 0, 0L, retvalue, &written)
 
 or
 
- callback(b, BIO_CB_WRITE|BIO_CB_RETURN, buf, 0, 0L, retvalue)
+ callback(b, BIO_CB_PUTS|BIO_CB_RETURN, buf, 0, 0L, retvalue)
 
 after.
 
@@ -205,6 +205,10 @@ or
 
 after.
 
+Note: B<cmd> == B<BIO_CTRL_SET_CALLBACK> is special, because B<parg> is not the
+argument of type B<BIO_info_cb> itself.  In this case B<parg> is a pointer to
+the actual call parameter, see B<BIO_callback_ctrl>.
+
 =back
 
 =head1 EXAMPLE

diff --git a/test/bio_callback_test.c b/test/bio_callback_test.c
new file mode 100644 (file)
index 0000000..f1712b3
--- /dev/null
+++ b/test/bio_callback_test.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright 2018 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the OpenSSL license (the "License").  You may not use
+ * this file except in compliance with the License.  You can obtain a copy
+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ */
+#include <stdio.h>
+#include <string.h>
+#include <openssl/bio.h>
+
+#include "testutil.h"
+
+#define MAXCOUNT 5
+static int         my_param_count;
+static BIO        *my_param_b[MAXCOUNT];
+static int         my_param_oper[MAXCOUNT];
+static const char *my_param_argp[MAXCOUNT];
+static int         my_param_argi[MAXCOUNT];
+static long        my_param_argl[MAXCOUNT];
+static long        my_param_ret[MAXCOUNT];
+
+static long my_bio_callback(BIO *b, int oper, const char *argp, int argi,
+                            long argl, long ret)
+{
+    if (my_param_count >= MAXCOUNT)
+        return -1;
+    my_param_b[my_param_count]    = b;
+    my_param_oper[my_param_count] = oper;
+    my_param_argp[my_param_count] = argp;
+    my_param_argi[my_param_count] = argi;
+    my_param_argl[my_param_count] = argl;
+    my_param_ret[my_param_count]  = ret;
+    my_param_count++;
+    return ret;
+}
+
+static int test_bio_callback(void)
+{
+    int ok = 0;
+    BIO *bio;
+    int i;
+    char *test1 = "test";
+    char *test2 = "hello";
+
+    my_param_count = 0;
+
+    bio = BIO_new(BIO_s_mem());
+    if (bio == NULL)
+        goto err;
+
+    BIO_set_callback(bio, my_bio_callback);
+    i = BIO_write(bio, test1, 4);
+    if (!TEST_int_eq(i, 4)
+            || !TEST_int_eq(my_param_count, 2)
+            || !TEST_ptr_eq(my_param_b[0], bio)
+            || !TEST_int_eq(my_param_oper[0], BIO_CB_WRITE)
+            || !TEST_ptr_eq(my_param_argp[0], test1)
+            || !TEST_int_eq(my_param_argi[0], 4)
+            || !TEST_long_eq(my_param_argl[0], 0L)
+            || !TEST_long_eq(my_param_ret[0], 1L)
+            || !TEST_ptr_eq(my_param_b[1], bio)
+            || !TEST_int_eq(my_param_oper[1], BIO_CB_WRITE | BIO_CB_RETURN)
+            || !TEST_ptr_eq(my_param_argp[1], test1)
+            || !TEST_int_eq(my_param_argi[1], 4)
+            || !TEST_long_eq(my_param_argl[1], 0L)
+            || !TEST_long_eq(my_param_ret[1], 4L))
+        goto err;
+
+    i = BIO_puts(bio, test2);
+    if (!TEST_int_eq(i, 5)
+            || !TEST_int_eq(my_param_count, 4)
+            || !TEST_ptr_eq(my_param_b[2], bio)
+            || !TEST_int_eq(my_param_oper[2], BIO_CB_PUTS)
+            || !TEST_ptr_eq(my_param_argp[2], test2)
+            || !TEST_int_eq(my_param_argi[2], 0)
+            || !TEST_long_eq(my_param_argl[2], 0L)
+            || !TEST_long_eq(my_param_ret[2], 1L)
+            || !TEST_ptr_eq(my_param_b[3], bio)
+            || !TEST_int_eq(my_param_oper[3], BIO_CB_PUTS | BIO_CB_RETURN)
+            || !TEST_ptr_eq(my_param_argp[3], test2)
+            || !TEST_int_eq(my_param_argi[3], 0)
+            || !TEST_long_eq(my_param_argl[3], 0L)
+            || !TEST_long_eq(my_param_ret[3], 5L))
+        goto err;
+
+    i = BIO_free(bio);
+
+    if (!TEST_int_eq(i, 1)
+            || !TEST_int_eq(my_param_count, 5)
+            || !TEST_ptr_eq(my_param_b[4], bio)
+            || !TEST_int_eq(my_param_oper[4], BIO_CB_FREE)
+            || !TEST_ptr_eq(my_param_argp[4], NULL)
+            || !TEST_int_eq(my_param_argi[4], 0)
+            || !TEST_long_eq(my_param_argl[4], 0L)
+            || !TEST_long_eq(my_param_ret[4], 1L))
+        goto finish;
+
+    ok = 1;
+    goto finish;
+
+err:
+    BIO_free(bio);
+
+finish:
+    /* This helps finding memory leaks with ASAN */
+    memset(my_param_b, 0, sizeof(my_param_b));
+    memset(my_param_argp, 0, sizeof(my_param_argp));
+    return ok;
+}
+
+int setup_tests(void)
+{
+    ADD_TEST(test_bio_callback);
+    return 1;
+}

diff --git a/test/build.info b/test/build.info
index 9fcaa7d8c82115af7a33c4a1555a8b655844d854..a4e6e78b608da2abe240fe139174b59671751939 100644 (file)
--- a/test/build.info
+++ b/test/build.info
@@ -40,6 +40,7 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=MAIN
           packettest asynctest secmemtest srptest memleaktest stack_test \
           dtlsv1listentest ct_test threadstest afalgtest d2i_test \
           ssl_test_ctx_test ssl_test x509aux cipherlist_test asynciotest \
+          bio_callback_test \
           bioprinttest sslapitest dtlstest sslcorrupttest bio_enc_test \
           pkey_meth_test pkey_meth_kdf_test uitest cipherbytes_test \
           asn1_encode_test asn1_string_table_test \
@@ -280,6 +281,10 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=MAIN
   INCLUDE[asynciotest]=../include
   DEPEND[asynciotest]=../libcrypto ../libssl libtestutil.a
 
+  SOURCE[bio_callback_test]=bio_callback_test.c
+  INCLUDE[bio_callback_test]=../include
+  DEPEND[bio_callback_test]=../libcrypto libtestutil.a
+
   SOURCE[bioprinttest]=bioprinttest.c
   INCLUDE[bioprinttest]=../include
   DEPEND[bioprinttest]=../libcrypto libtestutil.a

diff --git a/test/recipes/04-test_bio_callback.t b/test/recipes/04-test_bio_callback.t
new file mode 100644 (file)
index 0000000..1422cb6
--- /dev/null
+++ b/test/recipes/04-test_bio_callback.t
@@ -0,0 +1,12 @@
+#! /usr/bin/env perl
+# Copyright 2018 The OpenSSL Project Authors. All Rights Reserved.
+#
+# Licensed under the OpenSSL license (the "License").  You may not use
+# this file except in compliance with the License.  You can obtain a copy
+# in the file LICENSE in the source distribution or at
+# https://www.openssl.org/source/license.html
+
+
+use OpenSSL::Test::Simple;
+
+simple_test("test_bio_callback", "bio_callback_test");