> On 25 Feb 2021, at 18:52, Chad Fraleigh <ch...@triularity.org> wrote:
>
> On 2/24/2021 10:38 PM, Guo, Yejun wrote:
>> libavdevice/v4l2.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
>> index 365bacd771..e11d10d20e 100644
>> --- a/libavdevice/v4l2.c
>> +++ b/libavdevice/v4l2.c
>> @@ -1046,7 +1046,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx,
>> AVDeviceInfoList *device_l
>> return ret;
>> }
>> while ((entry = readdir(dir))) {
>> - char device_name[256];
>> + char device_name[512];
>
> Is there a portable path max constant that would be better for cases like
> this, rather than just picking another arbitrary buffer size?
There is PATH_MAX/MAX_PATH, however the problems are:
1) They are not necessarily a strict limit on path length, so no guarantee all
file names fit
2) Even if there is such a guarantee, that only applies to VALID file names,
however the files here are user input ans not necessarily valid, so using those
defines does not fix things anyway
Speaking generally I have doubts that these patches actually IMPROVE security
and don’t make it actually worse.
On the plus side I see:
- they fix truncations in those specific cases
On the minus side I see:
- file names can clearly be longer, so there is still a truncation issue even
after these patches, truncation just happens elsewhere and at larger lengths
- the warning is removed, so now there is nothing that reminds developers of
this issue existing
- it relies on “magic constants” that can easily become outdated and
re-introduce the issue again at any time
- there is as far as I can tell no evidence that any of these truncations cause
actual issues, or if so in which circumstances, so there is no evidence of
benefit. However as with all changes there is a risk of introducing new bugs
- size of on-stack variables are increased which may decrease effectiveness of
stack protection
As a result personally I am mostly worried about these patches if they are
applied without a deep security review of each and ensuring the issues are
understood and thoroughly fixed.
I before suggested using av_asprintf, which does at least permanently solve the
truncation issue without magic constants and eliminates on-stack arrays,
however I should also add that it might create a allocation failure issue,
which then opens up the whole “how to handle allocation failure without
introducing a even more tricky security issue”.
So I am afraid that these issues will be annoyingly costly to fix, and I am not
sure how big the desire is to do so...
Best regards,
Reimar
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
