[FFmpeg-devel] avformat/hlsenc: fix CID 1405135

CID: 1405135
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

> On Apr 25, 2017, at 18:47, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> CID: 1405135

I have no idea what this message is supposed to mean.

> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
> libavformat/hlsenc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 3ec0f330fb..b7aafb73da 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -394,11 +394,11 @@ static int do_encrypt(AVFormatContext *s)
>     av_strlcat(hls->key_basename, ".key", len);
> 
>     if (hls->key_url) {
> -        strncpy(hls->key_file, hls->key_url, sizeof(hls->key_file));
> -        strncpy(hls->key_uri, hls->key_url, sizeof(hls->key_uri));
> +        av_strlcpy(hls->key_file, hls->key_url, strlen(hls->key_url));
> +        av_strlcpy(hls->key_uri, hls->key_url, strlen(hls->key_url));

Changing this to av_strlcpy makes sense, but using strlen() here replaces a DoS vulnerability with a memory-corruption one. Use sizeof() like the original code in all 4 cases.

>     } else {
> -        strncpy(hls->key_file, hls->key_basename, sizeof(hls->key_file));
> -        strncpy(hls->key_uri, hls->key_basename, sizeof(hls->key_uri));
> +        av_strlcpy(hls->key_file, hls->key_basename, strlen(hls->key_basename));
> +        av_strlcpy(hls->key_uri, hls->key_basename, strlen(hls->key_basename));
>     }
> 
>     if (!*hls->iv_string) {
> -- 
> 2.11.0 (Apple Git-81)
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

On 4/25/2017 4:47 PM, Steven Liu wrote:
> CID: 1405135
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 3ec0f330fb..b7aafb73da 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -394,11 +394,11 @@ static int do_encrypt(AVFormatContext *s)
>      av_strlcat(hls->key_basename, ".key", len);
>
>      if (hls->key_url) {
> -        strncpy(hls->key_file, hls->key_url, sizeof(hls->key_file));
> -        strncpy(hls->key_uri, hls->key_url, sizeof(hls->key_uri));
> +        av_strlcpy(hls->key_file, hls->key_url, strlen(hls->key_url));
> +        av_strlcpy(hls->key_uri, hls->key_url, strlen(hls->key_url));
>      } else {
> -        strncpy(hls->key_file, hls->key_basename, sizeof(hls->key_file));
> -        strncpy(hls->key_uri, hls->key_basename, sizeof(hls->key_uri));
> +        av_strlcpy(hls->key_file, hls->key_basename, strlen(hls->key_basename));
> +        av_strlcpy(hls->key_uri, hls->key_basename, strlen(hls->key_basename));

If either hls->key_url or hls->key_basename, depending on which path the 
code takes, is longer in length than key_file or key_uri (both of size 
LINE_BUFFER_SIZE + 1), then the calls to av_strlcpy() will write past 
the end of the buffers.  As key_url corresponds to a user-passed option, 
and I think the same may be true for key_basename based on the way it is 
formed, this in theory could be a problem, although it is unlikely in 
practice since LINE_BUFFER_SIZE is 1024.  While ideally there would be a 
sanity check somewhere in the code to make sure those URLs aren't too 
long, an alternative is to copy up to max LINE_BUFFER_SIZE characters 
and make sure the string is null-terminated in that case.

Aaron Levinson