diff mbox

[FFmpeg-devel,v6] Fix integer parameters size check in SDP fmtp line

Message ID 1562913640-17395-1-git-send-email-olivier.maignial@smile.fr
State Superseded
Headers show

Commit Message

Olivier Maignial July 12, 2019, 6:40 a.m. UTC

Comments

Reimar Döffinger July 15, 2019, 7:10 p.m. UTC | #1 
This seems reasonable to me, but I have not been involved in any rtp code.
The commit message is maybe a bit on the verbose side, but I don't have any strong opinion.

On 12.07.2019, at 08:40, Olivier Maignial <olivier.maignial@smile.fr> wrote:

> === PROBLEM ===
> 
> I was trying to record h264 + aac streams from an RTSP server to mp4 file. using this command line:
>    ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a aac_adtstoasc test.mp4
> 
> FFmpeg then fail to record audio and output this logs:
>    [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>    [rtsp @ 0xcda1f0] Error parsing AU headers
>    ...
>    [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio: aac, 48000 Hz, 1 channels): unspecified sample format
> 
> In SDP provided by my RTSP server I had this fmtp line:
>    a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
> 
> In FFmpeg code, I found a check introduced by commit 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than 32 for fmtp line parameters.
> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams) give examples of "profile-level-id" values for AAC, up to 55.
> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any limit of size on interger parameters given in fmtp line.
> 
> === FIX ===
> 
> Instead of prohibit values over 32, I propose to check the possible integer overflow.
> The use of strtoll allow to check the string validity and the possible overflow.
> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX ensure to have the same behavior on 32 or 64 bits platforms.
> 
> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> It has passed the full fate tests suite sucessfully.
> 
> Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> ---
> 
> Changes V6 --> V7:
>    - Use long long int and strtoll. LLONG_MAX is always greather than INT32_MIN while LONG_MAX can be equal to INT32_MAX. It allows to accept full INT32 range.
>    - Avoid to use errno
> 
> libavformat/rtpdec_mpeg4.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..f1cbedf 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,18 @@ static int parse_fmtp(AVFormatContext *s,
>         for (i = 0; attr_names[i].str; ++i) {
>             if (!av_strcasecmp(attr, attr_names[i].str)) {
>                 if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> -                    int val = atoi(value);
> -                    if (val > 32) {
> +                    char *end_ptr = NULL;
> +                    long long int val = strtoll(value, &end_ptr, 10);
> +                    if (end_ptr == value || end_ptr[0] != '\0' ||
> +                        val < INT32_MIN || val > INT32_MAX) {
>                         av_log(s, AV_LOG_ERROR,
> -                               "The %s field size is invalid (%d)\n",
> -                               attr, val);
> +                               "The %s field value is not a valid number, or overflows int32: %s\n",
> +                               attr, value);
>                         return AVERROR_INVALIDDATA;
>                     }
> +
>                     *(int *)((char *)data+
> -                        attr_names[i].offset) = val;
> +                        attr_names[i].offset) = (int) val;
>                 } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) {
>                     char *val = av_strdup(value);
>                     if (!val)
> -- 
> 2.7.4
> 
> _______________________________________________
> 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".
Michael Niedermayer July 17, 2019, 8:28 p.m. UTC | #2 
On Fri, Jul 12, 2019 at 08:40:40AM +0200, Olivier Maignial wrote:
> === PROBLEM ===
> 
> I was trying to record h264 + aac streams from an RTSP server to mp4 file. using this command line:
>     ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a aac_adtstoasc test.mp4
> 
> FFmpeg then fail to record audio and output this logs:
>     [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
>     [rtsp @ 0xcda1f0] Error parsing AU headers
>     ...
>     [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio: aac, 48000 Hz, 1 channels): unspecified sample format
> 
> In SDP provided by my RTSP server I had this fmtp line:
>     a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
> 
> In FFmpeg code, I found a check introduced by commit 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than 32 for fmtp line parameters.
> However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams) give examples of "profile-level-id" values for AAC, up to 55.
> Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any limit of size on interger parameters given in fmtp line.
> 
> === FIX ===
> 
> Instead of prohibit values over 32, I propose to check the possible integer overflow.
> The use of strtoll allow to check the string validity and the possible overflow.
> Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX ensure to have the same behavior on 32 or 64 bits platforms.
> 
> This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> It has passed the full fate tests suite sucessfully.
> 
> Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> ---
> 
> Changes V6 --> V7:
>     - Use long long int and strtoll. LLONG_MAX is always greather than INT32_MIN while LONG_MAX can be equal to INT32_MAX. It allows to accept full INT32 range.
>     - Avoid to use errno
> 
>  libavformat/rtpdec_mpeg4.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..f1cbedf 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,18 @@ static int parse_fmtp(AVFormatContext *s,
>          for (i = 0; attr_names[i].str; ++i) {
>              if (!av_strcasecmp(attr, attr_names[i].str)) {
>                  if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> -                    int val = atoi(value);
> -                    if (val > 32) {
> +                    char *end_ptr = NULL;
> +                    long long int val = strtoll(value, &end_ptr, 10);
> +                    if (end_ptr == value || end_ptr[0] != '\0' ||
> +                        val < INT32_MIN || val > INT32_MAX) {
>                          av_log(s, AV_LOG_ERROR,
> -                               "The %s field size is invalid (%d)\n",
> -                               attr, val);
> +                               "The %s field value is not a valid number, or overflows int32: %s\n",
> +                               attr, value);
>                          return AVERROR_INVALIDDATA;
>                      }
> +
>                      *(int *)((char *)data+
> -                        attr_names[i].offset) = val;
> +                        attr_names[i].offset) = (int) val;

Looking at this a bit deeper, this reads several specific values
for example sizelength which is then used to read with get_bits_long()
that is limited to 32bit. Maybe iam missing some check but if not
that is adding a bug.
I did not check what the valid range of the other cases is but
they may have similar problems

[...]
Olivier Maignial July 22, 2019, 7:58 a.m. UTC | #3 
You are right, some values are expected to be <= 32.
However, it doesn't add a bug: Without my patch fail will occur at parsing
time; With my patch it will occur a bit later on check performed by
get_bits_long().
Maybe we can add a specific max value to each parameter to make logs more
accurate on what happens.

On Wed, Jul 17, 2019 at 10:28 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Jul 12, 2019 at 08:40:40AM +0200, Olivier Maignial wrote:
> > === PROBLEM ===
> >
> > I was trying to record h264 + aac streams from an RTSP server to mp4
> file. using this command line:
> >     ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy
> -bsf:a aac_adtstoasc test.mp4
> >
> > FFmpeg then fail to record audio and output this logs:
> >     [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
> >     [rtsp @ 0xcda1f0] Error parsing AU headers
> >     ...
> >     [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1
> (Audio: aac, 48000 Hz, 1 channels): unspecified sample format
> >
> > In SDP provided by my RTSP server I had this fmtp line:
> >     a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr;
> config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;
> >
> > In FFmpeg code, I found a check introduced by commit
> 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than
> 32 for fmtp line parameters.
> > However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual
> Streams) give examples of "profile-level-id" values for AAC, up to 55.
> > Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give
> any limit of size on interger parameters given in fmtp line.
> >
> > === FIX ===
> >
> > Instead of prohibit values over 32, I propose to check the possible
> integer overflow.
> > The use of strtoll allow to check the string validity and the possible
> overflow.
> > Value is then checked against INT32_MIN and INT32_MAX. Using
> INT32_MIN/MAX ensure to have the same behavior on 32 or 64 bits platforms.
> >
> > This patch fix my problem and I now can record my RTSP AAC stream to mp4.
> > It has passed the full fate tests suite sucessfully.
> >
> > Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> > ---
> >
> > Changes V6 --> V7:
> >     - Use long long int and strtoll. LLONG_MAX is always greather than
> INT32_MIN while LONG_MAX can be equal to INT32_MAX. It allows to accept
> full INT32 range.
> >     - Avoid to use errno
> >
> >  libavformat/rtpdec_mpeg4.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> > index 4f70599..f1cbedf 100644
> > --- a/libavformat/rtpdec_mpeg4.c
> > +++ b/libavformat/rtpdec_mpeg4.c
> > @@ -289,15 +289,18 @@ static int parse_fmtp(AVFormatContext *s,
> >          for (i = 0; attr_names[i].str; ++i) {
> >              if (!av_strcasecmp(attr, attr_names[i].str)) {
> >                  if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
> > -                    int val = atoi(value);
> > -                    if (val > 32) {
> > +                    char *end_ptr = NULL;
> > +                    long long int val = strtoll(value, &end_ptr, 10);
> > +                    if (end_ptr == value || end_ptr[0] != '\0' ||
> > +                        val < INT32_MIN || val > INT32_MAX) {
> >                          av_log(s, AV_LOG_ERROR,
> > -                               "The %s field size is invalid (%d)\n",
> > -                               attr, val);
> > +                               "The %s field value is not a valid
> number, or overflows int32: %s\n",
> > +                               attr, value);
> >                          return AVERROR_INVALIDDATA;
> >                      }
> > +
> >                      *(int *)((char *)data+
> > -                        attr_names[i].offset) = val;
> > +                        attr_names[i].offset) = (int) val;
>
> Looking at this a bit deeper, this reads several specific values
> for example sizelength which is then used to read with get_bits_long()
> that is limited to 32bit. Maybe iam missing some check but if not
> that is adding a bug.
> I did not check what the valid range of the other cases is but
> they may have similar problems
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
> _______________________________________________
> 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".
Michael Niedermayer July 22, 2019, 11:12 p.m. UTC | #4 
On Mon, Jul 22, 2019 at 09:58:58AM +0200, Olivier MAIGNIAL wrote:
> You are right, some values are expected to be <= 32.
> However, it doesn't add a bug: Without my patch fail will occur at parsing
> time; With my patch it will occur a bit later on check performed by
> get_bits_long().

get_bits_long has an assert on the size, so if its out of range its a bug

Thanks

[...]
diff mbox

Patch

=== PROBLEM ===

I was trying to record h264 + aac streams from an RTSP server to mp4 file. using this command line:
    ffmpeg -v verbose -y -i "rtsp://<ip>/my_resources" -codec copy -bsf:a aac_adtstoasc test.mp4

FFmpeg then fail to record audio and output this logs:
    [rtsp @ 0xcda1f0] The profile-level-id field size is invalid (40)
    [rtsp @ 0xcda1f0] Error parsing AU headers
    ...
    [rtsp @ 0xcda1f0] Could not find codec parameters for stream 1 (Audio: aac, 48000 Hz, 1 channels): unspecified sample format

In SDP provided by my RTSP server I had this fmtp line:
    a=fmtp:98 streamType=5; profile-level-id=40; mode=AAC-hbr; config=1188; sizeLength=13; indexLength=3; indexDeltaLength=3;

In FFmpeg code, I found a check introduced by commit 24130234cd9dd733116d17b724ea4c8e12ce097a. It disallows values greater than 32 for fmtp line parameters.
However, In RFC-6416 (RTP Payload Format for MPEG-4 Audio/Visual Streams) give examples of "profile-level-id" values for AAC, up to 55.
Furthermore, RFC-4566 (SDP: Session Description Protocol) do not give any limit of size on interger parameters given in fmtp line.

=== FIX ===

Instead of prohibit values over 32, I propose to check the possible integer overflow.
The use of strtoll allow to check the string validity and the possible overflow.
Value is then checked against INT32_MIN and INT32_MAX. Using INT32_MIN/MAX ensure to have the same behavior on 32 or 64 bits platforms.

This patch fix my problem and I now can record my RTSP AAC stream to mp4.
It has passed the full fate tests suite sucessfully.

Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
---

Changes V6 --> V7:
    - Use long long int and strtoll. LLONG_MAX is always greather than INT32_MIN while LONG_MAX can be equal to INT32_MAX. It allows to accept full INT32 range.
    - Avoid to use errno

 libavformat/rtpdec_mpeg4.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
index 4f70599..f1cbedf 100644
--- a/libavformat/rtpdec_mpeg4.c
+++ b/libavformat/rtpdec_mpeg4.c
@@ -289,15 +289,18 @@  static int parse_fmtp(AVFormatContext *s,
         for (i = 0; attr_names[i].str; ++i) {
             if (!av_strcasecmp(attr, attr_names[i].str)) {
                 if (attr_names[i].type == ATTR_NAME_TYPE_INT) {
-                    int val = atoi(value);
-                    if (val > 32) {
+                    char *end_ptr = NULL;
+                    long long int val = strtoll(value, &end_ptr, 10);
+                    if (end_ptr == value || end_ptr[0] != '\0' ||
+                        val < INT32_MIN || val > INT32_MAX) {
                         av_log(s, AV_LOG_ERROR,
-                               "The %s field size is invalid (%d)\n",
-                               attr, val);
+                               "The %s field value is not a valid number, or overflows int32: %s\n",
+                               attr, value);
                         return AVERROR_INVALIDDATA;
                     }
+
                     *(int *)((char *)data+
-                        attr_names[i].offset) = val;
+                        attr_names[i].offset) = (int) val;
                 } else if (attr_names[i].type == ATTR_NAME_TYPE_STR) {
                     char *val = av_strdup(value);
                     if (!val)