airbnb / lottie-web

Render After Effects animations natively on Web, Android and iOS, and React Native. http://airbnb.io/lottie/
MIT License
30.34k stars 2.87k forks source link

XSS vulnerability #2828

Open nigelgutzmann opened 2 years ago

nigelgutzmann commented 2 years ago

Tell us about your environment Any web browser

What did you do? Please explain the steps you took before you encountered the problem. I created a lottie file with an expression inside of it. I edited the expression to contain the following code, to expose an XSS vulnerability within lottie-web:

}]; alert(\"Arbitrary evil XSS code.\");[function _expression_function(){

Here's a proof of concept (warning this contains an XSS attack, but the attack only displays an alert): https://codesandbox.io/s/empty-snowflake-lq6yhq?file=/src/evil-animation.json:1694-1767

What did you expect to happen? I hope that arbitrary code execution would not be possible.

What actually happened? Please include as much relevant detail as possible. This shows an alert when the animation is played. In a more malicious situation an attacker could:

Please provide a download link to the After Effects file that demonstrates the problem. Any file

shaafiee commented 2 years ago

This is why this was important: https://github.com/airbnb/lottie-web/pull/2534

This: https://github.com/airbnb/lottie-web/blob/master/player/js/utils/expressions/ExpressionManager.js#L428 or a version of it, will be needed to preempt XSS calls.

antoineMoPa commented 2 years ago

This fix would be easy to bypass by obfuscating urls (ex: write ("h" + "ttps://") instead of https://). Also, it's still possible to run arbitrary code.

shaafiee commented 2 years ago

An exhaustive RE for all possible permutations you mentioned: h("|')?\s?+\s?("|')?t("|')?\s?+\s?("|')?t("|')?\s?+\s?("|')?p("|')?\s?+\s?("|')?s("|')?\s?+\s?("|')?:("|')?\s?+\s?("|')?\/("|')?\s?+\s?("|')?\/("|')?\s?+\s?("|')?

bodymovin commented 2 years ago

Hi, lottie uses eval for expressions, and it attempts to prevent the most obvious exploit cases. There's a lottie light version that doesn't use expressions that is usually suggested if the files that are being loaded are not safe. There is also a lottie worker version that should execute on its own js instance that should prevent these type of scenarios as well. Unfortunately, in order to support expressions, eval needs to be used, so there is not a workaround besides the ones previously mentioned. At this point, I don't think trying to continue patching the lottie-web context makes sense since it will never be completely safe. But any suggestions are welcome.

nigelgutzmann commented 2 years ago

@bodymovin I'm curious if you've explored using a Web Worker with only some features whitelisted?

https://stackoverflow.com/questions/10653809/making-webworkers-a-safe-environment/

This could allow you to remove all features that could be hijacked (DOM access, XHR, etc) and run the expression code in a protected environment.

I think the big change here would be that it would change the expression evaluation into an async operation because there would be postmessages going between the main thread and the web worker thread.

antoineMoPa commented 2 years ago

I think it's not immediately obvious for all users of lottie-web that it can run eval() on parts of the Lottie json. If no safer solution can be found, maybe a disclaimer in the Readme would be helpful?

michaeljherrmann commented 2 years ago

Or perhaps another idea is that expression support should be disabled by default and when initializing the renderer you have to explicitly enable it? And then you could add docs around that option on the risks of using expressions.

lottie.loadAnimation({
  container: element,
  renderer: 'svg',
  allowExpressions: true, // default of false
...
bodymovin commented 2 years ago

A disclaimer makes sense, I'll add it. Disabling expressions by default would be a breaking change and I'd like to avoid it. Running only the expressions on a separate context would slow down animations too much since they rely on values from the animation tree to do their calculations, that's why the other alternative is to directly use the lottie worker.

mbasaglia commented 2 years ago

Recently this PR was merged, which adds an option to disable expressions (keeping them enabled by default): https://github.com/airbnb/lottie-web/pull/2833

AdamEisfeld commented 5 months ago

Lottie really should be split into a base package that supports "plugins", and then eval can be enabled via installing an additional plugin package. Adding compile-time flags doesn't remove the vulnerability warning when installing the package, nor does importing from lottie light. This means for any teams trying for SOC compliance or just generally concerned about security, they'll skip lottie entirely.

It is unfortunate that this repo has seem to have died. It is still a functioning product and judging by the number of issues / open PRs, many people still have an interest in it.