From: Richard Levitte <levitte@openssl.org>
Date: Mon, 27 Jul 2020 20:02:07 +0000 (+0200)
Subject: DESERIALIZER: Make OSSL_DESERIALIZER_from_{bio,fp} use BIO_tell() / BIO_seek()
X-Git-Tag: openssl-3.0.0-alpha6~23
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1dbf4537738d86d8078b74c89e38b6ed0b2b1196;p=thirdparty%2Fopenssl.git

DESERIALIZER: Make OSSL_DESERIALIZER_from_{bio,fp} use BIO_tell() / BIO_seek()

Depending on the BIO used, using BIO_reset() may lead to "interesting"
results.  For example, a BIO_f_buffer() on top of another BIO that
handles BIO_reset() as a BIO_seek(bio, 0), the deserialization process
may find itself with a file that's rewound more than expected.

Therefore, OSSL_DESERIALIZER_from_{bio,fp}'s behaviour is changed to
rely purely on BIO_tell() / BIO_seek(), and since BIO_s_mem() is used
internally, it's changed to handle BIO_tell() and BIO_seek() better.

This does currently mean that OSSL_DESERIALIZER can't be easily used
with streams that don't support BIO_tell() / BIO_seek().

Fixes #12541

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/12544)
---

diff --git a/crypto/bio/bss_mem.c b/crypto/bio/bss_mem.c
index 4043043626b..d9580e6d376 100644
--- a/crypto/bio/bss_mem.c
+++ b/crypto/bio/bss_mem.c
@@ -176,6 +176,7 @@ static int mem_buf_free(BIO *a)
 
 /*
  * Reallocate memory buffer if read pointer differs
+ * NOT FOR RDONLY
  */
 static int mem_buf_sync(BIO *b)
 {
@@ -247,12 +248,18 @@ static long mem_ctrl(BIO *b, int cmd, long num, void *ptr)
     long ret = 1;
     char **pptr;
     BIO_BUF_MEM *bbm = (BIO_BUF_MEM *)b->ptr;
-    BUF_MEM *bm;
+    BUF_MEM *bm, *bo;            /* bio_mem, bio_other */
+    long off, remain;
 
-    if (b->flags & BIO_FLAGS_MEM_RDONLY)
+    if (b->flags & BIO_FLAGS_MEM_RDONLY) {
         bm = bbm->buf;
-    else
+        bo = bbm->readp;
+    } else {
         bm = bbm->readp;
+        bo = bbm->buf;
+    }
+    off = bm->data - bo->data;
+    remain = bm->length;
 
     switch (cmd) {
     case BIO_CTRL_RESET:
@@ -270,6 +277,18 @@ static long mem_ctrl(BIO *b, int cmd, long num, void *ptr)
             }
         }
         break;
+    case BIO_C_FILE_SEEK:
+        if (num < 0 || num > off + remain)
+            return -1;   /* Can't see outside of the current buffer */
+
+        bm->data = bo->data + num;
+        bm->length = bo->length - num;
+        bm->max = bo->max - num;
+        off = num;
+        /* FALLTHRU */
+    case BIO_C_FILE_TELL:
+        ret = off;
+        break;
     case BIO_CTRL_EOF:
         ret = (long)(bm->length == 0);
         break;
diff --git a/crypto/serializer/deserializer_lib.c b/crypto/serializer/deserializer_lib.c
index 912e9f8504d..7229bd36314 100644
--- a/crypto/serializer/deserializer_lib.c
+++ b/crypto/serializer/deserializer_lib.c
@@ -456,13 +456,19 @@ static int deser_process(const OSSL_PARAM params[], void *arg)
             && !OSSL_DESERIALIZER_is_a(deser, new_deser_inst->input_type))
             continue;
 
-        if (loc == 0) {
-            if (BIO_reset(bio) <= 0)
-                goto end;
-        } else {
-            if (BIO_seek(bio, loc) <= 0)
-                goto end;
-        }
+        /*
+         * Checking the return value of BIO_reset() or BIO_seek() is unsafe.
+         * Furthermore, BIO_reset() is unsafe to use if the source BIO happens
+         * to be a BIO_s_mem(), because the earlier BIO_tell() gives us zero
+         * no matter where we are in the underlying buffer we're reading from.
+         *
+         * So, we simply do a BIO_seek(), and use BIO_tell() that we're back
+         * at the same position.  This is a best effort attempt, but BIO_seek()
+         * and BIO_tell() should come as a pair...
+         */
+        (void)BIO_seek(bio, loc);
+        if (BIO_tell(bio) != loc)
+            goto end;
 
         /* Recurse */
         new_data.current_deser_inst_index = i;