[FFmpeg-devel] [PATCH] Ignore expired cookies

[Micah Galizia]

On Sun, Feb 12, 2017 at 6:28 AM, Nicolas George <george at nsup.org> wrote:
> 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.

Maybe a more permissive approach would be better. Iff we _can_ parse
the date format _AND_ it's expired then and only then ignore that
cookie. This would maintain compatibility with how it works now, but
correct the specific bug I'm observing.

> Did you test to see how real browsers handle it?

Browsers, no, but the Android video player, yes. They ignore the new
cookie value if it shows up with an expiry in the past (well, at least
an expiry date in 1970) and use the value it had already. Here is a
charles dump if you want to see:
https://dl.dropboxusercontent.com/u/83154919/snnow_full2.chls

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

I tested it against the neulion stream I have access to. I don't have
a whole lot else but hopefully if I minimize the situations in which
cookies are ignored it wont matter. I'll also try to find a few other
stream sources.

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

I think this was proposed last time I was adding cookies into the HLS
demuxer (years ago) but the problem is that you can't really pass
around complex data structures between demuxers (http, hls, etc).
AVOption was also quite restrictive last time I looked into doing
something more advanced like that.

One of the things Michael N. suggested back then was using a proper
cookie jar -- is there any reason I can't keep parsed cookies in an
in-memory global static variable scoped in http.c? Then we wouldn't
even need to pass the cookies as options around between demuxers
anymore since they'd have a perminant home in http.c, which should be
the only place they're used.

>> +        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.


It does not -- no %b or %Z -- I already looked...

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

The calling methods rely on that non-zero return value or they'll use
the cookies.

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

The first blocking issue for me is the limitations of
av_small_strptime() -- it doesn't support %b so I guess I'll have to
add it in a separate patch. I guess I'll look into that.

Anyway, thanks for the review -- I'm hoping someone can give me some
feedback about either the global static cookie jar or _only_
disqualifying the parsable expiry dates... I'm thinking the easier
approach over the short term would ignoring cookies with dates we
cannot parse.