[FFmpeg-devel] libswcale: Fix possible string overflow in test

Submitted by Adam Richter on May 12, 2019, 12:40 p.m.

Details

Message ID CAGn-TggLSO73Eie9PDSSChvvotF89AV+vLKe14Jv_f7F=8tCFQ@mail.gmail.com
State New
Headers show

Commit Message

Adam Richter May 12, 2019, 12:40 p.m.
This is a possible fix for a string overflow in some sscanf calls in
libswcale/tests/swscale.c, in the function fileTest(), found by
cppcheck.  Please see the attachment for more discussion of this.

Thanks in advance for considering this patch.

Adam

Comments

Michael Niedermayer May 13, 2019, 11:39 a.m.
On Sun, May 12, 2019 at 05:40:00AM -0700, Adam Richter wrote:
> This is a possible fix for a string overflow in some sscanf calls in
> libswcale/tests/swscale.c, in the function fileTest(), found by
> cppcheck.  Please see the attachment for more discussion of this.
> 
> Thanks in advance for considering this patch.
> 
> Adam

>  swscale.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 337bfa52e3917c2d896ca5c7ba1b669d5970cdab  0002-libswcale-Fix-possible-string-overflow-in-test.patch
> From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001
> From: Adam Richter <adamrichter4@gmail.com>
> Date: Sun, 12 May 2019 05:03:25 -0700
> Subject: [PATCH] libswcale: Fix possible string overflow in test.
> 
> In libswcale/tests/swcale.c, the function fileTest() calls sscanf in
> an argument of "%12s" on character srcStr[] and dstStr[], which are
> only 12 bytes.  So, if the input string is 12 characters, a
> terminating null byte can be written past the end of these arrays.
> 
> This bug was found by cppcheck.
> 
> I am not an ffmpeg or libswcale developer, and I believe that this is
> the first patch I am submitting to ffmpeg, so please let me know if
> I am doing anything wrong in the patch submission process.
> 
> For the same reason, please examine this patch skeptically, especially
> considering that I have not tested this patch other than to see that
> it compiled without complaint and that "make fate" completed with a
> zero exit code.  I do not know if this program actually
> expects these input strings to be a maximum of 11 or 12 characters long.
> In this patch, I assume that they could be 12 characters long, so I have
> extended the array sizes, but perhaps a more correct fix might
> be to change the "%12s" instances to "%11s" instead.
> 
> Thanks in advance for considering this patch.

I actually think 13 is not long enough for the longest name.
Ill fix it, thanks for finding this


[...]
Adam Richter May 13, 2019, 6:32 p.m.
Hi, Michael.

Thanks for reviewing that possible string overflow found by cppcheck
and proposing to try to make a better fix.  I'll assume no further
action on my part for this fix is necessary unless anyone tells me
otherwise.

Adam


Adam

On Mon, May 13, 2019 at 4:39 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> On Sun, May 12, 2019 at 05:40:00AM -0700, Adam Richter wrote:
> > This is a possible fix for a string overflow in some sscanf calls in
> > libswcale/tests/swscale.c, in the function fileTest(), found by
> > cppcheck.  Please see the attachment for more discussion of this.
> >
> > Thanks in advance for considering this patch.
> >
> > Adam
>
> >  swscale.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 337bfa52e3917c2d896ca5c7ba1b669d5970cdab  0002-libswcale-Fix-possible-string-overflow-in-test.patch
> > From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001
> > From: Adam Richter <adamrichter4@gmail.com>
> > Date: Sun, 12 May 2019 05:03:25 -0700
> > Subject: [PATCH] libswcale: Fix possible string overflow in test.
> >
> > In libswcale/tests/swcale.c, the function fileTest() calls sscanf in
> > an argument of "%12s" on character srcStr[] and dstStr[], which are
> > only 12 bytes.  So, if the input string is 12 characters, a
> > terminating null byte can be written past the end of these arrays.
> >
> > This bug was found by cppcheck.
> >
> > I am not an ffmpeg or libswcale developer, and I believe that this is
> > the first patch I am submitting to ffmpeg, so please let me know if
> > I am doing anything wrong in the patch submission process.
> >
> > For the same reason, please examine this patch skeptically, especially
> > considering that I have not tested this patch other than to see that
> > it compiled without complaint and that "make fate" completed with a
> > zero exit code.  I do not know if this program actually
> > expects these input strings to be a maximum of 11 or 12 characters long.
> > In this patch, I assume that they could be 12 characters long, so I have
> > extended the array sizes, but perhaps a more correct fix might
> > be to change the "%12s" instances to "%11s" instead.
> >
> > Thanks in advance for considering this patch.
>
> I actually think 13 is not long enough for the longest name.
> Ill fix it, thanks for finding this
>
>
> [...]
>
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Carl Eugen Hoyos May 13, 2019, 7:42 p.m.
Am Mo., 13. Mai 2019 um 20:33 Uhr schrieb Adam Richter <adamrichter4@gmail.com>:

> Thanks for reviewing that possible string overflow found by cppcheck
> and proposing to try to make a better fix.  I'll assume no further
> action on my part for this fix is necessary unless anyone tells me
> otherwise.

See:
http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=9d269301

Please remember not to top-post here, Carl Eugen

Patch hide | download patch | download mbox

From 8b5f994bcd2576588149f228695823b5cf8d3dc8 Mon Sep 17 00:00:00 2001
From: Adam Richter <adamrichter4@gmail.com>
Date: Sun, 12 May 2019 05:03:25 -0700
Subject: [PATCH] libswcale: Fix possible string overflow in test.

In libswcale/tests/swcale.c, the function fileTest() calls sscanf in
an argument of "%12s" on character srcStr[] and dstStr[], which are
only 12 bytes.  So, if the input string is 12 characters, a
terminating null byte can be written past the end of these arrays.

This bug was found by cppcheck.

I am not an ffmpeg or libswcale developer, and I believe that this is
the first patch I am submitting to ffmpeg, so please let me know if
I am doing anything wrong in the patch submission process.

For the same reason, please examine this patch skeptically, especially
considering that I have not tested this patch other than to see that
it compiled without complaint and that "make fate" completed with a
zero exit code.  I do not know if this program actually
expects these input strings to be a maximum of 11 or 12 characters long.
In this patch, I assume that they could be 12 characters long, so I have
extended the array sizes, but perhaps a more correct fix might
be to change the "%12s" instances to "%11s" instead.

Thanks in advance for considering this patch.

Signed-off-by: Adam Richter <adamrichter4@gmail.com>
---
 libswscale/tests/swscale.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libswscale/tests/swscale.c b/libswscale/tests/swscale.c
index e72c4c3306..cb731f6211 100644
--- a/libswscale/tests/swscale.c
+++ b/libswscale/tests/swscale.c
@@ -312,10 +312,10 @@  static int fileTest(const uint8_t * const ref[4], int refStride[4],
     while (fgets(buf, sizeof(buf), fp)) {
         struct Results r;
         enum AVPixelFormat srcFormat;
-        char srcStr[12];
+        char srcStr[13];
         int srcW = 0, srcH = 0;
         enum AVPixelFormat dstFormat;
-        char dstStr[12];
+        char dstStr[13];
         int dstW = 0, dstH = 0;
         int flags;
         int ret;
-- 
2.21.0