CVE-2018-0492
Opened this issue · 29 comments
Just a short heads-up:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-0492
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.
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.
Ubuntu actually had an additional patch as well that covered handling one of the system SIGNALs.
Upstreaming is for chumps ;)
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...
I wonder if people will understand that free accept null pointers as well.
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 ?
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.
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.
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)
Maybe it has to do with calling free() inside the signal handler, which is not considered an async-signal-safe function.
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.
It's possible that there is no actual priv esc. I will try to reach out to lamb.
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 (seeman 7 signal-safetyon 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.
On Hacker News, terom wrote:
The while loop in
maincallsplay_beepmultiple times. Each call toplay_beepopens the--deviceand sets the globalconsole_fd, and then sets the globalconsole_typebased on theioctl(EVIOCGSND)error, before callingdo_beep.This normally prevents the user from writing to arbitrary files with
--device, because without theioctl(EVIOCGSND)succeeding,do_beepwithBEEP_TYPE_CONSOLEonly does a (harmless?)ioctl(KIOCSOUND), not awritewith thestruct input_event. However, the signal handler callsdo_beepdirectly using the globals set byplay_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 firstplay_beep, and then race the open/ioctl in the secondplay_beepwith the signal handler such thatdo_beepgets called withconsole_fdpointing to your arbitrary file, and withconsole_typestill set toBEEP_TYPE_EVDEV, resulting in awriteto your arbitrary file.Exploiting that for privesc would require control over the
struct input_eventfor thewrite...handle_signalcallsdo_beepwith a fixedfreqof 0, so all of the initialized fields are set to fixed values... However, there's an unitializedstruct timevalat the beginning of thestruct input_event, and it's allocated on the stack...
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.
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.
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.
Opening tape devices or named pipes can have side effects.
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.
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.
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
- 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
it's better to not make the binary SUID
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.
- 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 ndim@9d2d0c1 - but still trying to remove the whole device name command line parameter from beep while keeping beep beeping.
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/foobaris a directory, the command will succeed. - If
/home/alice/foobaris a file, realpath() will fail withENOTDIR. - If
/home/alice/foobardoesn't exist, realpath() will fail withENOENT.
I think you should outright forbid .. path components in the device path.
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.
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.