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

POC: Navigator: disallow requests to some domains (currently mochiads.com) #3511

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrian17
Copy link
Collaborator

@adrian17 adrian17 commented Mar 2, 2021

Requested on discord, no GH issue :(

mochiads.com was used by some flash games for distributing ads (and other uses?) - currently the domain was taken over by what looks like malware distributor.

PR design questions:

  • disable flag in options? (needed for MVP?)
  • list of banned domains in options? (needed for MVP?)
  • what logging level (browsers usually log blocked requests by default)?
  • currently the error message is duplicated, because the Err() variant contents are pretty much just ignored. Maybe some better way to write this?

@Bale001
Copy link
Member

Bale001 commented Mar 2, 2021

I definitely think it should be an array of banned domains, looks weird for the domain to be hard coded like it is right now

@kmeisthax
Copy link
Member

I feel a little iffy about this feature, because it represents fundamentally changing existing SWF content to work around problems caused by a particular advertising vendor. At least for self-hosted Ruffle, it seems like something that should be developer-configurable and disabled by default.

For the extension build, this is at least a more defensible option, as the extension is specifically for enabling old and broken websites. Generally speaking, the way browsers handle this problem is to check a remote service like Google Safe Browsing (or the Tencent equivalent, because China) to see if a domain is known malicious. That's actually a trickier job than you'd think - especially when you want to preserve user privacy. Furthermore, we'd have to figure out how to actually manage such a service - who hosts the list and decides what domains should be added to it. I'm not sure if I like the idea of Ruffle phoning home, either.

I'm not sure if I have any good answers to any of those questions. To answer your other questions:

  • Yes, we should allow disabling this behavior in the extension
  • We should log everything at the same level that a CORS failure would hit - which I believe is console.error level.

pub fn is_fetch_domain_banned(url: &str) -> bool {
if let Ok(parsed_url) = Url::parse(url) {
if let Some(domain) = parsed_url.domain() {
if domain.ends_with("mochiads.com") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would also block the domain examplemochiads.com; should probably be domain == "mochiads.com" || domain.ends_with(".mochiads.com") or similar

Copy link
Member

@torokati44 torokati44 Mar 3, 2021

Choose a reason for hiding this comment

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

Some other things to keep in mind (although these are most likely nitpicking, overly defensive, and not relevant for this scenario):

  • mochiads.com. (note the trailing period) is the same as mochiads.com, except it's "absolute" instead of "relative" (thus, arguably more correct anyway, except almost nobody uses this form anywhere)
    EDIT: They are only actually the same under the assumption that com, being a fairly common TLD, doesn't/can't appear as a subdomain, but if there is no such rule, these two domains are actually different, but most likely resolve to the same address.
  • International domain names may be Punycode encoded, or represented directly in Unicode - not sure where the en-/decoding would occur, if any.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess this is just the job for something like tldextract crate.

Copy link
Member

Choose a reason for hiding this comment

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

Punycoding an ASCII domain merely prepends xn-- and appends - to the domain. The idea is that the prefix says "this is a Punycode domain", then you put all of the directly representable characters in the string, then the -, and finally an encoded list of Unicode code points and where to insert them into the ASCII part of the domain.

Strictly speaking, mochiads.com and xn--mochiads-.com are separate domains. I don't know if/how Adobe handled Punycoding internally or if movie authors were expected to do it themselves. I'm going to assume it works like HTML5 fetch, and thus is_fetch_domain_banned should be seeing Unicode strings that get passed to HTML5 fetch and then the browser actually encodes any IDNs for us. Please correct me if I'm wrong.

@madsmtm
Copy link
Contributor

madsmtm commented Mar 3, 2021

I think phoning home is only really something you'd do when the list of blocked domains gets to many thousand entries, and you can't comfortably store it on the users device; I think that's highly unlikely, and we can tackle that problem when that happens, if at all.

At least for self-hosted Ruffle, it seems like something that should be developer-configurable and disabled by default.

Agree with developer-configurable (could we just make it a crate feature? If someone really wants to input a custom domain, they could just modify the source), but I definitely think it should be enabled by default, also in the self-hosted version!

A few (valid IMHO) points from a Discord member:

[...] I imagine a scenario where a hacker owns mochibot.com and mochiads.com and sends back content to all those flash games that are still requesting ads from the old service.
Thinking through this "problem" its not just Mochi that could present risks, since there are thousands of broken or "bad" URLs in old games, some of which probably now redirect to spam or pr0n sites, etc.

@madsmtm
Copy link
Contributor

madsmtm commented Mar 3, 2021

We should also decide when we want to use this list; should we liberally add dead domains, or only when not adding them has a direct effect on the user's experience?

An example is MochiBot (homepage, ActionScript source, downloaded SWF, example request on current domain) which was just used for analytics, and doesn't (as of this writing) impact the behaviour of the running SWF.

But this domain is owned by an unknown actor (definitely not the one that the SWF author intended), and may at any point change the returned SWF and decide to serve ads, change the game behaviour, or just break games.

@MartySVK
Copy link
Contributor

MartySVK commented Mar 3, 2021

I agree with @kmeisthax
This is something that should be developer-configurable and disabled by default. Would be great to have a option to enable it in extension.

@Herschel
Copy link
Member

Herschel commented Mar 9, 2021

My initial reaction is also a little against include hardcoded domains by default -- it seems to me that this adds another layer that potentially rots over time as domains may change hands, etc. But we should definitely provide this as an option, both in self-hosted and in the extension.

This could also tie-in with a more general URL-redirection feature, where you can redirect network requests from one URL to another (#1486). This could allow None or "" to mean 'disallow requests that match this pattern'.

Haven't tested this yet; if mochiads.org is blocked, does this allow the Mochi preloader to exit quicker, or does it still show the loading bar for a few seconds?

@wojtekpolska
Copy link

I don't know if this thread is still active but:
i'd use the addon's settings for this, it would be under some "advanced" tab as blocked domains
it would contain blocked domains (like mochiads) by deafult, would allow users to add/remove domains to their liking (and also restore-deafult button)

ruffle for website owners would have a similar function in config somewhere

@stuaxo
Copy link

stuaxo commented Mar 7, 2022

Perfect is the enemy of done.

In that spirit - shipping this using an array initially (possibly only with mochiads in it to start), and then later on adding this to the addon settings may be a way forward ?

@danielhjacobs danielhjacobs added security Pull requests that address a security vulnerability T-feature Type: New Feature (that Flash doesn't have) A-web Area: Web & Extensions A-desktop Area: Desktop Application A-core Area: Core player, where no other category fits labels Sep 17, 2024
@orazioedoardo
Copy link

Requests can also be disallowed at the origin level with CSP, example: Content-Security-Policy: connect-src 'self'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits A-desktop Area: Desktop Application A-web Area: Web & Extensions security Pull requests that address a security vulnerability T-feature Type: New Feature (that Flash doesn't have)
Projects
None yet
Development

Successfully merging this pull request may close these issues.