Opened 3 years ago

Closed 3 years ago

#1409 closed Bug / Defect (fixed)

SIGSEGV when server config contains: push "echo"

Reported by: mandree Owned by: Gert Döring
Priority: critical Milestone: release 2.5.3
Component: Generic / unclassified Version: OpenVPN 2.5.1 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

a simple server-side

push "echo"

without arguments is sufficient to crash all client versions since 4008ce020526f950cb2055ba7effff8f7ceb13e4 (v2.5.1~9)
and up to and including at least
b7fe49c2f9fe967155960608a029041065c54916 (master branch)

Trivial fix shown below (this is modified code in the ~ line, adding p[1] &&.

NOTE: I don't know if we need to fix anything in the management code, too, I don't use it currently and can't test.

All details in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256331

NOTE: The doc/man-sections/server-options.rst does not list "echo" as pushable! We may want to update documentation on the --push option, too.

      else if (streq(p[0], "echo") || streq(p[0], "parameter"))
      {
          struct buffer string = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
          int j;
          bool good = true;
  
          VERIFY_PERMISSION(OPT_P_ECHO);
  
          for (j = 1; j < MAX_PARMS; ++j)
          {
              if (!p[j])
              {
                  break;
              }
              if (j > 1)
              {
                  good &= buf_printf(&string, " ");
              }
              good &= buf_printf(&string, "%s", p[j]);
          }
          if (good)
          {
              /* only message-related ECHO are logged, since other ECHOs
               * can potentially include security-sensitive strings */
~             if (p[1] && strncmp(p[1], "msg", 3) == 0)
              {
                  msg(M_INFO, "%s:%s",
                      pull_mode ? "ECHO-PULL" : "ECHO",
                      BSTR(&string));
              }
  #ifdef ENABLE_MANAGEMENT
              if (management)
              {
                  management_echo(management, BSTR(&string), pull_mode);
              }
  #endif
          }

Change History (2)

comment:1 Changed 3 years ago by Gert Döring

Milestone: release 2.5.3
Owner: set to Gert Döring
Status: newaccepted

Patch is in master and release/2.5

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

I leave this ticket open for now as reminder to release 2.5.3 soonish.

comment:2 Changed 3 years ago by Gert Döring

Resolution: fixed
Status: acceptedclosed

2.5.3 released today. Thanks again, Matthias.

Note: See TracTickets for help on using tickets.