PR: 1748

Fix nasty SSL BIO pop bug. Since this changes the behaviour of SSL BIOs and
will break applications that worked around the bug only included in 1.0.0 and
later.

diff --git a/CHANGES b/CHANGES
index 65ff95a788d0b1e71be4fe798ac38aa82b0c14dc..d74262e1bba975aed2bc14130852d21919995608 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -4,6 +4,16 @@
 
  Changes between 0.9.8k and 1.0  [xx XXX xxxx]
 
+  *) In BIO_pop() and BIO_push() use the ctrl argument (which was NULL) to
+     indicate the initial BIO being pushed or popped. This makes it possible
+     to determine whether the BIO is the one explicitly called or as a result
+     of the ctrl being passed down the chain. Fix BIO_pop() and SSL BIOs so
+     it handles reference counts correctly and doesn't zero out the I/O bio
+     when it is not being explicitly popped. WARNING: applications which
+     included workarounds for the old buggy behaviour will need to be modified
+     or they could free up already freed BIOs.
+     [Steve Henson]
+
   *) Rename uni2asc and asc2uni functions to OPENSSL_uni2asc and
      OPENSSL_asc2uni the original names were too generic and cause name
      clashes on Netware.

diff --git a/crypto/bio/bio_lib.c b/crypto/bio/bio_lib.c
index 3f52ae953c2946b88e6d04c360aa52a4c2b631f7..77f4de9c32413e8b28ad808ffab67873ca2a07f9 100644 (file)
--- a/crypto/bio/bio_lib.c
+++ b/crypto/bio/bio_lib.c
@@ -429,7 +429,7 @@ BIO *BIO_push(BIO *b, BIO *bio)
        if (bio != NULL)
                bio->prev_bio=lb;
        /* called to do internal processing */
-       BIO_ctrl(b,BIO_CTRL_PUSH,0,NULL);
+       BIO_ctrl(b,BIO_CTRL_PUSH,0,lb);
        return(b);
        }
 
@@ -441,7 +441,7 @@ BIO *BIO_pop(BIO *b)
        if (b == NULL) return(NULL);
        ret=b->next_bio;
 
-       BIO_ctrl(b,BIO_CTRL_POP,0,NULL);
+       BIO_ctrl(b,BIO_CTRL_POP,0,b);
 
        if (b->prev_bio != NULL)
                b->prev_bio->next_bio=b->next_bio;

diff --git a/doc/crypto/BIO_f_ssl.pod b/doc/crypto/BIO_f_ssl.pod
index f0b731731f505a5940d534f813bc542964e71d3b..bc5861ab34b31f2edce94ae4516234a81ccc8683 100644 (file)
--- a/doc/crypto/BIO_f_ssl.pod
+++ b/doc/crypto/BIO_f_ssl.pod
@@ -308,6 +308,15 @@ a client and also echoes the request to standard output.
 
  BIO_free_all(sbio);
 
+=head1 BUGS
+
+In OpenSSL versions before 1.0.0 the BIO_pop() call was handled incorrectly,
+the I/O BIO reference count was incorrectly incremented (instead of
+decremented) and dissociated with the SSL BIO even if the SSL BIO was not
+explicitly being popped (e.g. a pop higher up the chain). Applications which
+included workarounds for this bug (e.g. freeing BIOs more than once) should
+be modified to handle this fix or they may free up an already freed BIO.
+
 =head1 SEE ALSO
 
 TBA

diff --git a/ssl/bio_ssl.c b/ssl/bio_ssl.c
index da6dfd2262649de84bf469e9e40908022f68d95c..af319af302a1bc4a56f649e0522d14a6f43f6fdb 100644 (file)
--- a/ssl/bio_ssl.c
+++ b/ssl/bio_ssl.c
@@ -398,17 +398,19 @@ static long ssl_ctrl(BIO *b, int cmd, long num, void *ptr)
                        }
                break;
        case BIO_CTRL_POP:
-               /* ugly bit of a hack */
-               if (ssl->rbio != ssl->wbio) /* we are in trouble :-( */
+               /* Only detach if we are the BIO explicitly being popped */
+               if (b == ptr)
                        {
-                       BIO_free_all(ssl->wbio);
-                       }
-               if (b->next_bio != NULL)
-                       {
-                       CRYPTO_add(&b->next_bio->references,1,CRYPTO_LOCK_BIO);
+                       /* Shouldn't happen in practice because the
+                        * rbio and wbio are the same when pushed.
+                        */
+                       if (ssl->rbio != ssl->wbio)
+                               BIO_free_all(ssl->wbio);
+                       if (b->next_bio != NULL)
+                               CRYPTO_add(&b->next_bio->references,-1,CRYPTO_LOCK_BIO);
+                       ssl->wbio=NULL;
+                       ssl->rbio=NULL;
                        }
-               ssl->wbio=NULL;
-               ssl->rbio=NULL;
                break;
        case BIO_C_DO_STATE_MACHINE:
                BIO_clear_retry_flags(b);