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

Sanitize and sandbox toot embeds #9552

Merged
merged 1 commit into from Dec 23, 2018

Conversation

Projects
None yet
2 participants
@ThibG
Copy link
Collaborator

ThibG commented Dec 17, 2018

Up until now, we blindly included oEmbed replies from remote toots in the embed modal.
Thankfully, since we introduced a Content Security Policy, this is not a security issue in Mastodon itself.

It however causes Content Security Policy warnings, and we may still provide users with malicious instructions.

The proposed change sanitizes the oEmbed content before suggesting it to the user, and also sandboxes the code to prevent executing scripts in a same-origin context.

This does cause some functionality loss: the generated code for embedding remote toots will not include the JS snippet needed to adjust the iframe's height.

@ThibG ThibG added the security label Dec 17, 2018

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Dec 17, 2018

Another possibility, to avoid presenting the user untrusted code, is to accept serving remote toots with the embed controller, but it would require more changes.

@@ -10,6 +10,7 @@ def create
render json: status, serializer: OEmbedSerializer, width: 400
rescue ActiveRecord::RecordNotFound
oembed = FetchOEmbedService.new.call(params[:url])
oembed[:html] = Formatter.instance.sanitize(oembed[:html], Sanitize::Config::MASTODON_OEMBED) if oembed[:html].present?

This comment has been minimized.

@Gargron

Gargron Dec 17, 2018

Member

This will break Mastodon embeds because they need a <script> for iframe resizing, and this ruleset strips out script tags.

This comment has been minimized.

@ThibG

ThibG Dec 17, 2018

Author Collaborator

As I said:

This does cause some functionality loss: the generated code for embedding remote toots will not include the JS snippet needed to adjust the iframe's height.

But I prefer that to instructing users to include untrusted code (note that embeds from your own instance don't go through that codepath and the javascript tag is still included).

This comment has been minimized.

@ThibG

ThibG Dec 17, 2018

Author Collaborator

What I mean is, it's okayish to instruct people to include scripts that we control, but I don't think it's ok at all to instruct them to include scripts from a remote instance we don't control.

@Gargron Gargron merged commit e25947d into tootsuite:master Dec 23, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment