Beep security update • Hacker News Board

Re: Beep security update

#41

Post by amelius » Wed, Apr 04, 2018, 11:14 AM UTC

jstimpfle wrote: post #22

madez wrote: post #19

Earlier quoted context omitted.

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

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.

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

Re: Beep security update

#42

Post by jstimpfle » Wed, Apr 04, 2018, 11:17 AM UTC

Kamshak wrote: post #39

madez wrote: post #19

Earlier quoted context omitted.

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

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…

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.

Re: Beep security update

#43

Post by jwilk » Wed, Apr 04, 2018, 11:21 AM UTC

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

Re: Beep security update

#44

Post by laumars » Wed, Apr 04, 2018, 11:25 AM UTC

FreeFull wrote: post #24

dblotsky wrote: post #4

Earlier quoted context omitted.

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

Re: Beep security update

#45

Post by netsec_burn » Wed, Apr 04, 2018, 11:26 AM UTC

jfindley wrote: post #36

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.

Re: Beep security update

#46

Post by alxlaz » Wed, Apr 04, 2018, 11:41 AM UTC

alkonaut wrote: post #35

alxlaz wrote: post #30

Earlier quoted context omitted.

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 ve…

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?

Re: Beep security update

#47

Post by alxlaz » Wed, Apr 04, 2018, 11:50 AM UTC

jfindley wrote: post #36

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

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

Re: Beep security update

#48

Post by jstimpfle » Wed, Apr 04, 2018, 11:51 AM UTC

amelius wrote: post #41

jstimpfle wrote: post #22

Earlier quoted context omitted.

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.

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

Re: Beep security update

#49

Post by DCoder » Wed, Apr 04, 2018, 12:00 PM UTC

alxlaz wrote: post #47

jfindley wrote: post #36

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

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 rac…

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.

Re: Beep security update

#50

Post by alkonaut » Wed, Apr 04, 2018, 12:00 PM UTC

alxlaz wrote: post #46

alkonaut wrote: post #35

Earlier quoted context omitted.

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

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