
Beep security update - DyslexicAtheist
https://www.debian.org/security/2018/dsa-4163
======
garethrees
The author of beep needs to read the POSIX specification on async-signal-
safety [1]. In particular, it is not safe to call exit, free, ioctl, putchar,
or perror from a signal handler.

[1]
[http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2...](http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html)

~~~
madez
> In particular, it is not safe to call exit, free, ioctl, putchar, or perror
> from a signal handler.

Why is it accepted by the compiler, then?

~~~
simias
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 of emulating a mouse and keyboard and typing into the other
application's terminal window.

~~~
cryptonector
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 handler, but it's generally not worth it. Adhering to my more
constrained approach will keep your code safe and will make it easier to
always get it right.

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. E.g., imagine using
SIGUSR1/2 to manage verbosity levels...

~~~
bkeroack
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](https://golang.org/pkg/os/signal/#example_Notify)

~~~
cryptonector
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 pass the
siginfo_t data to the event loop. You can write(2) that to the self-pipe, but
you have to be careful of the possibility that it will fill up. You can always
create a new pipe(2), write(2) the siginfo_t to it, close(2) the write end,
and send the read side fd via a socketpair(2) that the event loop listens to.

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

~~~
slrz
Sure, if you're going to use non-portable constructs, there're better
alternatives to the self-pipe thing. Linux has something roughly similar with
signalfd.

The nice thing about the write-a-byte-to-the-pipe thing is that it works
virtually everywhere.

------
peoplewindow
From the beep readme:

 _By default beep is not installed with the suid bit set, because that would
just be zany. On the other hand, if you do make it suid root, all your
problems with beep bailing on ioctl calls will magically vanish, which is
pleasant, and the only reason not to is that any suid program is a potential
security hole. Conveniently, beep is very short, so auditing it is pretty
straightforward._

[https://github.com/johnath/beep](https://github.com/johnath/beep)

~~~
exikyut
And all this time I've read that and thought "well at least you're putting it
out there, just in case - almost for the sake of it"

Heh

------
akrasuski1
Also see [https://holeybeep.ninja/](https://holeybeep.ninja/)

~~~
voltagex_
I've got a couple of problems with that page:

* curl | sudo bash for two lines of script

* said script just checks if you have beep installed, not if you're vulnerable

~~~
simias
Not sure if you're aware of that but it's a joke page made as a parody of the
recent "branded vulnerability" craze. I would definitely advise against
running random scripts from joke pages, especially through sudo. In that
regard notice the "TODO" line from the script:

    
    
        #!/bin/sh
        # TODO: Backdoor this machine?
        modprobe pcspkr
        beep -l 1000 -r 3 -f 44000

~~~
jstarks
Just recently the script said this instead:

    
    
        #!/bin/sh
        curl https://l0.re/hb | bash
        modprobe pcspkr
        beep -l 1000 -r 3 -f 44000
    

And then that embedded URL says:

    
    
        echo ohai
    

But only after a long delay -- perhaps it is using one of the previously
documented techniques to determine whether it's being piped to bash and
behaving differently.

And now that I try the original curl again, that first line is gone
completely:

    
    
        #!/bin/sh
        modprobe pcspkr
        beep -l 1000 -r 3 -f 44000
    

Strange.

------
jfindley
I'd love to hear the backstory. Who on _earth_ goes looking for
vulnerabilities in beep?!

~~~
alxlaz
If I had to guess, I'd say they were doing it because it is often installed
with suid root.

Edit: also, there's a challenge about this in the program's 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. :)"

~~~
DCoder
It should be pointed out that the README is wrong about the race condition –
it is not limited to /dev/console, you can use the -e option to point it to a
different output device.

~~~
jwilk
It doesn't open /dev/console by default either (the default device is
/dev/tty0).

I don't see where the race condition is, though.

------
nxtafan
[https://github.com/johnath/beep/issues/11#issuecomment-37838...](https://github.com/johnath/beep/issues/11#issuecomment-378383752)

~~~
jwilk
The Debian patch with diff highlighting:

[https://gist.github.com/jwilk/561ae35894756aae1e31503d0c52db...](https://gist.github.com/jwilk/561ae35894756aae1e31503d0c52db7e)

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 for a user to open arbitrary file for writing with root privileges,
which is bad.

~~~
terom
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 :)

~~~
terom
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](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?

------
Y_Y
Can't one just

    
    
        sudo echo -e "\7” 
    

anyway? Or if you're trying to do something fancier there is snd-pcsp which
lets use use the piezo as a normal ALSA device (quality may vary). Not that
many modern machines even include such a useful device.

~~~
dblotsky
Nitpick: on my machine, that's:

    
    
        sudo echo -e "\07"

~~~
FreeFull
Simply displaying a character doesn't need sudo at all anyway, so just

    
    
        echo -e "\07"

~~~
laumars
`sudo` isn't there for displaying the character, it's there for ioctl to have
permissions to play the beep on remote terminals[1].

I think some terminal emulators (eg Konsole) will just read char 7 and play a
system defined WAV or other workaround but your more classic terminals (eg
xterm) would potentially have difficulties calling the system bell.

[1] for reference: Xorg terminals are also classed as remote terminals (eg
check the TTY column in `w` to see how Linux assigns pseudo-tty's to terminal
emulators inside X).

~~~
FreeFull
Even in that case, you'd have to run the terminal as root. Running echo as
root won't do anything. X11 also has beep functionality built in, which is
what I'd expect xterm to use.

~~~
laumars
> _Even in that case, you 'd have to run the terminal as root. Running echo as
> root won't do anything._

To be honest that was my assumption as well but I didn't test that theory so
was only reporting on the rationale of the GPs post

> _X11 also has beep functionality built in, which is what I 'd expect xterm
> to use_

X11 runs as root though (that said Xorg may not anymore now as there were
talks about patching Linux so it didn't have to be) so Xorg could easily still
use the standard ioctl bell (that's possibly all Xorg is doing behind the
scenes in fact?)

But as I said earlier, some GUI terminal emulators avoid ioctl entirely and
play a WAV tone instead.

------
0x0
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](https://tracker.debian.org/pkg/beep) it
shows a git link to
[http://git.deb.at/w/pkg/beep.git](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 security fixes aren't available together with the
main git repository?

I don't mean to cause drama, because I think the Debian security team does an
awesome job of keeping an eye on patching everything in the distribution. But
I'm really curious about how one would go about with further development on a
package like this when the git commits aren't available? There's probably a
source tarball for the .debs in the apt repos (hard to tell from a mobile web
browser) but is that good enough if it doesn't contain git commits?

Also I really just wanted to read this security patch source diff on the go :)

~~~
lwf
(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/](http://github.com/johnath/beep/) [2]:
[https://wiki.debian.org/UsingQuilt](https://wiki.debian.org/UsingQuilt) [3]:
[https://sources.debian.org/src/beep/unstable/](https://sources.debian.org/src/beep/unstable/)

~~~
0x0
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?

~~~
lwf
This came up when RedHat stopped breaking out their patches against the Linux
kernel: [https://lwn.net/Articles/432012/](https://lwn.net/Articles/432012/)

Relevant parts quoted:

> While there can certainly be arguments about what "preferred form" means for
> source code distributions, Red Hat's actions here don't seem very close to
> the line. The basic idea is that the code be released in digital form, along
> with any build scripts, so that downstream developers can reproduce the GPL-
> covered binary that is being shipped. It is partly meant to prevent
> recalcitrant vendors from releasing the source on paper—or stamped into clay
> tablets in cuneiform. One could argue that those methods do provide the
> source code, but it certainly isn't in a form preferred by anyone—nor does
> it adhere to the "medium customarily used for software interchange"
> requirement.

> Obfuscated source code, where all the variables and function names get
> turned into unintelligible gibberish, or the source is deliberately split up
> in ways that are harder to work with, are a bit more of a gray area—but not
> much. Those kinds of actions should be seen as clear attempts to circumvent
> the GPL requirements. But that's not at all what Red Hat is doing.

> The tarball that Red Hat is releasing may not be the preferred form for the
> Red Hat kernel developers to use, but that is not part of the requirement.
> Tarball releases of GPL code are a longstanding tradition in the free
> software world. Many projects still do it that way and the FSF itself has
> made releases that way in the past. While it is nice to get some visibility
> into the version control system of a project, it certainly isn't required
> for GPL compliance. No one complained that Red Hat didn't provide access to
> its Git repositories back when it was still providing the broken-out
> patches, for example. A Git repository is definitely the preferred form for
> Red Hat developers to use, but there are lots of reasons that the company
> might want to keep its repositories to itself—something that is obviously
> within its rights.

------
tejasmanohar
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 appears /dev/console is overridable,
[https://github.com/johnath/beep/blob/master/beep.c#L149](https://github.com/johnath/beep/blob/master/beep.c#L149).

~~~
jesboat
FWIW, that comment was written before the option to control the path beep uses
was added. Without the option to control the path, the potential race is only
exploitable by somebody who can write to /dev, in which case that person is
already root.

------
oh5nxo
There's the common minor error with perror(). Intervening printf("\a"); can
foul the errno and you get the

beep: open: not a typewriter

------
alex_duf
That's hilarious

------
baybal2
Next, somebody will find a security hole in True (1)

