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

fix(FEC-11791): share url has cross-site scripting vulnerability #4255

Merged
merged 2 commits into from Jan 3, 2022

Conversation

lianbenjamin
Copy link
Collaborator

description of the issue:
when share plugin is enabled for the player, the share url is not being sanitized, which exposes a security vulnerability.

the solution:
sanitizing encoded and decoded share url.

Solves FEC-11791

@PlaykitJs-Bot
Copy link

Live Pull Request Urls

@@ -616,7 +616,7 @@
res = document.URL;
}
}
return res;
return kWidget.sanitize(decodeURIComponent(res));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to encode the url again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the decodeURIComponent replaces each escape sequence in the encoded with the character that it represents in ascii. if it's not an escaping char, it moves on. for example:
https://www.kaltura.com/.../embed/dynamic?%22%3E%3Cimg%20src=x%20onerror=prompt(4)%3E
would turn into:
https://www.kaltura.com/.../embed/dynamic?">
by doing so, the sanitizer would be able to replace any escaping chars with empty strings. without that decoding stage, sanitizer won't work in cases where there is encoded uri. if the uri is not encoded (most cases), the decoder does nothing and returns the uri string as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think encode it again it safer. if it was encoded we have to keep it encoded. if not we just encode the url, what the side effect? @OrenMe wdyt?

@yairans yairans requested a review from OrenMe December 23, 2021 09:07
@yairans
Copy link
Contributor

yairans commented Dec 23, 2021

@OrenMe please take a look on this PR

@PlaykitJs-Bot
Copy link

Updated pull request 4255: b3f6f0d

1 similar comment
@PlaykitJs-Bot
Copy link

Updated pull request 4255: b3f6f0d

@lianbenjamin lianbenjamin merged commit 4f11b6f into 2.92 Jan 3, 2022
@lianbenjamin lianbenjamin deleted the FEC-11791-share-url-sanitize branch January 3, 2022 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants