New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVE-2018-0492 #11

Open
MarkusTeufelberger opened this Issue Apr 3, 2018 · 21 comments

Comments

Projects
None yet
@MarkusTeufelberger

MarkusTeufelberger commented Apr 3, 2018

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

@s7726

This comment has been minimized.

s7726 commented Apr 3, 2018

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

This comment has been minimized.

MarkusTeufelberger commented Apr 3, 2018

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

This comment has been minimized.

s7726 commented Apr 3, 2018

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

@Nicals

This comment has been minimized.

Nicals commented Apr 4, 2018

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

This comment has been minimized.

markand commented Apr 4, 2018

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

@Stargateur

This comment has been minimized.

Stargateur commented Apr 4, 2018

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

This comment has been minimized.

MarkusTeufelberger commented Apr 4, 2018

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

This comment has been minimized.

LangerJan commented Apr 4, 2018

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

This comment has been minimized.

Stargateur commented Apr 4, 2018

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

This comment has been minimized.

LangerJan commented Apr 4, 2018

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

@s7726

This comment has been minimized.

s7726 commented Apr 4, 2018

@zb3

This comment has been minimized.

zb3 commented Apr 4, 2018

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

This comment has been minimized.

rain-1 commented Apr 4, 2018

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

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Apr 4, 2018

app-misc/beep: patch against CVE-2018-0292.
Bug: https://bugs.gentoo.org/652330
See-Also: johnath/beep#11
Package-Manager: Portage-2.3.19, Repoman-2.3.6
@LangerJan

This comment has been minimized.

LangerJan commented Apr 5, 2018

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

This comment has been minimized.

jwilk commented Apr 5, 2018

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

This comment has been minimized.

rain-1 commented Apr 5, 2018

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

This comment has been minimized.

SpComb commented Apr 5, 2018

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

This comment has been minimized.

optnfast commented Apr 6, 2018

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

This comment has been minimized.

jwilk commented Apr 6, 2018

Opening tape devices or named pipes can have side effects.

@AGWA

This comment has been minimized.

AGWA commented Apr 8, 2018

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

This comment has been minimized.

Arignir commented Apr 10, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment