[FFmpeg-devel,v3] Fix sdp size check on fmtp integer parameters

Submitted by Olivier Maignial on April 19, 2019, 1 p.m.

Details

Message ID 1555678810-859-1-git-send-email-olivier.maignial@smile.fr
State New
Headers show

Commit Message

Olivier Maignial April 19, 2019, 1 p.m.
RFC-4566 do not give any limit of size on interger parameters given in fmtp line.
By reading some more RFCs it is possible to find examples where some integers parameters are greater than 32 (see RFC-6416, 7.4)

Instead I propose to check just check the eventual integer overflow.
Using INT_MIN and INT_MAX ensure that it will work whatever the size of int given by compiler

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

Changes v2 -> v3:
    Fix over/underflow checking in case of sizeof(int) == sizeof(long)

If MAX/MIN_INT == MAX/MIN_LONG overflow would not be detected by just checking value is in range [MAX_INT,MIN_INT].

In case of over/underflow strtol return MAX/MIN_LONG and set errno to ERANGE.
As MAX/MIN_LONG are valid values, the only way to detect over/underflow is to check errno.

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

Comments

Olivier Maignial May 9, 2019, 4:16 p.m.
Hello,
Just a reminder about this patch :)

On Fri, Apr 19, 2019 at 3:00 PM Olivier Maignial <olivier.maignial@smile.fr>
wrote:

> RFC-4566 do not give any limit of size on interger parameters given in
> fmtp line.
> By reading some more RFCs it is possible to find examples where some
> integers parameters are greater than 32 (see RFC-6416, 7.4)
>
> Instead I propose to check just check the eventual integer overflow.
> Using INT_MIN and INT_MAX ensure that it will work whatever the size of
> int given by compiler
>
> Signed-off-by: Olivier Maignial <olivier.maignial@smile.fr>
> ---
>
> Changes v2 -> v3:
>     Fix over/underflow checking in case of sizeof(int) == sizeof(long)
>
> If MAX/MIN_INT == MAX/MIN_LONG overflow would not be detected by just
> checking value is in range [MAX_INT,MIN_INT].
>
> In case of over/underflow strtol return MAX/MIN_LONG and set errno to
> ERANGE.
> As MAX/MIN_LONG are valid values, the only way to detect over/underflow is
> to check errno.
>
>  libavformat/rtpdec_mpeg4.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
> index 4f70599..d40cb5a 100644
> --- a/libavformat/rtpdec_mpeg4.c
> +++ b/libavformat/rtpdec_mpeg4.c
> @@ -289,15 +289,33 @@ 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;
> +                    errno = 0;
> +                    long int val = strtol(value, &end_ptr, 10);
> +                    if (value[0] == '\n' || end_ptr[0] != '\0') {
>                          av_log(s, AV_LOG_ERROR,
> -                               "The %s field size is invalid (%d)\n",
> -                               attr, val);
> +                               "The %s field value is not a number
> (%s)\n",
> +                               attr, value);
>                          return AVERROR_INVALIDDATA;
>                      }
> +                    if ((val == LONG_MAX && errno == ERANGE) ||
> +                        val > INT_MAX) {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s overflow maximum
> integer value.\n",
> +                               attr);
> +                        return AVERROR_INVALIDDATA;
> +                    }
> +                    if ((val == LONG_MIN && errno == ERANGE) ||
> +                        val < INT_MIN)
> +                    {
> +                        av_log(s, AV_LOG_ERROR,
> +                               "Value of field %s underflow minimum
> integer value.\n",
> +                               attr);
> +                        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
>
>

Patch hide | download patch | download mbox

diff --git a/libavformat/rtpdec_mpeg4.c b/libavformat/rtpdec_mpeg4.c
index 4f70599..d40cb5a 100644
--- a/libavformat/rtpdec_mpeg4.c
+++ b/libavformat/rtpdec_mpeg4.c
@@ -289,15 +289,33 @@  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;
+                    errno = 0;
+                    long int val = strtol(value, &end_ptr, 10);
+                    if (value[0] == '\n' || end_ptr[0] != '\0') {
                         av_log(s, AV_LOG_ERROR,
-                               "The %s field size is invalid (%d)\n",
-                               attr, val);
+                               "The %s field value is not a number (%s)\n",
+                               attr, value);
                         return AVERROR_INVALIDDATA;
                     }
+                    if ((val == LONG_MAX && errno == ERANGE) ||
+                        val > INT_MAX) {
+                        av_log(s, AV_LOG_ERROR,
+                               "Value of field %s overflow maximum integer value.\n",
+                               attr);
+                        return AVERROR_INVALIDDATA;
+                    }
+                    if ((val == LONG_MIN && errno == ERANGE) ||
+                        val < INT_MIN)
+                    {
+                        av_log(s, AV_LOG_ERROR,
+                               "Value of field %s underflow minimum integer value.\n",
+                               attr);
+                        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)