openssl s_time: plug SSL leak in doConnection

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

openssl s_time: plug SSL leak in doConnection

Scott Cheloha
tb@ spotted this one.

If BIO_new fails we leak scon, so SSL_free it in that case.

ok?

--
Scott Cheloha

Index: usr.bin/openssl/s_time.c
===================================================================
RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.25
diff -u -p -r1.25 s_time.c
--- usr.bin/openssl/s_time.c 11 Aug 2018 16:07:36 -0000 1.25
+++ usr.bin/openssl/s_time.c 13 Aug 2018 15:53:12 -0000
@@ -365,8 +346,10 @@ doConnection(SSL * scon)
  long verify_error;
  int i;
 
- if ((conn = BIO_new(BIO_s_connect())) == NULL)
+ if ((conn = BIO_new(BIO_s_connect())) == NULL) {
+ SSL_free(scon);
  return (NULL);
+ }
 
 /* BIO_set_conn_port(conn,port);*/
  BIO_set_conn_hostname(conn, s_time_config.host);

Reply | Threaded
Open this post in threaded view
|

Re: openssl s_time: plug SSL leak in doConnection

Jeremie Courreges-Anglas-2
On Mon, Aug 13 2018, Scott Cheloha <[hidden email]> wrote:
> tb@ spotted this one.
>
> If BIO_new fails we leak scon, so SSL_free it in that case.
>
> ok?

Alternative proposal: let doConnection() and benchmark() free the
resources they allocated respectively.


Index: s_time.c
===================================================================
RCS file: /d/cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.25
diff -u -p -p -u -r1.25 s_time.c
--- s_time.c 11 Aug 2018 16:07:36 -0000 1.25
+++ s_time.c 13 Aug 2018 16:50:23 -0000
@@ -414,7 +414,7 @@ benchmark(int reuse_session)
 {
  double totalTime = 0.0;
  int nConn = 0;
- SSL *scon = NULL;
+ SSL *scon = NULL, *newcon;
  time_t finishtime;
  int ret = 1;
  char buf[1024 * 8];
@@ -455,8 +455,10 @@ benchmark(int reuse_session)
  for (;;) {
  if (finishtime < time(NULL))
  break;
- if ((scon = doConnection(reuse_session ? scon : NULL)) == NULL)
+ newcon = doConnection(reuse_session ? scon : NULL);
+ if (newcon == NULL)
  goto end;
+ scon = newcon;
 
  if (s_time_config.www_path != NULL) {
  int i, retval = snprintf(buf, sizeof buf,

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: openssl s_time: plug SSL leak in doConnection

Theo Buehler-5
On Mon, Aug 13, 2018 at 07:01:25PM +0200, Jeremie Courreges-Anglas wrote:

> On Mon, Aug 13 2018, Scott Cheloha <[hidden email]> wrote:
> > tb@ spotted this one.
> >
> > If BIO_new fails we leak scon, so SSL_free it in that case.
> >
> > ok?
>
> Alternative proposal: let doConnection() and benchmark() free the
> resources they allocated respectively.
>

I'd be ok with that as a workaround, but it doesn't change the fact that
doConnection() desperately needs a cleanup.

As Joel pointed out off-list, it would probably be better to to move the
call to SSL_new() out of doConnection() and then simplify the latter.

Cheloha is looking into that, and I think we can give this a bit of time.

Reply | Threaded
Open this post in threaded view
|

Re: openssl s_time: plug SSL leak in doConnection

Scott Cheloha
On Mon, Aug 13, 2018 at 07:20:38PM +0200, Theo Buehler wrote:

> On Mon, Aug 13, 2018 at 07:01:25PM +0200, Jeremie Courreges-Anglas wrote:
> > On Mon, Aug 13 2018, Scott Cheloha <[hidden email]> wrote:
> > > tb@ spotted this one.
> > >
> > > If BIO_new fails we leak scon, so SSL_free it in that case.
> > >
> > > ok?
> >
> > Alternative proposal: let doConnection() and benchmark() free the
> > resources they allocated respectively.
> >
>
> I'd be ok with that as a workaround, but it doesn't change the fact that
> doConnection() desperately needs a cleanup.
>
> As Joel pointed out off-list, it would probably be better to to move the
> call to SSL_new() out of doConnection() and then simplify the latter.
>
> Cheloha is looking into that, and I think we can give this a bit of time.

Here is such a diff.

Move SSL_new() out into benchmark() and eliminate all SSL_new/SSL_free
juggling from doConnection.  Because doConnection is no longer allocating,
have it return an integer.  benchmark() is now less compact but it is
clearer that we never leak an SSL object as all the allocations and frees
occur within the same scope.

The only behavior change is that we now always do SSL_set_connect_state
for the connection instead of skipping that for the object's first use.
After reading the manpage I'm pretty sure this is harmless.  Can someone
more familiar with libssl weigh in, though?

The for-loop in doConnection remains peculiar, but that's cosmetic.

Thoughts?

Index: s_time.c
===================================================================
RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.26
diff -u -p -r1.26 s_time.c
--- s_time.c 14 Aug 2018 15:25:04 -0000 1.26
+++ s_time.c 17 Aug 2018 20:00:00 -0000
@@ -90,7 +90,7 @@
 extern int verify_depth;
 
 static void s_time_usage(void);
-static SSL *doConnection(SSL * scon);
+static int doConnection(SSL *);
 static int benchmark(int);
 
 static SSL_CTX *tm_ctx = NULL;
@@ -345,42 +345,31 @@ s_time_main(int argc, char **argv)
 /***********************************************************************
  * doConnection - make a connection
  * Args:
- * scon = earlier ssl connection for session id, or NULL
+ * scon = earlier ssl connection for session id
  * Returns:
- * SSL * = the connection pointer.
+ * 0 on success, nonzero on error
  */
-static SSL *
-doConnection(SSL * scon)
+int
+doConnection(SSL *scon)
 {
  struct pollfd pfd[1];
- SSL *serverCon;
  BIO *conn;
  long verify_error;
  int i;
 
  if ((conn = BIO_new(BIO_s_connect())) == NULL)
- return (NULL);
+ return 1;
 
-/* BIO_set_conn_port(conn,port);*/
  BIO_set_conn_hostname(conn, s_time_config.host);
-
- if (scon == NULL)
- serverCon = SSL_new(tm_ctx);
- else {
- serverCon = scon;
- SSL_set_connect_state(serverCon);
- }
-
- SSL_set_bio(serverCon, conn, conn);
+ SSL_set_connect_state(scon);
+ SSL_set_bio(scon, conn, conn);
 
  /* ok, lets connect */
  for (;;) {
- i = SSL_connect(serverCon);
+ i = SSL_connect(scon);
  if (BIO_sock_should_retry(i)) {
  BIO_printf(bio_err, "DELAY\n");
-
- i = SSL_get_fd(serverCon);
- pfd[0].fd = i;
+ pfd[0].fd = SSL_get_fd(scon);
  pfd[0].events = POLLIN;
  poll(pfd, 1, -1);
  continue;
@@ -389,17 +378,15 @@ doConnection(SSL * scon)
  }
  if (i <= 0) {
  BIO_printf(bio_err, "ERROR\n");
- verify_error = SSL_get_verify_result(serverCon);
+ verify_error = SSL_get_verify_result(scon);
  if (verify_error != X509_V_OK)
  BIO_printf(bio_err, "verify error:%s\n",
     X509_verify_cert_error_string(verify_error));
  else
  ERR_print_errors(bio_err);
- if (scon == NULL)
- SSL_free(serverCon);
- return NULL;
+ return 1;
  }
- return serverCon;
+ return 0;
 }
 
 static int
@@ -415,7 +402,9 @@ benchmark(int reuse_session)
 
  if (reuse_session) {
  /* Get an SSL object so we can reuse the session id */
- if ((scon = doConnection(NULL)) == NULL) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ if ((doConnection(scon)) {
  fprintf(stderr, "Unable to get connection\n");
  goto end;
  }
@@ -448,7 +437,11 @@ benchmark(int reuse_session)
  for (;;) {
  if (finishtime < time(NULL))
  break;
- if ((scon = doConnection(reuse_session ? scon : NULL)) == NULL)
+ if (!reuse_session) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ }
+ if (doConnection(scon))
  goto end;
 
  if (s_time_config.www_path != NULL) {

Reply | Threaded
Open this post in threaded view
|

Re: openssl s_time: plug SSL leak in doConnection

Scott Cheloha
On Fri, Aug 17, 2018 at 04:43:03PM -0500, Scott Cheloha wrote:

> On Mon, Aug 13, 2018 at 07:20:38PM +0200, Theo Buehler wrote:
> > On Mon, Aug 13, 2018 at 07:01:25PM +0200, Jeremie Courreges-Anglas wrote:
> > > On Mon, Aug 13 2018, Scott Cheloha <[hidden email]> wrote:
> > > > tb@ spotted this one.
> > > >
> > > > If BIO_new fails we leak scon, so SSL_free it in that case.
> > > >
> > > > ok?
> > >
> > > Alternative proposal: let doConnection() and benchmark() free the
> > > resources they allocated respectively.
> > >
> >
> > I'd be ok with that as a workaround, but it doesn't change the fact that
> > doConnection() desperately needs a cleanup.
> >
> > As Joel pointed out off-list, it would probably be better to to move the
> > call to SSL_new() out of doConnection() and then simplify the latter.
> >
> > Cheloha is looking into that, and I think we can give this a bit of time.
>
> Here is such a diff.
>
> Move SSL_new() out into benchmark() and eliminate all SSL_new/SSL_free
> juggling from doConnection.  Because doConnection is no longer allocating,
> have it return an integer.  benchmark() is now less compact but it is
> clearer that we never leak an SSL object as all the allocations and frees
> occur within the same scope.
>
> The only behavior change is that we now always do SSL_set_connect_state
> for the connection instead of skipping that for the object's first use.
> After reading the manpage I'm pretty sure this is harmless.  Can someone
> more familiar with libssl weigh in, though?
>
> The for-loop in doConnection remains peculiar, but that's cosmetic.

Here is a cleaner version of the prior diff with input from tb.

Thoughts?  ok?

Index: s_time.c
===================================================================
RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
retrieving revision 1.26
diff -u -p -r1.26 s_time.c
--- s_time.c 14 Aug 2018 15:25:04 -0000 1.26
+++ s_time.c 18 Aug 2018 00:51:14 -0000
@@ -90,7 +90,7 @@
 extern int verify_depth;
 
 static void s_time_usage(void);
-static SSL *doConnection(SSL * scon);
+static int doConnection(SSL *);
 static int benchmark(int);
 
 static SSL_CTX *tm_ctx = NULL;
@@ -345,42 +345,28 @@ s_time_main(int argc, char **argv)
 /***********************************************************************
  * doConnection - make a connection
  * Args:
- * scon = earlier ssl connection for session id, or NULL
+ * scon = earlier ssl connection for session id
  * Returns:
- * SSL * = the connection pointer.
+ * 1 on success, 0 on error
  */
-static SSL *
-doConnection(SSL * scon)
+static int
+doConnection(SSL *scon)
 {
  struct pollfd pfd[1];
- SSL *serverCon;
  BIO *conn;
  long verify_error;
  int i;
 
  if ((conn = BIO_new(BIO_s_connect())) == NULL)
- return (NULL);
-
-/* BIO_set_conn_port(conn,port);*/
+ return 0;
  BIO_set_conn_hostname(conn, s_time_config.host);
-
- if (scon == NULL)
- serverCon = SSL_new(tm_ctx);
- else {
- serverCon = scon;
- SSL_set_connect_state(serverCon);
- }
-
- SSL_set_bio(serverCon, conn, conn);
-
- /* ok, lets connect */
+ SSL_set_connect_state(scon);
+ SSL_set_bio(scon, conn, conn);
  for (;;) {
- i = SSL_connect(serverCon);
+ i = SSL_connect(scon);
  if (BIO_sock_should_retry(i)) {
  BIO_printf(bio_err, "DELAY\n");
-
- i = SSL_get_fd(serverCon);
- pfd[0].fd = i;
+ pfd[0].fd = SSL_get_fd(scon);
  pfd[0].events = POLLIN;
  poll(pfd, 1, -1);
  continue;
@@ -389,17 +375,15 @@ doConnection(SSL * scon)
  }
  if (i <= 0) {
  BIO_printf(bio_err, "ERROR\n");
- verify_error = SSL_get_verify_result(serverCon);
+ verify_error = SSL_get_verify_result(scon);
  if (verify_error != X509_V_OK)
  BIO_printf(bio_err, "verify error:%s\n",
     X509_verify_cert_error_string(verify_error));
  else
  ERR_print_errors(bio_err);
- if (scon == NULL)
- SSL_free(serverCon);
- return NULL;
+ return 0;
  }
- return serverCon;
+ return 1;
 }
 
 static int
@@ -415,7 +399,9 @@ benchmark(int reuse_session)
 
  if (reuse_session) {
  /* Get an SSL object so we can reuse the session id */
- if ((scon = doConnection(NULL)) == NULL) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ if (!doConnection(scon)) {
  fprintf(stderr, "Unable to get connection\n");
  goto end;
  }
@@ -448,7 +434,11 @@ benchmark(int reuse_session)
  for (;;) {
  if (finishtime < time(NULL))
  break;
- if ((scon = doConnection(reuse_session ? scon : NULL)) == NULL)
+ if (!reuse_session) {
+ if ((scon = SSL_new(tm_ctx)) == NULL)
+ goto end;
+ }
+ if (!doConnection(scon))
  goto end;
 
  if (s_time_config.www_path != NULL) {

Reply | Threaded
Open this post in threaded view
|

Re: openssl s_time: plug SSL leak in doConnection

Theo Buehler-5
On Fri, Aug 17, 2018 at 07:51:26PM -0500, Scott Cheloha wrote:

> On Fri, Aug 17, 2018 at 04:43:03PM -0500, Scott Cheloha wrote:
> > On Mon, Aug 13, 2018 at 07:20:38PM +0200, Theo Buehler wrote:
> > > On Mon, Aug 13, 2018 at 07:01:25PM +0200, Jeremie Courreges-Anglas wrote:
> > > > On Mon, Aug 13 2018, Scott Cheloha <[hidden email]> wrote:
> > > > > tb@ spotted this one.
> > > > >
> > > > > If BIO_new fails we leak scon, so SSL_free it in that case.
> > > > >
> > > > > ok?
> > > >
> > > > Alternative proposal: let doConnection() and benchmark() free the
> > > > resources they allocated respectively.
> > > >
> > >
> > > I'd be ok with that as a workaround, but it doesn't change the fact that
> > > doConnection() desperately needs a cleanup.
> > >
> > > As Joel pointed out off-list, it would probably be better to to move the
> > > call to SSL_new() out of doConnection() and then simplify the latter.
> > >
> > > Cheloha is looking into that, and I think we can give this a bit of time.
> >
> > Here is such a diff.
> >
> > Move SSL_new() out into benchmark() and eliminate all SSL_new/SSL_free
> > juggling from doConnection.  Because doConnection is no longer allocating,
> > have it return an integer.  benchmark() is now less compact but it is
> > clearer that we never leak an SSL object as all the allocations and frees
> > occur within the same scope.
> >
> > The only behavior change is that we now always do SSL_set_connect_state
> > for the connection instead of skipping that for the object's first use.
> > After reading the manpage I'm pretty sure this is harmless.  Can someone
> > more familiar with libssl weigh in, though?
> >
> > The for-loop in doConnection remains peculiar, but that's cosmetic.
>
> Here is a cleaner version of the prior diff with input from tb.
>
> Thoughts?  ok?

Looks good.

ok