Null pointer dereferences?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Null pointer dereferences?

Zubin Mevawalla
I was curious if these were real null pointer dereference issues?

CodeAi, an automated repair tool being developed at Qbit logic,
suggested a couple of if-guards as fixes.

The first was in crypto/async/async_wait.c on line 157. `prev` is
assigned to NULL on 144, and it doesn't look like it is assigned to
anything in the while loop. Access to the field `next` from variable
`prev` thus results in a null pointer dereference.

diff --git a/crypto/async/async_wait.c b/crypto/async/async_wait.c
--- a/crypto/async/async_wait.c
+++ b/crypto/async/async_wait.c
@@ -154,7 +154,9 @@ int ASYNC_WAIT_CTX_clear_fd(ASYNC_WAIT_CTX *ctx,
const void *key)
                 if (ctx->fds == curr) {
                     ctx->fds = curr->next;
                 } else {
-                    prev->next = curr->next;
+                    if(prev) {
+                        prev->next = curr->next;
+                    }
                 }

                 /* It is responsibility of the caller to cleanup before calling


The second fix was in ssl/statem/statem_srvr.c on line 1442 having
seen a path through the control flow where access to the field
`pre_proc_exts` from variable `clienthello` results in a null pointer
dereference. On line 1243 if `clienthello` is NULL then control jumps
to line 1440, and `clienthello` is dereferenced on line 1442 without
having being assigned.

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1439,7 +1439,9 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL
*s, PACKET *pkt)
  err:
     ossl_statem_set_error(s);

-    OPENSSL_free(clienthello->pre_proc_exts);
+    if(clienthello) {
+        OPENSSL_free(clienthello->pre_proc_exts);
+    }
     OPENSSL_free(clienthello);

     return MSG_PROCESS_ERROR;

Could I submit these as patches if they look alright?

Thanks so much,

Zubin
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: Null pointer dereferences?

OpenSSL - Dev mailing list
> The first was in crypto/async/async_wait.c on line 157. `prev` is assigned to
> NULL on 144, and it doesn't look like it is assigned to anything in the while
> loop.

Line 177.
 
> -    OPENSSL_free(clienthello->pre_proc_exts);
> +    if(clienthello) {
> +        OPENSSL_free(clienthello->pre_proc_exts);
> +    }

Without the curly braces :)  yes, this seems like a bug.
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: Null pointer dereferences?

Matt Caswell-2
In reply to this post by Zubin Mevawalla


On 09/05/17 01:22, Zubin Mevawalla wrote:

> I was curious if these were real null pointer dereference issues?
>
> CodeAi, an automated repair tool being developed at Qbit logic,
> suggested a couple of if-guards as fixes.
>
> The first was in crypto/async/async_wait.c on line 157. `prev` is
> assigned to NULL on 144, and it doesn't look like it is assigned to
> anything in the while loop. Access to the field `next` from variable
> `prev` thus results in a null pointer dereference.
>
> diff --git a/crypto/async/async_wait.c b/crypto/async/async_wait.c
> --- a/crypto/async/async_wait.c
> +++ b/crypto/async/async_wait.c
> @@ -154,7 +154,9 @@ int ASYNC_WAIT_CTX_clear_fd(ASYNC_WAIT_CTX *ctx,
> const void *key)
>                  if (ctx->fds == curr) {
>                      ctx->fds = curr->next;
>                  } else {
> -                    prev->next = curr->next;
> +                    if(prev) {
> +                        prev->next = curr->next;
> +                    }
>                  }
>
>                  /* It is responsibility of the caller to cleanup before calling
>
>
> The second fix was in ssl/statem/statem_srvr.c on line 1442 having
> seen a path through the control flow where access to the field
> `pre_proc_exts` from variable `clienthello` results in a null pointer
> dereference. On line 1243 if `clienthello` is NULL then control jumps
> to line 1440, and `clienthello` is dereferenced on line 1442 without
> having being assigned.
>
> diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
> --- a/ssl/statem/statem_srvr.c
> +++ b/ssl/statem/statem_srvr.c
> @@ -1439,7 +1439,9 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL
> *s, PACKET *pkt)
>   err:
>      ossl_statem_set_error(s);
>
> -    OPENSSL_free(clienthello->pre_proc_exts);
> +    if(clienthello) {
> +        OPENSSL_free(clienthello->pre_proc_exts);
> +    }
>      OPENSSL_free(clienthello);
>
>      return MSG_PROCESS_ERROR;
>
> Could I submit these as patches if they look alright?

The first one is not a bug. For the second one, please do via our github
site. No curly braces for just a single line, and make the "if"
expression "if (clienthello != NULL)".

Matt
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev