[FFmpeg-devel] [PATCH] Ignore expired cookies

Nicolas George george at nsup.org
Sun Feb 12 13:28:21 EET 2017


Thanks for the patch. See remarks below.

Le tridi 23 pluviôse, an CCXXV, Micah Galizia a écrit :
> On some authenticated Neulion streams, they send a cookie from the past,
> like so:
> 
> Set-Cookie: nlqptid=""; Domain=.neulion.com; Expires=Thu, 01-Jan-1970
> 00:00:10 GMT; Path=/
> 
> As a result, the good cookie value is overwritten and authentication breaks
> immediately. I realise disqualifying a cookie over the date might open a
> can of worms when it comes to date formatting used by different systems,

In principle, I would be in favour of correctly heeding all elements of
the protocol, and only on top of that provide users the option of
ignoring certain elements. Parsing the expires field certainly enters
this frame.

> but I added Neulions wrong format and the http standard format.

In that case, maybe ignoring the whole cookie because the date is
invalid would be a better idea.

Did you test to see how real browsers handle it?

> Please let me know if this is acceptable. I've run it against fate and
> there were no problems.

That is good practice. Unfortunately, FATE tests almost nothing about
network protocols. If you did not, you need to check with a few web
servers that currently work.


Now reviewing details of the latest version of the patch:

> diff --git a/libavformat/http.c b/libavformat/http.c
> index 944a6cf..24368aa 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -682,12 +682,46 @@ static int parse_icy(HTTPContext *s, const char *tag, const char *p)
>  
>  static int parse_cookie(HTTPContext *s, const char *p, AVDictionary **cookies)
>  {
> -    char *eql, *name;
> +    char *eql, *name, *expiry;
>  
>      // duplicate the cookie name (dict will dupe the value)
>      if (!(eql = strchr(p, '='))) return AVERROR(EINVAL);
>      if (!(name = av_strndup(p, eql - p))) return AVERROR(ENOMEM);
>  
> +    // ensure the expiry is sane

> +    if ((expiry = strstr(eql, "Expires="))) {

I believe this is not correct. First, AFAIK, all the structure elements
in HTTP are case insensitive. Second, the string "Expires=" could
legitimately be part of the cookie value itself.

I suggest to parse the subfields of the set-cookie field correctly: it
is a sequence of "key=value" pairs (with the "=value" part optional, but
can be quoted) separated by semicolons and optional spaces. Furthermore,
it would be useful for other fields too, such as content-type.

I realize it is a little bit more work, but I think it is reasonable.

> +        struct tm tm_buf = {0};
> +        char *end;
> +
> +        // get the start & the end of the expiry ('11 Feb 2017 09:41:35 GMT')
> +        // this skips past the day of the week by finding the space following it

> +        expiry += 8 * sizeof(char);

sizeof(char) = 1 by definition, thus writing it is always unnecessary,
and rarely useful for readability.

> +        while (*expiry != ' ') expiry++;

I suspect tabs and other whitespace-style characters could be present
here.

More importantly, this code will happily skip the end-of-string marker
if there are no spaces at all, and that is a big no.

Also, style nit for here and other places: the code base does not
usually use single-line loops or conditions.

> +        expiry++;
> +        end = expiry+1;

> +        while (*end != ';') end++;

Same problem here.

> +
> +        // ensure the time is parsable
> +        end = strptime(expiry, "%d %b %Y %H:%M:%S %Z", &tm_buf);

strptime() is an XSI extension, and as such not portable enough for the
FFmpeg code base. You can look if av_small_strptime() can do the trick.

According to
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date
(which is more readable than
https://tools.ietf.org/html/rfc6265#section-5.1.1)
the correct date syntax starts with the abbreviated day name and a
comma, i.e. "%a,". I suspect the code that skips until a space above is
there to skip that component, but it should be part of the date parsing
itself.

Also, %Z is a non-standard GNU and BSD extension, not supported by
av_small_strptime(). Fortunately, still according to MDN, only "GMT" is
supported here.

> +
> +        // ensure neulion's different format is parsable
> +        if (!end) end = strptime(expiry, "%d-%b-%Y %H:%M:%S %Z", &tm_buf);
> +
> +        // if the expire is specified but unparsable, this cookie is invalid
> +        if (!end) {
> +            av_log(s, AV_LOG_ERROR, "Unable to parse expiry for cookie '%s'\n", p);
> +            av_free(name);
> +            return AVERROR(EINVAL);
> +        }
> +
> +        // no cookies from the past (neulion)

> +        if (mktime(&tm_buf) < time(NULL)) {

Since only GMT is supported, I think you need to use av_timegm().

> +            av_log(s, AV_LOG_ERROR, "Ignoring cookie from the past '%s'\n", p);
> +            av_free(name);
> +            return AVERROR(EINVAL);

I think it does not need to be an error, but that depends on the general
policy to deal with this kind of cases.

> +        }
> +    }
> +
>      // add the cookie to the dictionary
>      av_dict_set(cookies, name, eql, AV_DICT_DONT_STRDUP_KEY);
>  

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170212/dd47eedc/attachment.sig>


More information about the ffmpeg-devel mailing list