[FFmpeg-devel] [PATCH] avformat/libsrt: Decode URL parameter strings

[FFmpeg-devel] [PATCH] avformat/libsrt: Decode URL parameter strings

[Armin Hasitzka]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

Hi again,

we found this the other day when using a stream ID containing "%2F",
expecting this to be resolved to "/". While "%2F" could obviously be sent
decoded, "&" (decoded) would currently end the value and not be used, "+"
(decoded) would be overwritten with " ", and "=" (decoded) could cause
entirely unexpected outcomes (albeit all these characters being allowed by
SRT for various string inputs).

As for changing `av_strndup` to `ff_urldecode` (which removes a length
check); I don't think that this particular length check added any
protection (`av_find_info_tag` adds a trailing `\0` if found). This
thinking is also evident at the two other places where `ff_urldecode`
replaced `av_strdup`.

It would be amazing if we could get this merged upstream :)

Best
Armin

[-- Attachment #2: 0001-avformat-libsrt-Decode-URL-parameter-strings.patch --]
[-- Type: application/octet-stream, Size: 1920 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/libsrt: Decode URL parameter strings

["zhilizhao(赵志立)"]

> On Aug 11, 2023, at 00:13, Armin Hasitzka <armin@grabyo.com> wrote:
> 
> Hi again,
> 
> we found this the other day when using a stream ID containing "%2F",
> expecting this to be resolved to "/". While "%2F" could obviously be sent
> decoded, "&" (decoded) would currently end the value and not be used, "+"
> (decoded) would be overwritten with " ", and "=" (decoded) could cause
> entirely unexpected outcomes (albeit all these characters being allowed by
> SRT for various string inputs).
> 
> As for changing `av_strndup` to `ff_urldecode` (which removes a length
> check); I don't think that this particular length check added any
> protection (`av_find_info_tag` adds a trailing `\0` if found). This
> thinking is also evident at the two other places where `ff_urldecode`
> replaced `av_strdup`.
> 
> It would be amazing if we could get this merged upstream :)
> 

Thanks for the contribution, but the issue is kind of complex.

1. The format of streamid isn’t take URL into consideration

"#!::u=admin,r=foo”

2. Obviously some implementation and usecases depend on these fields to
be passed directly without URL encoding/decoding.

The same issue has been filed to srt again and again, ref.

https://github.com/Haivision/srt/issues/2749

and #1871, #1173, #2015.

I think we shouldn’t put these fields into URL at the first place.
Now doing any change, even if it’s technically correct, will make
regression issues.

I have no idea to improve the code. But it’s easy to solve from the
user’s point of view: don’t set these fields via URL, just use options.
These is no implementation dependent behavior with options.

> ---
>  libavformat/libsrt.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
> index cd8f5b1e7d..8986616334 100644
> --- a/libavformat/libsrt.c
> +++ b/libavformat/libsrt.c
> @@ -32,6 +32,7 @@
>  #include "network.h"
>  #include "os_support.h"
>  #include "url.h"
> +#include "urldecode.h"
>  
>  /* This is for MPEG-TS and it's a default SRTO_PAYLOADSIZE for SRTT_LIVE (8 TS packets) */
>  #ifndef SRT_LIVE_DEFAULT_PAYLOAD_SIZE
> @@ -547,7 +548,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "passphrase", p)) {
>              av_freep(&s->passphrase);
> -            s->passphrase = av_strndup(buf, strlen(buf));
> +            s->passphrase = ff_urldecode(buf, 0);
>          }
>  #if SRT_VERSION_VALUE >= 0x010302
>          if (av_find_info_tag(buf, sizeof(buf), "enforced_encryption", p)) {
> @@ -632,7 +633,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "streamid", p)) {
>              av_freep(&s->streamid);
> -            s->streamid = av_strdup(buf);
> +            s->streamid = ff_urldecode(buf, 0);
>              if (!s->streamid) {
>                  ret = AVERROR(ENOMEM);
>                  goto err;
> @@ -640,7 +641,7 @@ static int libsrt_open(URLContext *h, const char *uri, int flags)
>          }
>          if (av_find_info_tag(buf, sizeof(buf), "smoother", p)) {
>              av_freep(&s->smoother);
> -            s->smoother = av_strdup(buf);
> +            s->smoother = ff_urldecode(buf, 0);
>              if(!s->smoother) {
>                  ret = AVERROR(ENOMEM);
>                  goto err;
> -- 
> 2.41.0
> 
> 


_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avformat/libsrt: Decode URL parameter strings

[Armin Hasitzka]

I think accepting URL params in first place isn't inherently bad (depending
on the use case, it might make certain interactions easier).

I expected regressions to be brought up and just want to point out that,
while absolutely possible, actual regressions will likely be fairly rare:


   - any "unencoded" characters will continue to work (eg "/" passed in
   will continue to be "/")
   - usage of "+" and "&" will cause unintended behaviour already ("="
   _might_ cause unintended behaviour today)
   - the main regression comes from "%.." substrings that are _not_ meant
   to be url URL encoded but happen to be that and will now be turned into
   something else


Because of this, I did not expect this patch to make it into any patch or
minor release. We were hoping though, that this would be a contender for
the next major one.

On Fri, 11 Aug 2023 at 03:55, "zhilizhao(赵志立)" <quinkblack@foxmail.com>
wrote:

>
>
> > On Aug 11, 2023, at 00:13, Armin Hasitzka <armin@grabyo.com> wrote:
> >
> > Hi again,
> >
> > we found this the other day when using a stream ID containing "%2F",
> > expecting this to be resolved to "/". While "%2F" could obviously be sent
> > decoded, "&" (decoded) would currently end the value and not be used, "+"
> > (decoded) would be overwritten with " ", and "=" (decoded) could cause
> > entirely unexpected outcomes (albeit all these characters being allowed
> by
> > SRT for various string inputs).
> >
> > As for changing `av_strndup` to `ff_urldecode` (which removes a length
> > check); I don't think that this particular length check added any
> > protection (`av_find_info_tag` adds a trailing `\0` if found). This
> > thinking is also evident at the two other places where `ff_urldecode`
> > replaced `av_strdup`.
> >
> > It would be amazing if we could get this merged upstream :)
> >
>
> Thanks for the contribution, but the issue is kind of complex.
>
> 1. The format of streamid isn’t take URL into consideration
>
> "#!::u=admin,r=foo”
>
> 2. Obviously some implementation and usecases depend on these fields to
> be passed directly without URL encoding/decoding.
>
> The same issue has been filed to srt again and again, ref.
>
> https://github.com/Haivision/srt/issues/2749
>
> and #1871, #1173, #2015.
>
> I think we shouldn’t put these fields into URL at the first place.
> Now doing any change, even if it’s technically correct, will make
> regression issues.
>
> I have no idea to improve the code. But it’s easy to solve from the
> user’s point of view: don’t set these fields via URL, just use options.
> These is no implementation dependent behavior with options.
>
> > ---
> >  libavformat/libsrt.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/libsrt.c b/libavformat/libsrt.c
> > index cd8f5b1e7d..8986616334 100644
> > --- a/libavformat/libsrt.c
> > +++ b/libavformat/libsrt.c
> > @@ -32,6 +32,7 @@
> >  #include "network.h"
> >  #include "os_support.h"
> >  #include "url.h"
> > +#include "urldecode.h"
> >
> >  /* This is for MPEG-TS and it's a default SRTO_PAYLOADSIZE for
> SRTT_LIVE (8 TS packets) */
> >  #ifndef SRT_LIVE_DEFAULT_PAYLOAD_SIZE
> > @@ -547,7 +548,7 @@ static int libsrt_open(URLContext *h, const char
> *uri, int flags)
> >          }
> >          if (av_find_info_tag(buf, sizeof(buf), "passphrase", p)) {
> >              av_freep(&s->passphrase);
> > -            s->passphrase = av_strndup(buf, strlen(buf));
> > +            s->passphrase = ff_urldecode(buf, 0);
> >          }
> >  #if SRT_VERSION_VALUE >= 0x010302
> >          if (av_find_info_tag(buf, sizeof(buf), "enforced_encryption",
> p)) {
> > @@ -632,7 +633,7 @@ static int libsrt_open(URLContext *h, const char
> *uri, int flags)
> >          }
> >          if (av_find_info_tag(buf, sizeof(buf), "streamid", p)) {
> >              av_freep(&s->streamid);
> > -            s->streamid = av_strdup(buf);
> > +            s->streamid = ff_urldecode(buf, 0);
> >              if (!s->streamid) {
> >                  ret = AVERROR(ENOMEM);
> >                  goto err;
> > @@ -640,7 +641,7 @@ static int libsrt_open(URLContext *h, const char
> *uri, int flags)
> >          }
> >          if (av_find_info_tag(buf, sizeof(buf), "smoother", p)) {
> >              av_freep(&s->smoother);
> > -            s->smoother = av_strdup(buf);
> > +            s->smoother = ff_urldecode(buf, 0);
> >              if(!s->smoother) {
> >                  ret = AVERROR(ENOMEM);
> >                  goto err;
> > --
> > 2.41.0
> >
> >
>
>
> _______________________________________________
> 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".
>
_______________________________________________
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".