Commit 902ac3d8 authored by Evan W. Patton's avatar Evan W. Patton

Fix XSS vulnerability in ClientJsonParser

ClientJsonParser was calling a GWT API that used eval() to parse JSON
content rather than JSON.parse(). A maliciously crafted project
containing an extension components.json file with Javascript instead
of JSON would allow injecting arbitrary Javascript into the user's
session. This commit switches to strict parsing and includes exception
handling to report corrupt/invalid components.json to the user.

Change-Id: Iafaaf004310ac45cf0c1cea18eae1cfd58de17ef
parent 563ec8c2
...@@ -593,6 +593,18 @@ public interface OdeMessages extends Messages { ...@@ -593,6 +593,18 @@ public interface OdeMessages extends Messages {
@Description("For importing from a URL") @Description("For importing from a URL")
String componentImportFromURL(); String componentImportFromURL();
@DefaultMessage("The component database in the project \"{0}\" is corrupt.")
@Description("Error message when the component database is not valid.")
String componentDatabaseCorrupt(String projectName);
@DefaultMessage("The extension description of \"{0}\" in the project \"{1}\" is corrupt.")
@Description("Error message when the component descriptors for an extension are not parsable.")
String extensionDescriptorCorrupt(String extensionName, String projectName);
@DefaultMessage("The project \"{0}\" contains an invalid extension. App Inventor will attempt to continue.")
@Description("Error message when an extension descriptor pathname does not have the correct structure.")
String invalidExtensionInProject(String projectName);
//Connect //Connect
@DefaultMessage("Connect") @DefaultMessage("Connect")
@Description("Label of the button leading to Connect related cascade items") @Description("Label of the button leading to Connect related cascade items")
......
...@@ -39,6 +39,7 @@ import com.google.appinventor.shared.storage.StorageUtil; ...@@ -39,6 +39,7 @@ import com.google.appinventor.shared.storage.StorageUtil;
import com.google.appinventor.shared.youngandroid.YoungAndroidSourceAnalyzer; import com.google.appinventor.shared.youngandroid.YoungAndroidSourceAnalyzer;
import com.google.common.collect.Maps; import com.google.common.collect.Maps;
import com.google.gwt.core.client.Scheduler; import com.google.gwt.core.client.Scheduler;
import com.google.gwt.json.client.JSONException;
import com.google.gwt.user.client.Command; import com.google.gwt.user.client.Command;
import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwt.user.client.rpc.AsyncCallback;
...@@ -471,8 +472,7 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang ...@@ -471,8 +472,7 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang
} }
public void addComponent(final ProjectNode node, final Command afterComponentAdded) { public void addComponent(final ProjectNode node, final Command afterComponentAdded) {
final ProjectNode compNode = node; final String fileId = node.getFileId();
final String fileId = compNode.getFileId();
AsyncCallback<ChecksumedLoadFile> callback = new OdeAsyncCallback<ChecksumedLoadFile>(MESSAGES.loadError()) { AsyncCallback<ChecksumedLoadFile> callback = new OdeAsyncCallback<ChecksumedLoadFile>(MESSAGES.loadError()) {
@Override @Override
public void onSuccess(ChecksumedLoadFile result) { public void onSuccess(ChecksumedLoadFile result) {
...@@ -483,7 +483,23 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang ...@@ -483,7 +483,23 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang
this.onFailure(e); this.onFailure(e);
return; return;
} }
JSONValue value = new ClientJsonParser().parse(jsonFileContent); JSONValue value = null;
try {
value = new ClientJsonParser().parse(jsonFileContent);
} catch(JSONException e) {
// thrown if jsonFileContent is not valid JSON
String[] parts = fileId.split("/");
if (parts.length > 3 && fileId.endsWith("components.json")) {
ErrorReporter.reportError(Ode.MESSAGES.extensionDescriptorCorrupt(parts[2], project.getProjectName()));
} else {
ErrorReporter.reportError(Ode.MESSAGES.invalidExtensionInProject(project.getProjectName()));
}
numExternalComponentsLoaded++;
if (afterComponentAdded != null) {
afterComponentAdded.execute();
}
return;
}
COMPONENT_DATABASE.addComponentDatabaseListener(YaProjectEditor.this); COMPONENT_DATABASE.addComponentDatabaseListener(YaProjectEditor.this);
if (value instanceof JSONArray) { if (value instanceof JSONArray) {
JSONArray componentList = value.asArray(); JSONArray componentList = value.asArray();
...@@ -625,7 +641,12 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang ...@@ -625,7 +641,12 @@ public final class YaProjectEditor extends ProjectEditor implements ProjectChang
private void resetExternalComponents() { private void resetExternalComponents() {
COMPONENT_DATABASE.addComponentDatabaseListener(this); COMPONENT_DATABASE.addComponentDatabaseListener(this);
COMPONENT_DATABASE.resetDatabase(); try {
COMPONENT_DATABASE.resetDatabase();
} catch(JSONException e) {
// thrown if any of the component/extension descriptions are not valid JSON
ErrorReporter.reportError(Ode.MESSAGES.componentDatabaseCorrupt(project.getProjectName()));
}
externalComponents.clear(); externalComponents.clear();
extensionsInNode.clear(); extensionsInNode.clear();
extensionToNodeName.clear(); extensionToNodeName.clear();
......
...@@ -18,6 +18,6 @@ public class ClientJsonParser implements JSONParser { ...@@ -18,6 +18,6 @@ public class ClientJsonParser implements JSONParser {
@Override @Override
public JSONValue parse(String source) { public JSONValue parse(String source) {
return source.isEmpty() ? null return source.isEmpty() ? null
: ClientJsonValue.convert(com.google.gwt.json.client.JSONParser.parseLenient(source)); : ClientJsonValue.convert(com.google.gwt.json.client.JSONParser.parseStrict(source));
} }
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment