From: schwarze <schwarze@openbsd.org>
Date: Wed, 7 Dec 2022 23:08:47 +0000 (+0000)
Subject: Improve the implementation of BIO_push(3) such that it changes nothing
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=2181dbec822e4d5e40993d0c2893700992d8bc06;p=openbsd

Improve the implementation of BIO_push(3) such that it changes nothing
and reports failure if a call would result in a cycle.
The algorithm used was originally suggested by jsing@.
Feedback and OK tb@.
---

diff --git a/lib/libcrypto/bio/bio_lib.c b/lib/libcrypto/bio/bio_lib.c
index 4c3d7ed5f57..3eb0869ca92 100644
--- a/lib/libcrypto/bio/bio_lib.c
+++ b/lib/libcrypto/bio/bio_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: bio_lib.c,v 1.41 2022/12/06 17:59:21 schwarze Exp $ */
+/* $OpenBSD: bio_lib.c,v 1.42 2022/12/07 23:08:47 schwarze Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -637,6 +637,12 @@ BIO_push(BIO *b, BIO *bio)
 
 	if (b == NULL)
 		return (bio);
+
+	/* If this would create a cycle, change nothing and fail. */
+	for (lb = bio; lb != NULL; lb = lb->next_bio)
+		if (lb == b)
+			return NULL;
+
 	lb = b;
 	while (lb->next_bio != NULL)
 		lb = lb->next_bio;
diff --git a/lib/libcrypto/man/BIO_push.3 b/lib/libcrypto/man/BIO_push.3
index d091c7ccca8..01f426c1ef7 100644
--- a/lib/libcrypto/man/BIO_push.3
+++ b/lib/libcrypto/man/BIO_push.3
@@ -1,4 +1,4 @@
-.\" $OpenBSD: BIO_push.3,v 1.10 2022/12/07 22:30:15 tb Exp $
+.\" $OpenBSD: BIO_push.3,v 1.11 2022/12/07 23:08:47 schwarze Exp $
 .\" full merge up to:
 .\" OpenSSL doc/man3/BIO_push.pod 791bfd91 Nov 19 20:38:27 2021 +0100
 .\" OpenSSL doc/man7/bio.pod 1cb7eff4 Sep 10 13:56:40 2019 +0100
@@ -105,6 +105,7 @@ It is either at the end of its chain
 or there is exactly one following BIO.
 If there is neither a preceding nor a following BIO,
 it can be regarded as a chain with one member.
+Every chain has exactly one beginning and exactly one end.
 .Pp
 .Fn BIO_push
 appends the chain starting at
@@ -140,6 +141,11 @@ For portability, it is best to make sure that
 is at the beginning of its chain before calling
 .Fn BIO_push .
 .Pp
+The LibreSSL implementation of
+.Fn BIO_push
+never creates cycles.
+If a call would result in a cycle, nothing is changed and the call fails.
+.Pp
 .Fn BIO_pop
 removes the BIO
 .Fa b
@@ -208,11 +214,16 @@ have any effect is
 .Fn BIO_push
 returns
 .Fa b
-if it is not
-.Dv NULL
-or
+for success or a different pointer for failure.
+In particular, it fails and returns
 .Fa new_tail
-if it is.
+if
+.Fa b
+is
+.Dv NULL .
+In LibreSSL, it fails and returns
+.Dv NULL
+if appending would create a cycle.
 .Pp
 .Fn BIO_pop
 returns the BIO that followed
@@ -283,6 +294,22 @@ and the new chain will be
 data can be written to
 .Sy md1
 as before.
+.Pp
+Even though LibreSSL handles some of the edge cases gracefully,
+the following idiom is recommended for portable error checking:
+.Bd -literal -offset indent
+if (b == NULL || new_tail == NULL || b == new_tail)
+	/* Report the problem and bail out. */
+if (BIO_push(b, new_tail) != b)
+	/* Report that nothing was changed
+	 * because it would have created a cycle. */
+.Ed
+.Pp
+Even with the portable idiom, old and non-LibreSSL library implementations
+may silently attempt to create cycles instead of rejecting them and returning
+.Dv NULL ,
+which may result in infinite loops, infinite recursion, or segmentation
+faults either right away or later on.
 .Sh SEE ALSO
 .Xr BIO_find_type 3 ,
 .Xr BIO_new 3 ,