From: Dr. Stephen Henson <steve@openssl.org>
Date: Thu, 25 Jun 2009 11:26:45 +0000 (+0000)
Subject: PR: 1748
X-Git-Tag: OpenSSL_1_0_0-beta3~41
X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=0cb76e79df31e8c6ef9dbbfb92330f90243766be;p=oweals%2Fopenssl.git

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 65ff95a788..d74262e1bb 100644
--- 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 3f52ae953c..77f4de9c32 100644
--- 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 f0b731731f..bc5861ab34 100644
--- 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 da6dfd2262..af319af302 100644
--- 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);