Skip to content
This repository has been archived by the owner on Jul 11, 2022. It is now read-only.

XSS security enhancements #121

Open
phloxic opened this issue Jul 26, 2013 · 17 comments
Open

XSS security enhancements #121

phloxic opened this issue Jul 26, 2013 · 17 comments
Assignees
Milestone

Comments

@phloxic
Copy link
Contributor

phloxic commented Jul 26, 2013

  1. allow the core player only to load SWF plugins from the same domain as where it is located
  2. parse playerId with a regex as strict as possible
@ghost ghost assigned anssip Jul 26, 2013
@phloxic
Copy link
Contributor Author

phloxic commented Aug 7, 2013

  • direct embedding: disallow external config files from domains other than from where core is loaded

anssip added a commit that referenced this issue Sep 9, 2013
@anssip
Copy link
Contributor

anssip commented Sep 9, 2013

@blacktrash Why is playerId parsing with regex needed?

@phloxic
Copy link
Contributor Author

phloxic commented Sep 12, 2013

Maybe I'm wrong, but I understood that api methods and events normally only work if there's a playerId present - again normally generated by the API scripts.
To prevent "illegal" annexion of the api by rogue embedding scripts, my idea was to exclude playerIds which are not generated by the api script. If there is indeed a pattern to which the playerIds generated by flowplayer.js obey.
A lot of ifs and assumptions due to my lack of knowledge of the as code, so feel free to ignere if that does not make sense or cannot be implemented.

@danrossi
Copy link
Contributor

This might be possible via the application domain api possibly for the loader. It will follow whatever the cross domain policy is normally.

@daveo1001
Copy link

Hi gentlemen,
I was wondering how the solution of this matter was going.

@daveo1001
Copy link

Doh, If I knew how to read I would have seen that anssip fixed it in the dev branch.
I now have a follow up stupid question, how does one take the github source files and make a swf? I've got a commercial version.

@danrossi
Copy link
Contributor

checkout the dev branch + flash-build rep. You run the build file from the flash-build directory now.

@daveo1001
Copy link

Pew. Spent all day trying to set up the flex/ant/build. I'm officially deeming it over my head.

Looking forward to an official release. 😁

@danrossi
Copy link
Contributor

If you follow the developer instructions all you have to do is type "ant build-biz"

@phloxic
Copy link
Contributor Author

phloxic commented Jan 12, 2014

@anssip - the external config file restriction is not working: http://flowplayer.blacktrash.org/test/embed-external.html - core loaded from (releases.)flowplayer.org, config loaded from (media.)blacktrash.org.

Maybe it does not check against the location of the core, but of the viewing domain.
Re-opening.

@phloxic phloxic reopened this Jan 12, 2014
@xzyfer
Copy link

xzyfer commented Jul 25, 2014

Has this been fixed yet?

@ViliusS
Copy link

ViliusS commented Aug 2, 2014

So, now even security vulnerabilities are not fixed in Flash version anymore? I think a sign is clear, Flowplayer Flash is not supported anymore.

@timw
Copy link

timw commented Jan 19, 2016

Bypasses of the XSS checking introduced here went public at Black Hat 2015.

http://www.bishopfox.com/download/5439/ (around slide 54)
https://youtu.be/ekUQIVUzDX4 (at 21:16)

@danrossi
Copy link
Contributor

Thanks for that update. There is a possible issue with the local domain check. I say remove this completely and I have no idea what it is for.

if (url.indexOf("/") == 0) return true;

Ads plugins are loaded externally also so not sure what to say other than to specifically whitelist those domains internally and block all external domain plugins. I'm sure there is a simpler way with the use of the loader context which initially checks for the policy file.

This probably needs to be scrapped also or looked into.

https://github.com/flowplayer/flash/blob/master/core/src/actionscript/org/flowplayer/view/PluginLoader.as#L107

@danrossi
Copy link
Contributor

This would be under the assumption siteA has been modified to begin with. It could also affect any plugin based player also. They might just go ahead with javascript malware includes then if the page is modified :)

Within JWPlayer's own AssetLoader which is still used for Flash plugins it has no such check at all, it only uses the cross domain context and relies on crossdomain policies as standard checking for "http" on the url.

This may be blown out of proportions but ideally just scrap these bits out, allow only under same domain and add whitelists for the ads plugins.

Javascript injections is by far more common and much worse.

This is the bit the document mentions.

if (url.indexOf("/") == 0) return true;

danrossi added a commit that referenced this issue Jan 30, 2016
…abled wildcard security for loading crossdomain plugin code for now as same domain does not need this.
@danrossi
Copy link
Contributor

Please take a look at that branch. It fixes this bypass. I am not sure what that forward slash was for really. It could simply check for "http" to determine local loading but this particular code wants to include http://localhost. Another check elsewhere was looking for occurances of

://

which needed to be changed to

//

To accomodate the new protocoless urls.

@danrossi
Copy link
Contributor

This is probably not needed anymore but needs heavy testing. Not sure what to do with the ads plugins yet apart from whitelist them ? Is there another ticket for this ?

bbc97b3#diff-402066450e33de5c5ce6a7ec96e48a87R108

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants