Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: allow some basic HTTP headers to be passed on to ffmpeg #10402

Merged
merged 1 commit into from Sep 6, 2016

Conversation

@basrieter
Copy link
Contributor

commented Sep 4, 2016

As discussed with @FernetMenta in #9330 this is my attempt to allow basic HTTP headers to be send to ffmpeg. I also update the CurlFile.cpp to filter out all but the standard HTTP headers so it behaves the same in both cases.

av_dict_set(&options, it->first.c_str(), value.c_str(), 0);
}
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
else if (name == "accept" || name == "accept-charset" || name == "accept-encoding" || name == "accept-language" || name == "accept-datetime" ||

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

i'd block accept-charset and accept-encoding and content-length these should be managed internally by ffmpeg

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

I agree, I will change this tonight.

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

I have just removed these 3.

@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

I don't actually see why the travis-ci failed. Seems like a test failed:
396/493 Test #396: TestThreadLocal.HeapDestroyed ..........................................***Exception: SegFault 0.28 sec

But I wonder how or if I caused this?

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch 4 times, most recently from 9161b7c to b1b7b1b Sep 5, 2016
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

@basrieter seems you used tabs instead of spaces in the changes.

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from b1b7b1b to 9f01614 Sep 5, 2016
@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

Not tabs, but I used 4 space indentation instead of 2 space. Changed that now.

@@ -829,8 +829,26 @@ void CCurlFile::ParseAndCorrectUrl(CURL &url2)
m_postdata = Base64::Decode(value);
m_postdataset = true;
}
else
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
else if (name == "accept" || name == "accept-charset" || name == "accept-encoding" || name == "accept-language" || name == "accept-datetime" ||

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

accept-charset is set at line 822, same for cookie, referer and so on and maybe move "accept-encoding" to line 815

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

True, I will remove them here.

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from 9f01614 to 37e2a9e Sep 5, 2016
@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

I removed the duplicate header types in the if clauses.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

jenkins build this please

name == "multiple_requests" || name == "post_data" || name == "mime_type" ||
name == "icy" || name == "icy_metadata_headers" || name == "icy_metadata_packet" || name == "method" ||
name == "auth_type" || name == "none" || name == "basic" || name == "send_expect_100" || name == "location" ||
name == "end_offset" || name == "http_proxy" || name == "reconnect" || name == "reconnect_at_eof" || name == "reconnect_streamed" ||

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

drop proxy, this is already handled by this method

{
av_dict_set(&options, "user-agent", value.c_str(), 0);
hasUserAgent = true;
}

// these are the officially supported headers from ffmpeg
else if (name == "seekable" || name == "chunked_post" || name == "content_type" || name == "metadata" || name == "offset" ||

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

this is not what I suggested. addons are not supposed to set ffmpeg options directly because they don't influence how Kodi would open the stream. go through the list of http headers. then check it you can set the option directly by av_dict_set or you need to set it in headers.

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

Sorry, that is my bad then. Let me fix this.

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from 37e2a9e to b2430fc Sep 5, 2016
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
else if (name == "accept" || name == "accept-language" || name == "accept-datetime" ||
name == "authorization" || name == "cache-control" || name == "connection" || name == "cookie" || name == "content-md5" ||
name == "content-type" || name == "date" || name == "expect" || name == "forwarded" || name == "from" || name == "host" || name == "if-match" ||

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

example: "content-type" is set by ffmpeg option "content_type"

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

btw content-type is directly connected with post_data couse its used with POST request

name == "x-wap-profile" || name == "proxy-connection" || name == "x-uidh" || name == "x-csrf-token" || name == "x-request-id" || name == "x-correlation-id")
{
CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() adding custom header option '%s: %s'", it->first.c_str(), value.c_str());
headers.append(it->first).append(": ").append(value).append("\r\n");

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

ok, that should to. ffmpeg seems to treat headers as if it were set by option directly.

@@ -655,9 +655,26 @@ AVDictionary *CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput()
av_dict_set(&options, "user-agent", value.c_str(), 0);
hasUserAgent = true;
}
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)
else if (name == "accept" || name == "accept-language" || name == "accept-datetime" ||
name == "authorization" || name == "cache-control" || name == "connection" || name == "cookie" || name == "content-md5" ||

This comment has been minimized.

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from b2430fc to 2b78763 Sep 5, 2016
name == "authorization" || name == "cache-control" || name == "connection" || name == "content-md5" ||
name == "date" || name == "expect" || name == "forwarded" || name == "from" || name == "host" || name == "if-match" ||
name == "if-modified-since" || name == "if-none-match" || name == "if-range" || name == "if-unmodified-since" || name == "max-forwards" ||
name == "origin" || name == "pragma" || name == "proxy-authorization" || name == "range" || name == "referer" || name == "te" || name == "upgrade" ||

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

proxy-authorization is already handled

}
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields) are appended as actual headers
else if (name == "accept" || name == "accept-language" || name == "accept-datetime" ||
name == "authorization" || name == "cache-control" || name == "connection" || name == "content-md5" ||

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

authorization if used must not be logged

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

Right, I guess the host header also makes no sense to add here.

name == "max-forwards" || name == "origin" || name == "pragma" || name == "proxy-authorization" || name == "range" ||
name == "te" || name == "upgrade" || name == "via" || name == "warning" || name == "x-requested-with" || name == "dnt" || name == "x-forwarded-for" ||
name == "x-forwarded-host" || name == "x-forwarded-proto" || name == "front-end-https" || name == "x-http-method-override" || name == "x-att-deviceid" ||
name == "x-wap-profile" || name == "proxy-connection" || name == "x-uidh" || name == "x-csrf-token" || name == "x-request-id" || name == "x-correlation-id")

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

proxy-connection is same as connection, see wiki article

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from 2b78763 to 274ee8c Sep 5, 2016
@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

I cleaned the list up and re-ordered it a bit so the order in both files are the same.

@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

I also remove the host and content-length as it makes no sense to set from here. Not sure what to do with the content-type. In the CurlFile it makes sense as it also has post data. But for ffmpeg?

hasUserAgent = true;
}
else if (name == "content-type")

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

content-type makes sense only when we allow ffmpeg option post_data

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

Ok, but would that be likely to happen? A POST for ffmpeg?

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

I could add:

else if (name == "post-data")
{
  postData = Base64::Decode(value);
  av_dict_set(&options, "post_data", postData.c_str(), 0);
  CLog::Log(LOGDEBUG, "CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput() adding ffmpeg option 'post_data: %s'", postData.c_str());
}

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

who knows, i'd say that most of that http headers are really useless for us, but there will be many use-cases for exotic headers when this PR gets in

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

That why I stuck to the basic list. But, I will remove the content-type. I don't thing that ffmpeg will ever be sending POSTs.

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from 274ee8c to 155f294 Sep 5, 2016
@basrieter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

Removed the content-type for ffmpeg.

@@ -642,6 +643,7 @@ AVDictionary *CDVDDemuxFFmpeg::GetFFMpegOptionsFromInput()
std::map<std::string, std::string> protocolOptions;
url.GetProtocolOptions(protocolOptions);
std::string headers;
std::string postData;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

seems not be be used, same for include Base64.h

This comment has been minimized.

Copy link
@Sorien

Sorien Sep 5, 2016

forgot to remove and base64.h include as well

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

ah, I am going to cry ...... stupid

@basrieter basrieter force-pushed the basrieter:http_headers_for_ffmpeg branch from 155f294 to ed00cd6 Sep 5, 2016
@@ -829,8 +829,29 @@ void CCurlFile::ParseAndCorrectUrl(CURL &url2)
m_postdata = Base64::Decode(value);
m_postdataset = true;
}
else
// other standard headers (see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Sep 5, 2016

Member

@arnova do you agree to this?

This comment has been minimized.

Copy link
@basrieter

basrieter Sep 5, 2016

Author Contributor

I am actually limiting the number of headers here. It was initially just appending every other parameter:

SetRequestHeader(it->first, value);

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Sep 6, 2016

jenkins build this please

@mgonzales71

This comment has been minimized.

Copy link

commented on ed00cd6 Sep 6, 2016

Might I suggest a few more?

Just did a quick grep of a few ffmpeg/curl internet stream logs I had backed up for headers that I didn't see included here:

Icy-MetaData:
ETag:
Strict-Transport-Security:
X-Frame-Options:
Accept-Ranges:
Content-Range:
Accept-Charset:

I am sure more will come up in due time 😀

Thanks for all your hard work folks! 👍

M

This comment has been minimized.

Copy link
Member

replied Sep 6, 2016

The X-Frame-Options HTTP response header can be used to indicate whether or not a browser should be allowed to render a page in a frame, iframe or object"

Please elaborate why you think this is useful

This comment has been minimized.

Copy link

replied Sep 6, 2016

well - that's what I get for quickly scanning log files - that header was from a curl session not related to streaming so my bad.

This comment has been minimized.

Copy link

replied Feb 12, 2017

What's the purpose of whitelisting headers?

Case in point: It breaks Kodi addons by throwing out authentication-releated headers, like over here: hippojay/plugin.video.plexbmc#181.

In this case, whitelisting everything that starts with X- would fix the issue in the plugin. What could break?

This comment has been minimized.

Copy link
Contributor Author

replied Feb 12, 2017

There was a very long discussion on what headers to allow (see there original PR #10402) as initially no headers were allowed. The current implementation allows the most common headers. Although allowing all custom X- prefixed headers seems reasonable, I have doubts as the X- prefixed headers are officially deprecated (see https://tools.ietf.org/html/rfc6648). Let's wait and see what @FernetMenta thinks.

This comment has been minimized.

Copy link
Member

replied Feb 13, 2017

As @basrieter mentioned, this was done for good reasons. It is not about restricting the HTTP protocol but the mechanism Kodi provides to addons using its infrastructure. The past has shown that this can result into a security breach, sending sensitive data out in the wild. This mechanism lacks a method of identifying what in the provided url was really meant to be a header.

With growing number of addons we need to start discussion about how to deal with security. Maybe we should start introducing security settings that users can extra privileges to addons.

This comment has been minimized.

Copy link

replied Feb 13, 2017

But it effectively is restricting the HTTP protocol.

If I want my plugin to misbehave then I easily can send relevant data as an URL parameter or in the request body. I can even encrypt it. There is no way for Kodi to check this, so claiming that restricting headers would have security implications is indefensible as a position.

I'm pretty sure that anecdotal evidence can be found where some plugin sent sensitive data via HTTP header, but it's easy to see that this change would do nothing to stop the author of such a plugin. In fact, in the particular issue that brought me here, the opposite is the case. The header in question is a server authentication token. Sending it as URL parameter would populate log files 1000-fold with that token and it would be easy to argue that not spamming logs the sensitive information has a positive effect on the user's security.

Anecdotal evidence can be found for and against, it's not good guidance. If this is an infrastructure method intended to underpin general communication in Kodi, then it should support HTML without making any assumptions. How headers are being used or even mis-used is completely orthogonal to the purpose of a function that implements the HTTP interface.

To employ a bad analogy, that's like restricting the steering wheel of a car so that drivers it can't make left turns anymore and then justifying it by saying that left turns are statistically more likely to cause accidents therefore it's for the driver's safety and three rights make a left anyway so it's not a bad move. It's still a bad move.

This comment has been minimized.

Copy link

replied Feb 19, 2017

While I understand and partially agree with the spirit of the Kodi Dev teams reasons for the restrictions.

I guess my ultimate take is that most folks already have a firewall, anti-virus and anti-malware (etc. etc.) installed on their system for protection.

So how about a compromise?

Would you consider adding an advanced setting switch to allow all headers?

Then you don't have to play net and protocol nanny for those that understand and assume the risks?

I would hope this sort of solution would be how most things should be solved when it is decided to arbitrarily impose restrictions for the masses but allow those that assume and understand the risks to remove the training wheels and drive like big boys.

Thoughts?

This comment has been minimized.

Copy link
Member

replied Feb 19, 2017

I would rather make it a GUI setting as mentioned above.

This comment has been minimized.

Copy link

replied Feb 22, 2017

I maintain the position that

  • There is no actual increase in user security to be gained from restricting the HTTP headers. As a malicious developer I can send my sensitive data in any of the allowed headers, or in a myriad of other ways. It simply does not add up to "more security".
  • A false sense of security is a bad thing, it's worse than no security at all.
  • It's not the duty of an infrastructure method to be partisan of the tasks it is asked to carry out.
  • It breaks existing, benign implementations, forcing them to change working code for no other reason than to comply with a what seems like a completely arbitrary "allowed set" of headers.
  • It introduces needless complexity at the wrong point. I foresee that reasons will come up to extend the list of "allowed" headers, bit by bit.
  • Overall, it cripples the usefulness of HTTP for all the wrong reasons.
  • An advanced option in the UI that switches off this behavior is a bad compromise, because the entire behavior itself is not advantageous to begin with.

I appreciate the fact that this has been in discussion for a while, even though I find it a bit odd that the discussion and also the code comments seem to focus around ffmpeg, which hardly is the only use case here.

Maybe I am missing an important, justifying point. But so far I am not convinced that this really is a useful change and think that rolling it back entirely does not have substantial drawbacks.

@arnova

This comment has been minimized.

Copy link
Member

commented Sep 6, 2016

@FernetMenta : Looks good to me.

@FernetMenta FernetMenta merged commit 1cda07d into xbmc:master Sep 6, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins.kodi.tv You did a great job. Have a cookie.
Details
@basrieter basrieter referenced this pull request Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.