MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.68k stars 1.34k forks source link

Whitelist review: Java AWT #2531

Open Cervator opened 8 years ago

Cervator commented 8 years ago

As noted in #2494 whitelisting the entire java.awt package may be overkill and sneaky things could exist in there that could be a security risk for the module sandbox. We also probably don't need most of it. Packages java.awt.geom and java.awt.image are also whitelisted but might be less trouble.

Two ways to approach:

  1. Go through the java.awt package and look for scary classes (anything that allows file or socket access, that sort of thing). If there are no dubious classes we're probably OK and this could be closed
  2. Simply take off the java.awt package, see what happens! Likely the game will crash with a ClassNotFound style error. Add that one class instead (to the CLASSES list instead of the PACKAGES list), rinse, repeat.

To fully exercise modules potentially using AWT may have to try a few sets of modules (JoshariasSurvival, LightAndShadow, GooeysQuests, MedievalCities). Can run the game itself from source but copy in all modules from a game zip into /modules (except the few that are bundled with the engine) to test more easily.

Since that process is more about playing the game till it crashes this might not be too hard to test out some. At least option 2. And we need some more proper testers anyway, this could be good practice! :-)

Whitelist is edited in ExternalApiWhitelist.java

engiValk commented 7 years ago

So, AWT has a huge amount of files and also calls the use of A LOT of different .jar files. Notably the security ones. While from what I've seen it does look fairly safe, its probably not worth the risk, it uses over 2000 files alone. Its very likely something we don't want can slip through the cracks.

I am still working through it to find any holes that I can but it will take a long time, I'd personally recommended just white listing what we need, or, at least be very careful up until the point I can review everything and give a more direct answer, but that could take about a month of me reading it all.

Cervator commented 7 years ago

Thanks for the initial look @engiValk :-)

I would definitely start looking at just whatever Terasology actually uses (project search for java.awt ?) then consider whether whatever usage could be whitelisted on its own or has a superior replacement. Then take the big AWT item off the whitelist entirely rather than worry about all that stuff.

skaldarnar commented 7 years ago

I quickly greped the occurrences of java.awt in the main project (no additional modules). Here's a sorted list of the inputs. If it is not too much hassle, the start import could be removed alongside the whitelist changes.

import java.awt.*;
import java.awt.BorderLayout;
import java.awt.Canvas;
import java.awt.Color;
import java.awt.Component;
import java.awt.Dimension;
import java.awt.Graphics2D;
import java.awt.Graphics;
import java.awt.GraphicsEnvironment;
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.RenderingHints;
import java.awt.Toolkit;
import java.awt.datatransfer.Clipboard;
import java.awt.datatransfer.DataFlavor;
import java.awt.datatransfer.StringSelection;
import java.awt.datatransfer.Transferable;
import java.awt.datatransfer.UnsupportedFlavorException;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.ComponentEvent;
import java.awt.event.ComponentListener;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;
import java.awt.event.MouseMotionListener;
import java.awt.event.MouseWheelEvent;
import java.awt.event.MouseWheelListener;
import java.awt.event.WindowEvent;
import java.awt.event.WindowListener;
import java.awt.geom.Rectangle2D;
import java.awt.image.BufferedImage;
import java.awt.image.ColorModel;
import java.awt.image.DataBufferByte;
import java.awt.image.DataBufferInt;
import java.awt.image.DirectColorModel;
import java.awt.image.Raster;
import java.awt.image.WritableRaster;
Cervator commented 7 years ago

More research posted by @smsunarto over at http://forum.terasology.org/threads/java-awt-whitelist.1741/#post-14374

iojw commented 7 years ago

Also chiming in with the research I did a while back containing some more potentially harmful classes as well as a breakdown of the usage of java.awt 🙂

http://forum.terasology.org/threads/java-awt-whitelist-research.1726/

skaldarnar commented 7 years ago

I just merged the two threads into http://forum.terasology.org/threads/java-awt-whitelist.1741/

Anyone interested in taking on this issue. I think there is a clear line of what is needed (should be whitelisted) and what is not. Can we also do it the other way around and blacklist the potentially harmful parts of AWT?

JohnMH commented 7 years ago

I'd suggest just not doing whitelist security? That essentially makes this non-free software, and it's pretty stupid from a security standpoint.

OvermindDL1 commented 7 years ago

I'd suggest just not doing whitelist security? That essentially makes this non-free software, and it's pretty stupid from a security standpoint.

It would also allow a modder to send whatever code they wanted to you just by joining them, thus doing whatever they want on your machine. Short of using a sandboxed language, whitelisting java modules is... closeish... >.>

JohnMH commented 7 years ago

@OvermindDL1 Solution to that: Don't let the server send arbitrary code.. Which I don't see it being able to do? Are you suggesting the server can send Java class objects over the network and have a method on them invoked?

OvermindDL1 commented 7 years ago

Except it is useful for the server to send mods so the clients can be synched without needing to download an entire pack of their own while getting the right things enabled.

Cervator commented 7 years ago

@JohnMHarrisJr

I'd suggest just not doing whitelist security? That essentially makes this non-free software, and it's pretty stupid from a security standpoint.

Wait what? What does a whitelist have to do with whether or not this is free software?

This is not some list of projects we have decided are cool or not. It is specifically for what modules are allowed to access in the engine and beyond. You can have anything of your choice added by simply asking, adding it in your own fork, or even just turning off security if you don't care for it.

The engine has no restrictions. You can add any dependency you want and it'll work. The security intent here is entirely to sandbox in user-added and contributed content modules. We're trying to support both a secure and convenient ecosystem for modules to make it easier for players to get content they're interested in while not risking their system - while even allowing users to entirely opt out of our systems to use alternatives of their own choice

Just to re-emphasize: the whitelist is between the game engine and content modules. Not between our project and third parties. It is so a user cannot write a module that uses File to recursively delete every file on your system it can get to, host a server, advertise it to others, and entrap victims by having them simply run the game and join that server. So long as they actually have a legitimate copy of the engine anyway.

Hopefully there's just a misunderstanding of intent here :-)

JohnMH commented 7 years ago

@Cervator This makes it harder for those wishing to run their own customized clients, among other things.

The concept of "legitimate client" is also an issue. I highly suggest looking at how Minetest does things.

JohnMH commented 7 years ago

@OvermindDL1 If security is a concern, the server should not be able to send mods at all. There are a handful of ways around these 'whitelists', for example. One can also (ab)use the ClassLoader to bypass the whitelist entirely.

Cervator commented 7 years ago

This makes it harder for those wishing to run their own customized clients, among other things.

But - it doesn't? The client is pure engine. No whitelist.

The moment you're hacking your own client or distributing a nefarious fake then that's out of our hands. The goal of the whitelist is to make a legit client invulnerable to malicious modules - that's it.

If you can find ways to write a malicious module that can compromise a legit client, via classloader tricks or whatever, please do so we can improve the system :-) The whitelist isn't at all perfect yet, that's partly why this issue is here.

JohnMH commented 7 years ago

"hacking your own client or distributing a nefarious fake"

Again, you shouldn't assume the client is running exactly what's in this repository. Why should I have to run your code exactly as it is? Why is "legit client" something you think exists?

And classloader tricks and things like that are just par for the course with Java.

Cervator commented 7 years ago

@JohnMHarrisJr - why are you here? It sounds like you're complaining just to complain.

The official download is what we distribute, from this repository, as the maintainers of the project. That's the product. That's how this project works.

The license allows you to do almost whatever you want with the code. We encourage it. But anything else is no longer the official game and outside the scope of what we're worried about.

If you run any other download you assume 100% of the risk. We try to encourage people to avoid that. We don't care if you don't want to run what we distribute, and don't support or endorse any alternatives. Simple as that. We do go out of our way some to make it easy to host your own things like the new identity server or telemetry stuff, rather than force the use of whatever option we may offer.

If you want a project that supports alternative offerings as other "first class citizens" then you're in the wrong place. I'll pick any inherent security risks with Java over trying to warrant variants like that. And besides as mentioned earlier this isn't even about the whole game client, it is about the security barrier we put modules behind.

We are simply trying to build a nice safe sandbox for others to share modules with each others in - and so long as they downloaded the game from us any module from anywhere should be safe to run inside that sandbox.

JohnMH commented 7 years ago

@Cervator I am here because I was made aware of this issue and thought I would offer assistance.

There is no "official" download. The moment you start to label software as "official" and "unofficial", you're getting closer to the proprietary software mindset. There is no product. You're creating a software project, not a product. You're not selling apples.

The project is under the Apache License, people can modify the code and do whatever they want with it. That includes clients running modified code connecting to unmodified servers.

SkySom commented 7 years ago

You're right, we are creating a software project, one which we have to support. Calling something "official" means that we promise the best level of support we can provide . Anything else and you're potentially on your own. So yes, you can modify the client and connect, but that doesn't mean that you can expect it to work perfectly, nor for us to treat it as a valid configuration.

smsunarto commented 7 years ago

@JohnMHarrisJr hey, I heard you like debating word usage. So if we don't call the community supported and organization endorse client as the "official", what should we call it? :)

Cervator commented 7 years ago

@JohnMHarrisJr I'd appreciate knowing from where you were being made aware that this was an issue, so I can go correct those people's misinterpretations as well :-)

Again - you can do whatever you want, as per the license. Have fun. In the meantime we're using this strategy to keep regular players able to safely download content modules automatically without risking their computer - so long as they are using our very official download.

If you think that makes Terasology proprietary software ... I honestly don't know what to tell you. I think we may live in different dimensions or something. I consider that subject off topic for this issue talking about specific packages on a whitelist solely used to block modules off from the engine, all entirely within our project with no relevance to the outside world

If you find it offensive that somebody somewhere on the internet could be running a game server you can't connect to with your customized version of their software well that's your offense to take. That's not the problem of the project if that's how they choose to run it, nor does the Apache license extend to offer guidance for how to maintain a project like that. Just how the code itself can be used.

If you want to passionately discuss open source purist philosophy I'm sure there is a place for that. This isn't it.

JohnMH commented 7 years ago

It'd probably be best to discuss this either on a mailing list or at least another issue, but I'll respond here I suppose.

The issue with calling it 'official' rather than 'supported' or something like that is that the terminology discourages running altered versions. This is troublesome for free software, as anything that discourages modification also discourages possible contributors.

@Cervator I'm sorry for any misunderstanding, I never meant to suggest that the software was proprietary just because of the terminology used. I agree that this particular issue is not the correct place for the current discussion.

Cervator commented 7 years ago

@JohnMHarrisJr Thank you.

As noted above if you find actual concrete flaws in the current security implementation we can test for and improve that's hugely appreciated. Just this discussion itself went a bit off the rails.

We do encourage hacking away to your heart's content - for developers able to take care of themselves, like you. Simultaneously we also discourage using third party downloads / alternative clients for those players who just want a safe game, many of which haven't a clue about programming.

(We occasionally get support requests from people who downloaded some ancient game version off one of those typical file sharing / spammy ad-filled mirroring sites - that can be frustrating)

engiValk commented 6 years ago

@Cervator I found a slight potential issue.

https://docs.oracle.com/javase/7/docs/api/java/awt/Robot.html

While it isn't anything too serious, it allows the module to take control of the client when it is loaded, this could theoretically be used to force a connection to an online server and that opens up a whole host of logging and other nasty things.

Just thought it might be of interest.