bio: various fixes and improvements

* simplify wolfSSL_BIO_set_conn_hostname, fixing OOB read
* restructure wolfSSL_BIO_ctrl_pending, fixing inverted check and
* ctrlCB checking
* return WOLFSSL_FAILURE in wolfSSL_BIO_up_ref when refInc fails,
  updated test to reflect this
* check arguments for NULL in wolfSSL_BIO_ADDR_size
* replace non-portable type long usigned int with size_t
* wolfSSL_BIO_MEMORY_write: return WOLFSSL_BIO_ERROR on failure instead
  of WOLFSSL_FAILURE, return 0 when len is 0
* wolfSSL_BIO_get_fp: fix type mismatch comparing XFILE* pointer against
  XBADFILE
* wolfSSL_BIO_ctrl: add NULL check on bio before switch
* wolfSSL_BIO_pop: clear bio prev and next pointers after unlinking
* wolfSSL_BIO_gets: place null terminator after actual bytes read from
  BIO_BIO nread

Author: rizlik

src/bio.c

@@ -164,7 +164,7 @@ static int wolfSSL_BIO_MEMORY_read(WOLFSSL_BIO* bio, void* buf, int len)
             /* Resize the memory so we are not taking up more than necessary.
              * memmove reverts internally to memcpy if areas don't overlap */
             XMEMMOVE(bio->mem_buf->data, bio->mem_buf->data + bio->rdIdx,
-                    (long unsigned int)bio->wrSz - (size_t)bio->rdIdx);
+                    (size_t)bio->wrSz - (size_t)bio->rdIdx);
             bio->wrSz -= bio->rdIdx;
             bio->rdIdx = 0;
             /* Resize down to WOLFSSL_BIO_RESIZE_THRESHOLD for fewer
@@ -602,24 +602,24 @@ static int wolfSSL_BIO_MEMORY_write(WOLFSSL_BIO* bio, const void* data,
 
     if (bio == NULL || bio->mem_buf == NULL || data == NULL) {
         WOLFSSL_MSG("one of input parameters is null");
-        return WOLFSSL_FAILURE;
+        return WOLFSSL_BIO_ERROR;
     }
     if (bio->flags & WOLFSSL_BIO_FLAG_MEM_RDONLY) {
-        return WOLFSSL_FAILURE;
+        return WOLFSSL_BIO_ERROR;
     }
 
     if (len == 0)
-        return WOLFSSL_SUCCESS; /* Return early to make logic simpler */
+        return 0; /* Nothing to write */
 
     if (wolfSSL_BUF_MEM_grow_ex(bio->mem_buf, ((size_t)bio->wrSz) +
                                                     ((size_t)len), 0) == 0) {
         WOLFSSL_MSG("Error growing memory area");
-        return WOLFSSL_FAILURE;
+        return WOLFSSL_BIO_ERROR;
     }
 
     if (bio->mem_buf->data == NULL) {
         WOLFSSL_MSG("Buffer data is NULL");
-        return WOLFSSL_FAILURE;
+        return WOLFSSL_BIO_ERROR;
     }
 
     XMEMCPY(bio->mem_buf->data + bio->wrSz, data, (size_t)len);
@@ -868,7 +868,11 @@ long wolfSSL_BIO_ctrl(WOLFSSL_BIO *bio, int cmd, long larg, void *parg)
 
     WOLFSSL_ENTER("wolfSSL_BIO_ctrl");
 
-    if (bio && bio->method && bio->method->ctrlCb) {
+    if (bio == NULL) {
+        return WOLFSSL_FAILURE;
+    }
+
+    if (bio->method && bio->method->ctrlCb) {
         return bio->method->ctrlCb(bio, cmd, larg, parg);
     }
 
@@ -952,6 +956,7 @@ int wolfSSL_BIO_up_ref(WOLFSSL_BIO* bio)
     #ifdef WOLFSSL_REFCNT_ERROR_RETURN
         if (ret != 0) {
             WOLFSSL_MSG("Failed to lock BIO mutex");
+            return WOLFSSL_FAILURE;
         }
     #else
         (void)ret;
@@ -984,6 +989,8 @@ void wolfSSL_BIO_ADDR_clear(WOLFSSL_BIO_ADDR *addr) {
 }
 
 socklen_t wolfSSL_BIO_ADDR_size(const WOLFSSL_BIO_ADDR *addr) {
+    if (addr == NULL)
+        return 0;
     switch (addr->sa.sa_family) {
 #ifndef WOLFSSL_NO_BIO_ADDR_IN
     case AF_INET:
@@ -1144,6 +1151,7 @@ int wolfSSL_BIO_gets(WOLFSSL_BIO* bio, char* buf, int sz)
                 ret = wolfSSL_BIO_nread(bio, &c, cSz);
                 if (ret > 0 && ret < sz) {
                     XMEMCPY(buf, c, (size_t)ret);
+                    buf[ret] = '\0';
                 }
                 break;
             }
@@ -1299,51 +1307,42 @@ size_t wolfSSL_BIO_ctrl_pending(WOLFSSL_BIO *bio)
     WOLFSSL_ENTER("wolfSSL_BIO_ctrl_pending");
 #endif
 
-    if (bio == NULL) {
-        return 0;
-    }
-
-    if (bio->method != NULL && bio->method->ctrlCb != NULL) {
-        long ret;
-        WOLFSSL_MSG("Calling custom BIO ctrl pending callback");
-        ret = bio->method->ctrlCb(bio, WOLFSSL_BIO_CTRL_PENDING, 0, NULL);
-        return (ret < 0) ? 0 : (size_t)ret;
-    }
-
-    if (bio->type == WOLFSSL_BIO_MD ||
-            bio->type == WOLFSSL_BIO_BASE64) {
-        /* these are wrappers only, get next bio */
-        while (bio->next != NULL) {
-            bio = bio->next;
-            if (bio->type == WOLFSSL_BIO_MD ||
-                    bio->type == WOLFSSL_BIO_BASE64) {
-                break;
-            }
+    for (; bio != NULL; bio = bio->next) {
+        if (bio->method != NULL && bio->method->ctrlCb != NULL) {
+            long ret;
+            WOLFSSL_MSG("Calling custom BIO ctrl pending callback");
+            ret = bio->method->ctrlCb(bio, WOLFSSL_BIO_CTRL_PENDING, 0, NULL);
+            return (ret < 0) ? 0 : (size_t)ret;
         }
-    }
 
+        switch (bio->type) {
+        case WOLFSSL_BIO_MD:
+        case WOLFSSL_BIO_BASE64:
+            /* wrappers only, skip to next bio in chain */
+            continue;
 #ifndef WOLFCRYPT_ONLY
-    if (bio->type == WOLFSSL_BIO_SSL && bio->ptr.ssl != NULL) {
-        return (size_t)wolfSSL_pending(bio->ptr.ssl);
-    }
+        case WOLFSSL_BIO_SSL:
+            if (bio->ptr.ssl != NULL)
+                return (size_t)wolfSSL_pending(bio->ptr.ssl);
+            return 0;
 #endif
-
-    if (bio->type == WOLFSSL_BIO_MEMORY) {
-        return (size_t)(bio->wrSz - bio->rdIdx);
-    }
-
-    /* type BIO_BIO then check paired buffer */
-    if (bio->type == WOLFSSL_BIO_BIO && bio->pair != NULL) {
-        WOLFSSL_BIO* pair = bio->pair;
-        if (pair->wrIdx > 0 && pair->wrIdx <= pair->rdIdx) {
-            /* in wrap around state where beginning of buffer is being
-             * overwritten */
-            return ((size_t)pair->wrSz) - ((size_t)pair->rdIdx) +
-                                                ((size_t)pair->wrIdx);
-        }
-        else {
-            /* simple case where has not wrapped around */
-            return (size_t)(pair->wrIdx - pair->rdIdx);
+        case WOLFSSL_BIO_MEMORY:
+            return (size_t)(bio->wrSz - bio->rdIdx);
+        case WOLFSSL_BIO_BIO:
+            if (bio->pair != NULL) {
+                WOLFSSL_BIO* pair = bio->pair;
+                if (pair->wrIdx > 0 && pair->wrIdx <= pair->rdIdx) {
+                    /* wrap around state */
+                    return ((size_t)pair->wrSz) - ((size_t)pair->rdIdx) +
+                                                        ((size_t)pair->wrIdx);
+                }
+                else {
+                    return (size_t)(pair->wrIdx - pair->rdIdx);
+                }
+            }
+            return 0;
+        default:
+            return 0;
         }
     }
     return 0;
@@ -1827,7 +1826,7 @@ long wolfSSL_BIO_get_fp(WOLFSSL_BIO *bio, XFILE* fp)
 {
     WOLFSSL_ENTER("wolfSSL_BIO_get_fp");
 
-    if (bio == NULL || fp == XBADFILE) {
+    if (bio == NULL || fp == NULL) {
         return WOLFSSL_FAILURE;
     }
 
@@ -2830,39 +2829,17 @@ int wolfSSL_BIO_flush(WOLFSSL_BIO* bio)
         }
 
         newLen = XSTRLEN(name);
+        if (b->ip != NULL && XSTRLEN(b->ip) != newLen) {
+            XFREE(b->ip, b->heap, DYNAMIC_TYPE_OPENSSL);
+            b->ip = NULL;
+        }
         if (b->ip == NULL) {
-            /* +1 for null char */
             b->ip = (char*)XMALLOC(newLen + 1, b->heap, DYNAMIC_TYPE_OPENSSL);
             if (b->ip == NULL) {
                 WOLFSSL_MSG("Hostname malloc failed.");
                 return WOLFSSL_FAILURE;
             }
         }
-        else {
-            size_t currLen = XSTRLEN(b->ip);
-        #ifdef WOLFSSL_NO_REALLOC
-            char* tmp = NULL;
-        #endif
-
-            if (currLen != newLen) {
-        #ifdef WOLFSSL_NO_REALLOC
-                tmp = b->ip;
-                b->ip = (char*)XMALLOC(newLen+1, b->heap, DYNAMIC_TYPE_OPENSSL);
-                if (b->ip != NULL && tmp != NULL) {
-                    XMEMCPY(b->ip, tmp, newLen);
-                    XFREE(tmp, b->heap, DYNAMIC_TYPE_OPENSSL);
-                    tmp = NULL;
-            }
-        #else
-                b->ip = (char*)XREALLOC(b->ip, newLen + 1, b->heap,
-                    DYNAMIC_TYPE_OPENSSL);
-        #endif
-                if (b->ip == NULL) {
-                    WOLFSSL_MSG("Hostname realloc failed.");
-                    return WOLFSSL_FAILURE;
-                }
-            }
-        }
 
         XMEMCPY(b->ip, name, newLen);
         b->ip[newLen] = '\0';
@@ -3183,6 +3160,8 @@ int wolfSSL_BIO_flush(WOLFSSL_BIO* bio)
  */
 WOLFSSL_BIO* wolfSSL_BIO_pop(WOLFSSL_BIO* bio)
 {
+    WOLFSSL_BIO* next;
+
     if (bio == NULL) {
         WOLFSSL_MSG("Bad argument passed in");
         return NULL;
@@ -3196,7 +3175,11 @@ WOLFSSL_BIO* wolfSSL_BIO_pop(WOLFSSL_BIO* bio)
         bio->next->prev = bio->prev;
     }
 
-    return bio->next;
+    next = bio->next;
+    bio->prev = NULL;
+    bio->next = NULL;
+
+    return next;
 }
 
 

tests/api/test_ossl_bio.c

@@ -1150,7 +1150,7 @@ int test_wolfSSL_BIO_reset(void)
     ExpectIntEQ(BIO_read(bio, buf, 16), 10);
     ExpectIntEQ(XMEMCMP(buf, " your data", 10), 0);
     /* You cannot write to MEM BIO with read-only mode. */
-    ExpectIntEQ(BIO_write(bio, "WriteToReadonly", 15), 0);
+    ExpectIntEQ(BIO_write(bio, "WriteToReadonly", 15), WOLFSSL_BIO_ERROR);
     ExpectIntEQ(BIO_read(bio, buf, 16), -1);
     XMEMSET(buf, 0, 16);
     ExpectIntEQ(BIO_reset(bio), 1);