> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of Reimar
> D?ffinger
> Sent: 2021年2月26日 17:21
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V2 1/7] libavdevice/v4l2.c: fix build
> warning for [-Wformat-truncation=]
>
>
> > 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
For the code in this function, max length of file name is fixed, see the code
below.
Anyway, it still increases the on-stack variable size which might have
potential security
issue.
snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name);
'man readdir' shows:
struct dirent {
ino_t d_ino; /* Inode number */
off_t d_off; /* Not an offset; see below */
unsigned short d_reclen; /* Length of this record */
unsigned char d_type; /* Type of file; not supported
by all filesystem types */
char d_name[256]; /* Null-terminated filename */
};
>
> 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...
with such concern, maybe we can just let this build warning there.
_______________________________________________
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".
