CVE-2018-0492 - Githubissues

MarkusTeufelberger commented 6 years ago

Just a short heads-up: https://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-0492

s7726 commented 6 years ago

The package listed at this URL appears to contain a patch that resolves the vulnerability. https://packages.ubuntu.com/source/bionic/beep

Going straight to the debian package that fixed it did not have a patch, just a diff, so I figured this would be a quicker find/implementation.

MarkusTeufelberger commented 6 years ago

The diff seems to be this one:

diff --git a/beep.c b/beep.c
index 7da2e70..4323d31 100644
--- a/beep.c
+++ b/beep.c
@@ -109,6 +109,7 @@ void do_beep(int freq) {
      /* BEEP_TYPE_EVDEV */
      struct input_event e;

+     memset(&e, 0, sizeof(e));
      e.type = EV_SND;
      e.code = SND_TONE;
      e.value = freq;
@@ -124,10 +125,6 @@ void do_beep(int freq) {
 /* If we get interrupted, it would be nice to not leave the speaker beeping in
    perpetuity. */
 void handle_signal(int signum) {
-
-  if(console_device)
-    free(console_device);
-
   switch(signum) {
   case SIGINT:
   case SIGTERM:
@@ -257,7 +254,7 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
       result->verbose = 1;
       break;
     case 'e' : /* also --device */
-      console_device = strdup(optarg);
+      console_device = optarg;
       break;
     case 'h' : /* notice that this is also --help */
     default :
@@ -276,26 +273,6 @@ void play_beep(beep_parms_t parms) {
    "%d delay after) @ %.2f Hz\n",
    parms.reps, parms.length, parms.delay, parms.end_delay, parms.freq);

-  /* try to snag the console */
-  if(console_device)
-    console_fd = open(console_device, O_WRONLY);
-  else
-    if((console_fd = open("/dev/tty0", O_WRONLY)) == -1)
-      console_fd = open("/dev/vc/0", O_WRONLY);
-
-  if(console_fd == -1) {
-    fprintf(stderr, "Could not open %s for writing\n",
-      console_device != NULL ? console_device : "/dev/tty0 or /dev/vc/0");
-    printf("\a");  /* Output the only beep we can, in an effort to fall back on usefulness */
-    perror("open");
-    exit(1);
-  }
-
-  if (ioctl(console_fd, EVIOCGSND(0)) != -1)
-    console_type = BEEP_TYPE_EVDEV;
-  else
-    console_type = BEEP_TYPE_CONSOLE;
-  
   /* Beep */
   for (i = 0; i < parms.reps; i++) {                    /* start beep */
     do_beep(parms.freq);
@@ -305,8 +282,6 @@ void play_beep(beep_parms_t parms) {
     if(parms.end_delay || (i+1 < parms.reps))
        usleep(1000*parms.delay);                        /* wait...    */
   }                                                     /* repeat.    */
-
-  close(console_fd);
 }

@@ -328,6 +303,26 @@ int main(int argc, char **argv) {
   signal(SIGTERM, handle_signal);
   parse_command_line(argc, argv, parms);

+  /* try to snag the console */
+  if(console_device)
+    console_fd = open(console_device, O_WRONLY);
+  else
+    if((console_fd = open("/dev/tty0", O_WRONLY)) == -1)
+      console_fd = open("/dev/vc/0", O_WRONLY);
+
+  if(console_fd == -1) {
+    fprintf(stderr, "Could not open %s for writing\n",
+      console_device != NULL ? console_device : "/dev/tty0 or /dev/vc/0");
+    printf("\a");  /* Output the only beep we can, in an effort to fall back on usefulness */
+    perror("open");
+    exit(1);
+  }
+
+  if (ioctl(console_fd, EVIOCGSND(0)) != -1)
+    console_type = BEEP_TYPE_EVDEV;
+  else
+    console_type = BEEP_TYPE_CONSOLE;
+
   /* this outermost while loop handles the possibility that -n/--new has been
      used, i.e. that we have multiple beeps specified. Each iteration will
      play, then free() one parms instance. */
@@ -365,8 +360,8 @@ int main(int argc, char **argv) {
     parms = next;
   }

-  if(console_device)
-    free(console_device);
+  close(console_fd);
+  console_fd = -1;

   return EXIT_SUCCESS;
 }

...still a bit weird that Debian/Ubuntu fix stuff without opening PRs here. Hopefully they contacted the author in private at least.

s7726 commented 6 years ago

Ubuntu actually had an additional patch as well that covered handling one of the system SIGNALs. Upstreaming is for chumps ;)

Nicals commented 6 years ago

There is no activity on this Github account since June 2016, and no activity on this particular repository since 2013. I don't think this is the place to fix the code...

markand commented 6 years ago

I wonder if people will understand that free accept null pointers as well.

Stargateur commented 6 years ago

I don't understand where does this race condition come from, ioctl ?

The code of this program is ugly and the memset of this patch is ugly too and the choice to not duplicate stdarg come from nowhere.

Oh maybe the free in the signal handler ? In this case, why move "/ try to snag the console /" block code ?

MarkusTeufelberger commented 6 years ago

There is no activity on this Github account since June 2016, and no activity on this particular repository since 2013. I don't think this is the place to fix the code...

Where else would it be? Debian/Ubuntu lists http://johnath.com/beep/ as upstream and that page links to this repository.

At the very least, the patches by Debian/Ubuntu should have been submitted as pull requests.

LangerJan commented 6 years ago

Just my 2 cents and pure speculation:

Oh maybe the free in the signal handler ?

Since it is stated that this is a race condition, it has to do with the signal handler - no other race to see here. Therefore, we have a use-after-free situation with console_device here.

In this case, why move "/ try to snag the console /" block code ?

In order to not having a multiple use-after-free of console_device.

Stargateur commented 6 years ago

The signal handler don't call play_beep(), so there is no use-after-free of console_device. (expect the multiple possible free() of the same pointer witch of course is fatal)

LangerJan commented 6 years ago

Maybe it has to do with calling free() inside the signal handler, which is not considered an async-signal-safe function.

s7726 commented 6 years ago

The report says it's an issue when privilege escalated, so just thinking in the context of normal execution isn't enough.

On Wed, Apr 4, 2018, 7:22 AM LangerJan notifications@github.com wrote:

Maybe it has to do with calling free() inside the signal handler, which is not considered an async-signal-safe function.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/johnath/beep/issues/11#issuecomment-378617301, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDnWSgDR72weEQk4Tbl1aRQ-1699YD0ks5tlNcOgaJpZM4TE6O7 .

zb3 commented 6 years ago

I think the exploit could have a great educational value here.

EDIT: ioctl is not async-signal-safe either, but it's still called in patched version.

rain-1 commented 6 years ago

It's possible that there is no actual priv esc. I will try to reach out to lamb.

LangerJan commented 6 years ago

Here is an interesting article which explained the problem to me: http://lcamtuf.coredump.cx/signals.txt In short:

Using free() inside a signal handler is a bad idea. Its not an async-signal-safe function (see man 7 signal-safety on that). It could be interrupted with another signal, causing destruction of global structures/variables of the heap management.
The current version of beep.c here handles two signals (SIGINT and SIGTERM) in one handler function, which makes it possible to re-enter handle_signal() while it's running, allowing for a double free.

jwilk commented 6 years ago

On Hacker News, terom wrote:

The while loop in main calls play_beep multiple times. Each call to play_beep opens the --device and sets the global console_fd, and then sets the global console_type based on the ioctl(EVIOCGSND) error, before calling do_beep.

This normally prevents the user from writing to arbitrary files with --device, because without the ioctl(EVIOCGSND) succeeding, do_beep with BEEP_TYPE_CONSOLE only does a (harmless?) ioctl(KIOCSOUND), not a write with the struct input_event. However, the signal handler calls do_beep directly using the globals set by play_beep...

So I image that with something along the lines of beep --device=./symlink-to-tty ... --new ..., you can rewrite the symlink to point to an arbitrary file during the first play_beep, and then race the open/ioctl in the second play_beep with the signal handler such that do_beep gets called with console_fd pointing to your arbitrary file, and with console_type still set to BEEP_TYPE_EVDEV, resulting in a write to your arbitrary file.

Exploiting that for privesc would require control over the struct input_event for the write... handle_signal calls do_beep with a fixed freq of 0, so all of the initialized fields are set to fixed values... However, there's an unitialized struct timeval at the beginning of the struct input_event, and it's allocated on the stack...

rain-1 commented 6 years ago

Thank you very much @LangerJan and @jwilk - both those were very informative and provides a very clear and detailed understanding of the problem.

The patch resolves the double free and the uninitialized parts of the struct.

It seems that do_beep can still be re-entered - likely no longer a security vuln but perhaps still room for improvement by following the programming guidelines at the end of lcamtufs document.

SpComb commented 6 years ago

On Hacker News, terom wrote:

Disclaimer: this is just speculation on my part based on the changes in the patch... I may have misunderstood something, I didn't verify any of those assumptions.

optnfast commented 6 years ago

There's an additional minor issue, even with the do_beep race addressed: when setuid root, the program will open any caller-specified filename, as root, for write.

AFAIK the effect is only to disclose whether that file exists plus some information about its file type, via the error message. If the file is in a directory not accessible to the calling user then this represents an information leak.

However if there's such a thing as a file (e.g. a device file or something strange in /proc or /sys) that has side effects when opened, then the effect would be to enable unauthorized initiation of those side effects. I don't know of any examples of this and it seems like a bad design but I can't rule it out.

jwilk commented 6 years ago

Opening tape devices or named pipes can have side effects.

AGWA commented 6 years ago

There's an additional minor issue, even with the do_beep race addressed: when setuid root, the program will open any caller-specified filename, as root, for write.

This also allows an attacker to inhibit the execution of arbitrary programs. For instance, running beep -s -e /bin/sh will cause execve of /bin/sh to fail with ETXTBSY, which is a pretty nasty denial-of-service.

Arignir commented 6 years ago

I've done a blog post about my interpretation, explanations and exploit of this security flaw, in case one of you is interested.

You can find it here. Thanks.

ndim commented 5 years ago

My try at fixing this over at the beep fork I use for packaging for Fedora is as follows:

From 10cd5126f320154dccf344e19248c5589d9c20bb Mon Sep 17 00:00:00 2001
From: Hans Ulrich Niedermann <hun@n-dimensional.de>
Date: Fri, 28 Dec 2018 00:10:33 +0100
Subject: [PATCH] Fix CVE-2018-1000532 and mitigate against related issues

  * Separate initialization including checking for supported APIs
    and the main program which just uses the detected API without
    any further checks. This helps avoid code paths where code
    one API is actually run after initialization for the API.

  * If a device name is given as a command line argument,
    only allow device names starting with "/dev/input/".

  * Verify before open(2) that the device name actually refers
    to a character device special file by using stat(2).

  * After open(2), verify that console_fd actually
    points to a character device special file using fstat(2).

  * Check for API to use on console_fd only once during
    initialization using ioctl(2). The two APIs are

      * The console API which uses ioctl(2) KIOCSOUND on console_fd

      * The evdev API which uses write(2) on console_fd

    Then the actual do_beep() function can just do the proper
    thing without having to decide between APIs or falling back
    or anything.

  * Add "/dev/input/by-path/platform-pcspkr-event-spkr" to
    list of console devices to probe by default, so we have
    both console API type and evdev API type devices in that
    list to help with testing the code for both APIs.
---
 beep.c | 195 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 155 insertions(+), 40 deletions(-)

diff --git a/beep.c b/beep.c
index b838b85..2a7404e 100644
--- a/beep.c
+++ b/beep.c
@@ -16,6 +16,7 @@
  * Bug me, I like it:  http://johnath.com/  or johnath@johnath.com
  */

+#include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
 #include <math.h>
@@ -26,6 +27,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <sys/ioctl.h>
+#include <sys/stat.h>
 #include <sys/types.h>
 #include <linux/kd.h>
 #include <linux/input.h>
@@ -90,36 +92,45 @@ typedef struct beep_parms_t {
   struct beep_parms_t *next;  /* in case -n/--new is used. */
 } beep_parms_t;

-enum { BEEP_TYPE_CONSOLE, BEEP_TYPE_EVDEV };
-
-/* Momma taught me never to use globals, but we need something the signal 
+/* Use an enum and switch statements to have compiler warn us about
+ * unhandled cases. */
+typedef enum {
+         /* When the beep_type has not been set yet, do nothing */
+         BEEP_TYPE_UNSET   = 0,
+         /* Use the console API */
+         BEEP_TYPE_CONSOLE = 1,
+         /* Use the evdev API */
+         BEEP_TYPE_EVDEV   = 2,
+} beep_type_E;
+
+/* Momma taught me never to use globals, but we need something the signal
    handlers can get at.*/
 int console_fd = -1;
-int console_type = BEEP_TYPE_CONSOLE;
+beep_type_E console_type = BEEP_TYPE_UNSET;
 char *console_device = NULL;

 void do_beep(unsigned int freq) {
-  const uintptr_t argp = (freq != 0 ? (CLOCK_TICK_RATE/freq) : freq) & 0xffff;
+  switch (console_type) {
+  case BEEP_TYPE_CONSOLE: if (1) {
+      const uintptr_t argp = ((freq != 0) ? (CLOCK_TICK_RATE/freq) : freq) & 0xffff;
+      (void) ioctl(console_fd, KIOCSOUND, argp);
+    }
+    break;
+  case BEEP_TYPE_EVDEV: if (1) {
+      struct input_event e;
+
+      memset(&e, 0, sizeof(e));
+      e.type = EV_SND;
+      e.code = SND_TONE;
+      e.value = freq;

-  if(console_type == BEEP_TYPE_CONSOLE) {
-    if(ioctl(console_fd, KIOCSOUND, argp) < 0) {
-      putchar('\a');  /* Output the only beep we can, in an effort to fall back on usefulness */
-      perror("ioctl");
+      (void) write(console_fd, &e, sizeof(struct input_event));
     }
-  } else {
-     /* BEEP_TYPE_EVDEV */
-     struct input_event e;
-
-     memset(&e, 0, sizeof(e));
-     e.type = EV_SND;
-     e.code = SND_TONE;
-     e.value = freq;
-
-     if(write(console_fd, &e, sizeof(struct input_event)) < 0) {
-       putchar('\a'); /* See above */
-       perror("write");
-     }
+    break;
+  case BEEP_TYPE_UNSET:
+    /* Do nothing, if this case should ever happen which it should not. */
+    break;
   }
 }

@@ -258,7 +269,19 @@ void parse_command_line(int argc, char **argv, beep_parms_t *result) {
       result->verbose = 1;
       break;
     case 'e' : /* also --device */
-      console_device = optarg;
+      if (strncmp("/dev/input/", optarg, strlen("/dev/input/")) == 0) {
+   /* If device name starts with /dev/input/, we can assume evdev
+    * input device beeping is wished for and the corresponding
+    * device is somewhere in /dev/input/. Otherwise, the default
+    * console beeper will be used with its default name(s). */
+   console_device = optarg;
+      } else {
+   fprintf(stderr, "%s: "
+       "Not opening device '%s'. If you do need this device, please "
+       "report that fact to <https://github.com/ndim/beep/issues>.\n",
+       argv[0], optarg);
+   exit(EXIT_FAILURE);
+      }
       break;
     case 'h' : /* notice that this is also --help */
     default :
@@ -290,11 +313,42 @@ void play_beep(beep_parms_t parms) {
 }

+/* Open only character device special file (with race condition).
+ *
+ * We check whether this is a character device special file before
+ * opening as for some devices, opening has an effect and we can avoid
+ * this effect for those devices here.
+ *
+ * We still need to make sure that the file we have actually opened
+ * actually is a character device special file after we have actually
+ * opened it.
+ */
+int open_chr(const char *const argv0, const char *filename, int flags)
+{
+  struct stat sb;
+  if (-1 == stat(filename, &sb)) {
+    return -1;
+  }
+  if (S_ISCHR(sb.st_mode)) {
+    return open(filename, flags);
+  } else {
+    fprintf(stderr, "%s: "
+       "console file '%s' is not a character device special file\n",
+       argv0, filename);
+    exit(1);
+  }
+}
+

 int main(int argc, char **argv) {
   char sin[4096], *ptr;
-  
+
+  /* Parse command line */
   beep_parms_t *parms = (beep_parms_t *)malloc(sizeof(beep_parms_t));
+  if (NULL == parms) {
+    perror("malloc");
+    exit(1);
+  }
   parms->freq       = 0;
   parms->length     = DEFAULT_LENGTH;
   parms->reps       = DEFAULT_REPS;
@@ -304,29 +358,90 @@ int main(int argc, char **argv) {
   parms->verbose    = 0;
   parms->next       = NULL;

-  signal(SIGINT, handle_signal);
-  signal(SIGTERM, handle_signal);
   parse_command_line(argc, argv, parms);

-  /* try to snag the console */
-  if(console_device)
-    console_fd = open(console_device, O_WRONLY);
-  else
-    if((console_fd = open("/dev/tty0", O_WRONLY)) == -1)
-      console_fd = open("/dev/vc/0", O_WRONLY);
-
-  if(console_fd == -1) {
-    fprintf(stderr, "Could not open %s for writing\n",
-      console_device != NULL ? console_device : "/dev/tty0 or /dev/vc/0");
-    printf("\a");  /* Output the only beep we can, in an effort to fall back on usefulness */
-    perror("open");
+  /* Try opening a console device */
+  if (console_device) {
+    console_fd = open_chr(argv[0], console_device, O_WRONLY);
+  } else {
+    static char *console_device_list[] =
+      { "/dev/tty0",
+   "/dev/vc/0",
+   "/dev/input/by-path/platform-pcspkr-event-spkr",
+      };
+    for (size_t i=0; i<(sizeof(console_device_list)/sizeof(console_device_list[0])); ++i) {
+      if ((console_fd = open_chr(argv[0], console_device_list[i], O_WRONLY)) != -1) {
+   console_device = console_device_list[i];
+   break;
+      }
+    }
+  }
+
+  if (console_fd == -1) {
+    const int saved_errno = errno;
+    fprintf(stderr, "%s: Could not open %s for writing: %s\n",
+       argv[0],
+       ((console_device != NULL) ? console_device :
+        "console device"),
+       strerror(saved_errno));
+    /* Output the only beep we can, in an effort to fall back on usefulness */
+    printf("\a");
     exit(1);
   }

-  if (ioctl(console_fd, EVIOCGSND(0)) != -1)
+  /* Verify that console_fd is actually a character device special file */
+  if (1) {
+    struct stat sb;
+    if (-1 == fstat(console_fd, &sb)) {
+      perror("fstat");
+      exit(1);
+    }
+    if (S_ISCHR(sb.st_mode)) {
+      /* GOOD: console_fd is a character device special file. Use it. */
+    } else {
+      /* BAD: console_fd is not a character device special file. Do
+       * not use it any further, and certainly DO NOT WRITE to it.
+       */
+      fprintf(stderr,
+         "%s: opened console '%s' is not a character special device\n",
+         argv[0], console_device);
+      exit(1);
+    }
+  }
+
+  /* Determine the API supported by the opened console device */
+  if (ioctl(console_fd, EVIOCGSND(0)) != -1) {
+    if (parms->verbose) {
+      printf("Using BEEP_TYPE_EVDEV\n");
+    }
     console_type = BEEP_TYPE_EVDEV;
-  else
+  } else if (ioctl(console_fd, KIOCSOUND, 0) >= 0) {
+    /* turning off the beeps should be a safe way to check for API support */
+    if (parms->verbose) {
+      printf("Using BEEP_TYPE_CONSOLE\n");
+    }
     console_type = BEEP_TYPE_CONSOLE;
+  } else {
+    fprintf(stderr,
+       "%s: console device '%s' does not support either API\n",
+       argv[0], console_device);
+    /* Output the only beep we can, in an effort to fall back on usefulness */
+    printf("\a");
+    exit(1);
+  }
+
+  /* At this time, we know what API to use on which device, and we do
+   * not have to fall back onto printing '\a' any more.
+   */
+
+  /* After all the initialization has happened and the global
+   * variables used to communicate with the signal handlers have
+   * actually been set up properly, we can finally install the signal
+   * handlers. As we do not start making any noises, there is no need
+   * to install the signal handlers any earlier.
+   */
+  signal(SIGINT,  handle_signal);
+  signal(SIGTERM, handle_signal);

   /* this outermost while loop handles the possibility that -n/--new has been
      used, i.e. that we have multiple beeps specified. Each iteration will
-- 
2.17.2

jwilk commented 5 years ago

If a device name is given as a command line argument, only allow device names starting with "/dev/input/".

This check if susceptible to directory traversal. The attacker can use dot-dot sequences to bypass the check: /dev/input/../../anything/they/like

rain-1 commented 5 years ago

it's better to not make the binary SUID

ndim commented 5 years ago

I can see now why the evdev write(2)-based API was introduced: It allows the admin to set permissions for the device file, instead of the kernel having a hardcoded root requirement for the KIOCSOUND ioctl(2) of the console API.

So the priority for beep should absolutely be to start with looking for /dev/input/by-path/platform-pcspkr-event-spkr and try that first. Now the system must be set up such that the non-root user has write access to that device, such as the following:

setfacl -m u:${user}:w /dev/input/by-path/platform-pcspkr-event-spkr

Now the permission setup needs to be automated with whatever hooks the respective system offers for that (distro packagers' job), and beep needs to reliably find the pcspkr evdev device file. Then the KIOCSOUND API can be demoted to fallback for the rare case in which beep is running as root anyway and the pkspkr evdev device file has not been set up (maybe early during the boot or similarly).

Are there other device names for the PC speaker than /dev/input/by-path/platform-pcspkr-event-spkr? Just checking them in turn should work.

ndim commented 5 years ago

If a device name is given as a command line argument, only allow device names starting with "/dev/input/".

This check if susceptible to directory traversal. The attacker can use dot-dot sequences to bypass the check: /dev/input/../../anything/they/like

Gnarg. Thank you. Added realpath(3) for this check in https://github.com/ndim/beep/commit/9d2d0c1ac754a39711143ba85ac451d989ef0517 - but still trying to remove the whole device name command line parameter from beep while keeping beep beeping.

jwilk commented 5 years ago

realpath() can be abused to leak information about otherwise-inaccessible files. For example, consider the following command: beep -e /home/alice/foobar/../../../dev/input/by-path/platform-pcspkr-event-spkr

If /home/alice/foobar is a directory, the command will succeed.
If /home/alice/foobar is a file, realpath() will fail with ENOTDIR.
If /home/alice/foobar doesn't exist, realpath() will fail with ENOENT.

I think you should outright forbid .. path components in the device path.

ndim commented 5 years ago

realpath() can be abused to leak information about otherwise-inaccessible files. For example, consider the following command: beep -e /home/alice/foobar/../../../dev/input/by-path/platform-pcspkr-event-spkr
* If `/home/alice/foobar` is a directory, the command will succeed.

* If `/home/alice/foobar` is a file, realpath() will fail with `ENOTDIR`.

* If `/home/alice/foobar` doesn't exist, realpath() will fail with `ENOENT`.
I think you should outright forbid .. path components in the device path.

Symlinks to those files have no .. but still leak the same information. This kind of path cleanup is something that should be done once in a libc caliber library instead of a little utility like beep, or be avoided altogether.

The best way appears to forbid running beep as setuid or setgid, which gets rid of the whole problem with one user giving a device name and beep acting on that device name as another user. Checking the device appears like a good idea, but I am not so sure abo.

Removing the -e parameter would be another way to avoid the issue, but I am not certain that beep could reliably find the correct device to use in all cases, so I would like to give users that opportunity. Also while testing, the -e parameter is quite helpful.

ndim commented 5 years ago

Given the lack of activity in this code repositiory since 2013, I have taken up the codebase, fixed a number of issues including the two CVEs (CVE-2018-0492 and CVE-2018-1000532) we have discussed here, and put it up on https://github.com/spkr-beep/beep with release 1.4.2 being current.

johnath / beep

CVE-2018-0492 #11