diff mbox series

[Openvpn-devel] Fix SIGSEGV (NULL deref) receiving push "echo"

Message ID 20210603123019.422644-1-matthias.andree@gmx.de
State Accepted
Headers show
Series [Openvpn-devel] Fix SIGSEGV (NULL deref) receiving push "echo" | expand

Commit Message

Matthias Andree June 3, 2021, 2:30 a.m. UTC 
A server pushing "echo" without arguments can crash the client.
In such a situation, the code in question receives p[1] == NULL
(which was CLEAR(p)'ed above), hands it strncmp, which then
dereferences the null pointer.

Original report and analysis here:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256331

Fixes: Trac #1409
Reported-by: peo@nethead.se (to FreeBSD)
Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 src/openvpn/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.31.1

Comments

Gert Doering June 3, 2021, 4:38 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

This is obviously correct, and in hindsight a very obvious bug - we
seemed to have focused too much on the documentation aspect of the
commit introducing the bug, and not enough on the code change...

Your patch has been applied to the master and release/2.5 branch.

commit 0033811e0215af76f469d78912c95a2f59813454 (master).
commit d6e21bed964109abaf4bf03a951dc2fc9b1d5c1f (release/2.5)
Author: Matthias Andree
Date:   Thu Jun 3 14:30:19 2021 +0200

     Fix SIGSEGV (NULL deref) receiving push echo

     Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20210603123019.422644-1-matthias.andree@gmx.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22486.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair June 3, 2021, 6:51 a.m. UTC | #2 
On Thu, Jun 3, 2021 at 8:32 AM Matthias Andree <matthias.andree@gmx.de> wrote:
>
> A server pushing "echo" without arguments can crash the client.
> In such a situation, the code in question receives p[1] == NULL
> (which was CLEAR(p)'ed above), hands it strncmp, which then
> dereferences the null pointer.
>
> Original report and analysis here:
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256331
>
> Fixes: Trac #1409
> Reported-by: peo@nethead.se (to FreeBSD)
> Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
> ---
>  src/openvpn/options.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 8d417206..a54bc562 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5365,7 +5365,7 @@ add_option(struct options *options,
>          {
>              /* only message-related ECHO are logged, since other ECHOs
>               * can potentially include security-sensitive strings */
> -            if (strncmp(p[1], "msg", 3) == 0)
> +            if (p[1] && strncmp(p[1], "msg", 3) == 0)
>              {
>                  msg(M_INFO, "%s:%s",
>                      pull_mode ? "ECHO-PULL" : "ECHO",

Argh.. good to see already committed. I take all the blame for acking
the original..

This may take the award for the easiest bug to reproduce: just run
"openvpn --echo" no server, no config file required.



Selva
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 8d417206..a54bc562 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5365,7 +5365,7 @@  add_option(struct options *options,
         {
             /* only message-related ECHO are logged, since other ECHOs
              * can potentially include security-sensitive strings */
-            if (strncmp(p[1], "msg", 3) == 0)
+            if (p[1] && strncmp(p[1], "msg", 3) == 0)
             {
                 msg(M_INFO, "%s:%s",
                     pull_mode ? "ECHO-PULL" : "ECHO",