[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#40832: alsa-lib cannot find its plugins
From: |
Danny Milosavljevic |
Subject: |
bug#40832: alsa-lib cannot find its plugins |
Date: |
Wed, 29 Jul 2020 13:18:44 +0200 |
Hi Leo,
On Tue, 28 Jul 2020 19:56:23 -0400
Leo Famulari <leo@famulari.name> wrote:
> On Tue, Jul 28, 2020 at 12:52:41PM +0200, Danny Milosavljevic wrote:
> > some comments on the lastest patch:
>
> Thank you for reviewing the patch!
>
> > * The entire alsa-lib seems to use the idiom "malloc and then strcpy", or
> > "malloc and then sprintf", or, worse, "malloc, strcpy and multiple strcat".
> > These are a buffer overflow waiting to happen (when changing part of those
> > while doing ongoing maintenance; also the places where they use "+" is not
> > checked for overflow). That said, if they do it, we can do it that way,
> > too.
>
> This confirms what I felt — it's hard to feel confident about the bounds
> checking in this code. It seems to be based on the names of the plugin
> libraries not exceeding some magic length. It's hard to balance "doing
> the right thing" and using upstream's idioms.
After thinking about it more, I think it's much worse if the thing that is
stuck into the malloced block is the value of an environment variable.
When it's a compile-time variable you basically trust the code and the
distribution package not to have too-long paths in there that could overflow
the "+" in malloc(... + ). A distribution or upstream could do much worse
things than that, so that is not a credible threat to worry about.
For a runtime variable like the environment variable (that anyone can set to
anything), I am very much in favor of not using malloc(... + ) and instead
using asprintf, in order to prevent an exploitable buffer overflow just by
setting up an environment variable.
> When writing the patch, my investigation into the code made decide that
> it would not overflow, but now I don't remember why I thought that.
Thanks for that remark. It made me think and I came to the recommendation
above.
pgp3CVad0DRl4.pgp
Description: OpenPGP digital signature