artswrapper's new braces (Re: artswrapper defanged)

[Kevin Puetz]

Stefan Westerfeld wrote:

>    Hi!
> 
> On Wed, Aug 07, 2002 at 10:28:55PM -0500, Kevin Puetz wrote:
>> [...]
>>
>> arts will drop RT permissions entirely if an untrusted module is loaded,
>> *before* executing *any* code from this module.
>> 
>> after some further discussion, we settled in the following definition of
>> a trusted module: anything represented by a .la file owned by root.
>> 
>> [...]
>> 
>> Aftre I've actually tested this fix to ensure it does what I intended it
>> to, if feedback is positive, I will commit.
> 
> I have a few comments on the patch:
> 
> 1. as long as artsd doesn't drop RT prio before exec()ing artsmessage, a
>    trivial exploit is to provide an own version of artsmessage, which goes
>    into a for(;;); loop

Yes, RT needs to drop priority first. Not a big deal, though.

> 2. the .la-file definition might possibly be not sufficient, as I don't
> know
>    whether for instance in some situations libraries in LD_LIBRARY_PATH
>    could be used before these specified in the .la file - before you
>    commit the patch and claim it to be secure, you will need to ensure
>    this is not the case

since artswrapper is SUID when we run realtime, the runtime (kernel, linker, 
whatever) should have already removed all env vars influencing linking 
before running it. Otherwise, you could use this same technique to cause 
artswrapper to do anything you wanted as root - why bother with a user DoS 
then? So this one is taken care of for us before artswrapper even starts, 
as a basic part of the security of SUID executables.

> 3. another exploit not adressed by your patch is to start 2 artsds
>    simultaneously, each consuming 50% CPU power (for instance as synthesis
>    network), which is not catched by the self-monitoring

True... for this, we probably want to drop a root owned 
/var/run/artswrapper.pid (containing the PID of the artsd, not the wrapper, 
but I think calling it artswrapper.pid is a better fit...). Then we give RT 
to a new arts only if there isn't one already.

This is a good catch (I was thinking of artsd as exclusive anyway due to 
/dev/dsp locking, but a) that's not true on some cards and b) there's 
always -o null. So this needs to be added to artswrapper. Any volunteers? 
I'll get to it if nobody steps up, but not until after I've got the rest of 
this finished (since this one is a separate issue)

> 4. your patch might not address the possibility to start an artsd, let it
>    consume all cpu usage (which is detected by the self monitoring only
>    after some time), kill it, and start another artsd - which will lead to
>    a 99.9999% effective DOS attack - to adress this, artswrapper could be
>    modified somehow not to give RT prio to artsd for a while after one
>    (possible) DOS attack has taken place

No, it doesn't... but one we have a pid file, checking the mtime shouldn't 
be too hard. We might want to do a little more than just mtime though, 
since we probably want to allow one immediate restart (in case it was just 
a crash) then assume after two something is deeply wrong.

This could even be unintentional... noatun will immediately restart a 
crashed artsd, so a bad module crashing because of simple bugginess could 
mostly kill a machine this way.

> 5. I think it might be easier to code (and thus: easier to verify that the
> code
>    actually does the correct thing), if you _never_ load any untrusted
>    plugin into an artsd that is started with realtime priority, and inform
>    the user about that (i.e. "could not load untrusted plugin ... because
>    running with RT prio - either disable RT prio manually or make plugin
>    trusted doing chown/mod ...).

If pthreads doesn't provide a good way to iterate theads, this is definitely 
the way to go. We only support pthreads though, right? I'm pretty sure I 
can get all the PID's in a sane fashion. But if not, this would be another 
reasonable approach. It's a little less nice if you only rarely use an 
untrusted plugin though.

>    This is because it might be hard to implement to repriorize all already
>    running threads (because they not necessarily use Arts::Thread - for
>    instance they might be spawned by a specific decoder implementation,
>    such as Xine).

> And yes, given that "the other solution" (which is writing an RT monitor)
> seems to have very ugly issues on its own, I think if a solution in the
> style of your solution can adress all these items, possibly this is the
> best "fix" we can come up with. Its better than nothing, anyway ;).

I think it's the best approach we can do short of getting some kernel 
support. SCHED_FIFO is intended as an end-run around scheduler fairness, 
after all :-)

>    Cu... Stefan