mmsh: fixed printf injection bug in mmsh request

Message ID 1308211200-75278-1-git-send-email-martin@martin.st
State Committed
Commit 6095388812ce1b2a95e9917b89e5857639208f88
Headers show

Commit Message

Martin Storsjö June 16, 2011, 8 a.m.
From: Kirill Zorin <cyril.zorin@gmail.com>

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 libavformat/mmsh.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Ronald Bultje June 16, 2011, 12:04 p.m. | #1
Hi,

On Thu, Jun 16, 2011 at 4:00 AM, Martin Storsjö <martin@martin.st> wrote:
> From: Kirill Zorin <cyril.zorin@gmail.com>
>
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  libavformat/mmsh.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/libavformat/mmsh.c b/libavformat/mmsh.c
> index b19973e..af040e2 100644
> --- a/libavformat/mmsh.c
> +++ b/libavformat/mmsh.c
> @@ -231,7 +231,7 @@ static int mmsh_open(URLContext *h, const char *uri, int flags)
>         host, sizeof(host), &port, path, sizeof(path), location);
>     if (port<0)
>         port = 80; // default mmsh protocol port
> -    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, path);
> +    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, "%s", path);

OK, nice catch.

For the future, do we need to make the API more resilient to this by
e.g. having two functions, one w/o printf-style formatting and one
with?

Ronald
Martin Storsjö June 16, 2011, 2:43 p.m. | #2
On Thu, 16 Jun 2011, Ronald S. Bultje wrote:

> On Thu, Jun 16, 2011 at 4:00 AM, Martin Storsjö <martin@martin.st> wrote:
> > From: Kirill Zorin <cyril.zorin@gmail.com>
> >
> > Signed-off-by: Martin Storsjö <martin@martin.st>
> > ---
> >  libavformat/mmsh.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/libavformat/mmsh.c b/libavformat/mmsh.c
> > index b19973e..af040e2 100644
> > --- a/libavformat/mmsh.c
> > +++ b/libavformat/mmsh.c
> > @@ -231,7 +231,7 @@ static int mmsh_open(URLContext *h, const char *uri, int flags)
> >         host, sizeof(host), &port, path, sizeof(path), location);
> >     if (port<0)
> >         port = 80; // default mmsh protocol port
> > -    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, path);
> > +    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, "%s", path);
> 
> OK, nice catch.

Ok'd for push during freeze by Reinhard (together with the one for rtsp), 
and pushed.

> For the future, do we need to make the API more resilient to this by
> e.g. having two functions, one w/o printf-style formatting and one
> with?

I think that's overkill - instead we could add gcc specific attributes to 
allow the compiler to check for these kinds of issues, we do that for 
av_log already.

// Martin
Ronald Bultje June 16, 2011, 2:51 p.m. | #3
Hi,

On Thu, Jun 16, 2011 at 10:43 AM, Martin Storsjö <martin@martin.st> wrote:
> On Thu, 16 Jun 2011, Ronald S. Bultje wrote:
>> On Thu, Jun 16, 2011 at 4:00 AM, Martin Storsjö <martin@martin.st> wrote:
>> > From: Kirill Zorin <cyril.zorin@gmail.com>
>> >
>> > Signed-off-by: Martin Storsjö <martin@martin.st>
>> > ---
>> >  libavformat/mmsh.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/libavformat/mmsh.c b/libavformat/mmsh.c
>> > index b19973e..af040e2 100644
>> > --- a/libavformat/mmsh.c
>> > +++ b/libavformat/mmsh.c
>> > @@ -231,7 +231,7 @@ static int mmsh_open(URLContext *h, const char *uri, int flags)
>> >         host, sizeof(host), &port, path, sizeof(path), location);
>> >     if (port<0)
>> >         port = 80; // default mmsh protocol port
>> > -    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, path);
>> > +    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, "%s", path);
>>
>> OK, nice catch.
>
> Ok'd for push during freeze by Reinhard (together with the one for rtsp),
> and pushed.
>
>> For the future, do we need to make the API more resilient to this by
>> e.g. having two functions, one w/o printf-style formatting and one
>> with?
>
> I think that's overkill - instead we could add gcc specific attributes to
> allow the compiler to check for these kinds of issues, we do that for
> av_log already.

That sounds good enough to me.

Ronald

Patch

diff --git a/libavformat/mmsh.c b/libavformat/mmsh.c
index b19973e..af040e2 100644
--- a/libavformat/mmsh.c
+++ b/libavformat/mmsh.c
@@ -231,7 +231,7 @@  static int mmsh_open(URLContext *h, const char *uri, int flags)
         host, sizeof(host), &port, path, sizeof(path), location);
     if (port<0)
         port = 80; // default mmsh protocol port
-    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, path);
+    ff_url_join(httpname, sizeof(httpname), "http", NULL, host, port, "%s", path);
 
     if (ffurl_alloc(&mms->mms_hd, httpname, AVIO_FLAG_READ) < 0) {
         return AVERROR(EIO);