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

Color inputs are not filtered (security vulnerability for server-side blockly code evaluation) #2637

Closed
Alexejhero opened this issue Jul 15, 2019 · 11 comments

Comments

@Alexejhero
Copy link

Problem statement

The color literal block can be exploited to inject custom code. This is not a big issue with client-side generated code, but it can be a big issue with code that is saved and evaluated server-side.

For example, I have a website which allows users to make their own discord bot using blockly, and the code is saved in the server and eval'd when needed, meaning that the bot token could be leaked using this exploit.

Steps to Reproduce

  1. Manually load a workspace from a custom XML which contains an invalid color

  2. Generate code for that workspace

Expected Behavior

When generating the code, the color block should generate #ffffff or #000000, similarly to how invalid numbers generate NaN.

Actual Behavior

The color block generates its input as javascript without any kind of filter.

Additional Information

This exploit was discovered by @KhooHaoYit

@Alexejhero Alexejhero changed the title Potential security vulnerability regarding color inputs not being filtered Color inputs are not filtered (security vulnerability for server-side blockly code evaluation) Jul 15, 2019
@RoboErikG
Copy link
Contributor

@NeilFraser any thoughts on this?

@BeksOmega
Copy link
Collaborator

@RoboErikG I think this is covered by the new field class validation. Related test.

@Alexejhero
Copy link
Author

So this has been already fixed, but not released?

@RoboErikG
Copy link
Contributor

Yes, with caveats. The fix assumes you're taking users' xml, loading it into Blockly on the server, and generating code after it's been validated by Blockly's standard code. If users are generating code client side and sending that back you have no guarantees.

In general, preventing code injection is not high priority on our side, as Blockly allows users to write code in some form or another and many applications allow arbitrary code to be written even with standard blocks. Making sure fields/standard blocks are returning values as expected is something we work on, but to make code generation more reliable not for server side security.

If you're running user generated code on a server we recommend at least sandboxing, such as Neil's JS-Interpreter, and any other security practices that are appropriate for your application.

@Alexejhero
Copy link
Author

There was a previous exploit that allowed users to inject custom code by overriding workspaceToCode, but I fixed that by generating code server-side.

@Alexejhero
Copy link
Author

Sandboxing would not be a good solution here, because the code that is run on the server requires certain objects that have sensitive information.

For example, it requires message to run message.reply(), but message also has a message.client.token property, which should always be kept private.

@NeilFraser
Copy link
Contributor

Confirming vulnerability:
https://blockly-demo.appspot.com/static/demos/code/index.html#7ucqk4
Yikes! The good news is that the version in the develop branch already has this fixed.

Also tested other injection points. Injections of number fields go to NaN, injections of string fields are properly escaped, injections in variables and functions are escaped. Date and angle pickers don't have code generators, but a test of the angle picker in Blockly Games shows it defaults to 90. Comments appear safe from injection. That's all the injection points I can find.

It is very important that Blockly guarantees that malicious XML cannot generate arbitrary code. You should not be able to craft XML that generates an alert(document.cookie) in any language.

NeilFraser added a commit that referenced this issue Jul 15, 2019
The new validators already solve this problem, but as a second layer of defence, the generators should also be secured.  Issue #2637
@NeilFraser
Copy link
Contributor

In addition to the new validators, I've just created a change to our generators to add a second layer of defence. #2645

@RoboErikG
Copy link
Contributor

And walking back some of my comments. Preventing XML -> arbitrary code is high on our list of priorities. But we make mistakes, so I worry when someone depends on us doing that correctly in all cases. ;)

Thanks for pointing it out, @AlexejheroYTB
Thanks for the extra fixes, @NeilFraser

@Alexejhero
Copy link
Author

@KhooHaoYit is the one that found and told me about this exploit. I just reported it.

NeilFraser added a commit that referenced this issue Jul 15, 2019
The new validators already solve this problem, but as a second layer of defence, the generators should also be secured.  Issue #2637
@NeilFraser
Copy link
Contributor

Thanks everyone. If you want to grab this fix for your codebase without waiting for the next release, #2645 is a simple one-liner (per generated language).

rachel-fenichel pushed a commit that referenced this issue Jul 15, 2019
The new validators already solve this problem, but as a second layer of defence, the generators should also be secured.  Issue #2637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants