Beep security update • Hacker News Board

Re: Beep security update

#91

Post by lwf » Thu, Apr 05, 2018, 4:10 AM UTC

0x0 wrote: post #86

Does GPL say anything about distributing source via git repositories? I mean, it talks about "preferred form of the work for making modifications". But when I click through to the Debian PTS page for Beep at https://tracker.debian.org/pkg/beep it shows a git link to http://git.deb.at/w/pkg/beep.git which was last modified 23 months ago, so obviously does not contain this security fix. How is the GPL upheld if securit…

(N.B.: I'm on the Debian ftpmaster team, which reviews all new software in Debian to ensure it matches our policies and commitments)

> Does GPL say anything about distributing source via git repositories?

Nope.

> I mean, it talks about "preferred form of the work for making modifications".

This is generally interpreted as a fancy way of saying "source code" — e.g. a minified JavaScript program or something that could be decompiled isn't the "preferred form… for making modifications".

Source for all Debian packages can be retrieved via `apt-get source PACKAGE`, and via sources.debian.org[3].

Historically, the only hard requirement was that all patches be made available as a `.diff.gz` applied against the upstream, although for many years packages have since standardised on using `quilt`[2]. Some maintainers may also use git (and wrappers around it) to make things easier to manage, but quilt is sufficiently straightforward to work with that it's acceptable for most purposes.

The VCS page on the BTS points to the repository for the package. It contains an unreleased version (1.3.3-5), the security update only is a patch on the previous version released in Debian.

The last upstream release was 1.3 (2010), available on GitHub[1]. This is a Debian-specific patch against beep. We'd like all patches to make it upstream, and ensure maximum clarity, but we have limited time, and our first priority is the users of Debian. Sometimes, upstreams lose time, or there are coordination issues that prevent it. (not saying that's the case here)

[1]: http://github.com/johnath/beep/ [2]: https://wiki.debian.org/UsingQuilt [3]: https://sources.debian.org/src/beep/unstable/

Re: Beep security update

#92

Post by terom » Thu, Apr 05, 2018, 7:54 AM UTC

jwilk wrote: post #43

nxtafan wrote: post #3

https://github.com/johnath/beep/issues/11#issuecomment-37838...

The Debian patch with diff highlighting: https://gist.github.com/jwilk/561ae35894756aae1e31503d0c52db... AFAICS, it does this: 1) Fixes use of uninitialized memory. 2) Fixes double-free in the signal handler. 3) Makes sure that the device is opened only once. I still don't understand what exactly the bug is supposed to be. I also don't understand what is the purpose of 3). With this fix applied, it's still possible f…

My speculation on the race condition fixed in the patch:

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...

Seems like a curious security vulnerability, I'll assume the debian security team must have a working PoC in order to actually call it out as a local privesc vulnerability... I'd love to see the actual PoC eventually :)

Re: Beep security update

#93

Post by isotopp » Thu, Apr 05, 2018, 8:03 AM UTC

tomsmeding wrote: post #53

akrasuski1 wrote: post #8

Also see https://holeybeep.ninja/

The patch given on that page includes the line: !id>~/pwn.lol;beep # 13-21 12:53:21.000000000 +0100 Not bothering to test it, but I don't think that contributes to patching beep. Why do people always need to be annoying?

Congratulations. You found the actual exploit.

http://git.savannah.gnu.org/cgit/patch.git/tree/src/pch.c#n2...

patch calls /bin/ed. /bin/ed has a ! command that feeds stuff to /bin/sh. Feeding unreviewed patches to the patch command is actually arbitrary command execution.

This is particularly awesome if patch is being used by other programs (i.e. a CI pipeline or other contexts).

Re: Beep security update

#94

Post by jwilk » Thu, Apr 05, 2018, 9:04 AM UTC

terom wrote: post #92

jwilk wrote: post #43

Earlier quoted context omitted.

The Debian patch with diff highlighting: https://gist.github.com/jwilk/561ae35894756aae1e31503d0c52db... AFAICS, it does this: 1) Fixes use of uninitialized memory. 2) Fixes double-free in the signal handler. 3) Makes sure that the device is opened only once. I still don't understand what exactly the bug is supposed to be. I also don't understand what is the purpose of 3). With this fix applied, it's still possible f…

My speculation on the race condition fixed in the patch: 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)`…

Yeah, that makes sense. I took the liberty of pasting this comment to the upstream bug tracker:

https://github.com/johnath/beep/issues/11#issuecomment-37886...

Re: Beep security update

#95

Post by arghwhat » Thu, Apr 05, 2018, 2:07 PM UTC

cryptonector wrote: post #74

simias wrote: post #51

Earlier quoted context omitted.

I'll be quicker to blame the signal API than the programming language on that front. Dealing with unix signals correctly and robustly is far from trivial and rife with footguns. For instance I believe that Rust still doesn't have a good general purpose solution for handling signals that doesn't involve the libc and unsafe code. Signal is basically the crappiest form of IPC available on a modern operating system short…

The simplest way to write safe signal handlers is to only ever: - write(2) to STDERR_FILENO - write(2) to a "self-pipe" (i.e., a pipe where the same process is waiting on in its event loop), thus turning the async signal event into an async *I/O* event that can be handled without any constraints regarding async-signal-safety - _exit() Yes, there are other async-signal-safe functions that can be called from a signal h…

> ALSO, while we're at it, the only global or thread-local variables you can read from or write to from a signal handler must be of type volatile sig_atomic_t (or else volatile of any other integral or pointer type that you can use with atomic operations). This is very important.

I'd rather recommend C11 atomics without volatile. They use memory barriers to guarantee visibility, and getting the idea that "volatile" should be used in the context of atomics is a bad idea, as it allows reordering (unlike memory barriers).

Re: Beep security update

#96

Post by jwilk » Thu, Apr 05, 2018, 6:14 PM UTC

lwf wrote: post #91

0x0 wrote: post #86

Does GPL say anything about distributing source via git repositories? I mean, it talks about "preferred form of the work for making modifications". But when I click through to the Debian PTS page for Beep at https://tracker.debian.org/pkg/beep it shows a git link to http://git.deb.at/w/pkg/beep.git which was last modified 23 months ago, so obviously does not contain this security fix. How is the GPL upheld if securit…

(N.B.: I'm on the Debian ftpmaster team, which reviews all new software in Debian to ensure it matches our policies and commitments) > Does GPL say anything about distributing source via git repositories? Nope. > I mean, it talks about "preferred form of the work for making modifications". This is generally interpreted as a fancy way of saying "source code" — e.g. a minified JavaScript program or something that could…

> Source for all Debian packages can be retrieved via `apt-get source PACKAGE`, and via sources.debian.org

But sources.d.o seem to lack security updates (or at least the security update for this package is missing). :-\

Re: Beep security update

#97

Post by 0x0 » Thu, Apr 05, 2018, 7:23 PM UTC

lwf wrote: post #91

0x0 wrote: post #86

Does GPL say anything about distributing source via git repositories? I mean, it talks about "preferred form of the work for making modifications". But when I click through to the Debian PTS page for Beep at https://tracker.debian.org/pkg/beep it shows a git link to http://git.deb.at/w/pkg/beep.git which was last modified 23 months ago, so obviously does not contain this security fix. How is the GPL upheld if securit…

(N.B.: I'm on the Debian ftpmaster team, which reviews all new software in Debian to ensure it matches our policies and commitments) > Does GPL say anything about distributing source via git repositories? Nope. > I mean, it talks about "preferred form of the work for making modifications". This is generally interpreted as a fancy way of saying "source code" — e.g. a minified JavaScript program or something that could…

Thanks for your detailed reply.

I'm still curious about the general case of GPL and Git. If I were to hack on some GPL-licensed software, my "preferred form of the work for making modifications" would hands-down be a .git repository, because all the individual git commits and commitmsgs are important (meta-)information, almost as important as the source code files themselves. It also makes collaborating on improvements much easier when you can work on git commit objects, with their parent(s) commit hash references. Without those, determining where and if a .patch should be applied to a source code dump becomes much harder.

In other words, if there exists a .git repository for a given piece of software, and all one gets is a .tar.gz flat source dump snapshot, I feel like... something has been left out?

Re: Beep security update

#98

Post by terom » Thu, Apr 05, 2018, 7:36 PM UTC

terom wrote: post #92

jwilk wrote: post #43

Earlier quoted context omitted.

The Debian patch with diff highlighting: https://gist.github.com/jwilk/561ae35894756aae1e31503d0c52db... AFAICS, it does this: 1) Fixes use of uninitialized memory. 2) Fixes double-free in the signal handler. 3) Makes sure that the device is opened only once. I still don't understand what exactly the bug is supposed to be. I also don't understand what is the purpose of 3). With this fix applied, it's still possible f…

My speculation on the race condition fixed in the patch: 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)`…

There's now a PoC exploiting this race, seemingly placing the 32-bit -l option value into the uninitialized part of the `struct input_event` to modify a shell script that runs as root: https://gist.github.com/fkt/5f8f9560ef54e11ff7df8bec09dc8f9a

Remains to be seen if the PoC actually works though, I've been unable to win the race so far, although the varying ioctl errors indicate it might be close?

Re: Beep security update

#99

Post by master-litty » Thu, Apr 05, 2018, 8:02 PM UTC

tejasmanohar wrote: post #84

Is it worrisome that the Debian distribution comes with a package with this note in the README Decide for yourself, of course, but it looks safe to me - there's only one buffer and fgets doesn't let it overflow, there's only one file opening, and while there is a potential race condition there, it's with /dev/console. If someone can exploit this race by replacing /dev/console, you've got bigger problems. :) It also a…

It illustrates the author's attention to safety and security. Opposite of worrisome, even if it turned out exploitable.

True but same idea applies. If your end device is maliciously replaceable you've got bigger problems.

Re: Beep security update

#100

Post by JdeBP » Thu, Apr 05, 2018, 9:14 PM UTC

cryptonector wrote: post #79

bkeroack wrote: post #77

Earlier quoted context omitted.

Option 2 is very similar to how signal handlers work in Go. When a signal is received, a value is written to a channel and the library user is responsible for reading values from the channel and responding appropriately. https://golang.org/pkg/os/signal/#example_Notify

It's the only sane thing to do. A write(2) to STDERR_FILENO for verbosity/debugging is fine, but mostly you don't want to do this because it will interleave with any non-line-buffered stdio writes to it... An _exit(2) is also OK if you really want to do that, but generally you want to do some cleanup, so might as well do the self-pipe thing every time. The only tricky thing is when you use SA_SIGINFO and you want to…

It is not the only sane thing to do.

kevent() is another way to handle signals. It puts handling them into the program's main event loop, which is done synchronously with normal event-dispatching mechanisms and so does not have worries about asynchronous signal safety, because with kevent() they are just another type of filter.