Changeset 7749 in josm for trunk/src/org/openstreetmap

Timestamp:

2014-11-26T13:50:31+01:00 (9 years ago)

Author:

bastiK

Message:

remotecontrol /import: only allow whitelisted download tasks to be called from remotecontrol

Turned off import of session files for now.
I think it is not really an issue at the moment but as new features are added,
this may accidentally get overlooked.
E.g. any javascript execution with rhino engine as we currently do for loading of
preference snippets is a no-go.

In order to enable remotecontrol for a plugin download-task, override the method
isSafeForRemotecontrolRequests() in AbstractDownloadTask or derive from a
class that is already whitelisted (e.g. DownloadOsmTask).

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/OpenLocationAction.java (modified) (3 diffs)
• actions/downloadtasks/AbstractDownloadTask.java (modified) (2 diffs)
• actions/downloadtasks/DownloadGpsTask.java (modified) (1 diff)
• actions/downloadtasks/DownloadNotesTask.java (modified) (1 diff)
• actions/downloadtasks/DownloadOsmTask.java (modified) (1 diff)
• actions/downloadtasks/DownloadSessionTask.java (modified) (1 diff)
• actions/downloadtasks/DownloadTask.java (modified) (1 diff)
• io/remotecontrol/handler/ImportHandler.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/actions/OpenLocationAction.java

@@ -131 +131 @@
      * Replies the list of download tasks accepting the given url.
      * @param url The URL to open
+     * @param isRemotecontrol True if download request comes from remotecontrol.
      * @return The list of download tasks accepting the given url.
      * @since 5691
      */
-    public Collection<DownloadTask> findDownloadTasks(final String url) {
+    public Collection<DownloadTask> findDownloadTasks(final String url, boolean isRemotecontrol) {
         List<DownloadTask> result = new ArrayList<>();
         for (Class<? extends DownloadTask> taskClass : downloadTasks) {
@@ -140 +141 @@
                 try {
                     DownloadTask task = taskClass.getConstructor().newInstance();
-                    if (task.acceptsUrl(url)) {
+                    if (task.acceptsUrl(url, isRemotecontrol)) {
                         result.add(task);
                     }
@@ -179 +180 @@
     public void openUrl(boolean new_layer, final String url) {
         PleaseWaitProgressMonitor monitor = new PleaseWaitProgressMonitor(tr("Download Data"));
-        Collection<DownloadTask> tasks = findDownloadTasks(url);
+        Collection<DownloadTask> tasks = findDownloadTasks(url, false);
         DownloadTask task = null;
         Future<?> future = null;

trunk/src/org/openstreetmap/josm/actions/downloadtasks/AbstractDownloadTask.java

@@ -68 +68 @@
 
     // Can be overridden for more complex checking logic
-    @Override
     public boolean acceptsUrl(String url) {
         if (url==null) return false;
@@ -77 +76 @@
         }
         return false;
+    }
+
+    /**
+     * Check / decide if the task is safe for remotecontrol.
+     * 
+     * Keep in mind that a potential attacker has full control over the content
+     * of the file that will be downloaded.
+     * If it is possible to run arbitrary code or write to the local file
+     * system, then the task is (obviously) not save for remote execution.
+     * 
+     * The default value is false = unsafe. Override in a subclass to
+     * allow running the task via remotecontol.
+     * 
+     * @return true if it is safe to download and open any file of the
+     * corresponding format, false otherwise
+     */
+    public boolean isSafeForRemotecontrolRequests() {
+        return false;
+    }
+
+    @Override
+    public boolean acceptsUrl(String url, boolean isRemotecontrol) {
+        if (isRemotecontrol && !isSafeForRemotecontrolRequests()) return false;
+        return acceptsUrl(url);
     }
 

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadGpsTask.java

@@ -198 +198 @@
     }
 
+    @Override
+    public boolean isSafeForRemotecontrolRequests() {
+        return true;
+    }
+
     /**
      * Determines if the given URL denotes an OSM gpx-related API call.

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadNotesTask.java

@@ -67 +67 @@
     }
 
+    @Override
+    public boolean isSafeForRemotecontrolRequests() {
+        return true;
+    }
+
     abstract class DownloadTask extends PleaseWaitRunnable {
         protected OsmServerReader reader;

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadOsmTask.java

@@ -169 +169 @@
     }
 
+    @Override
+    public boolean isSafeForRemotecontrolRequests() {
+        return true;
+    }
+
     /**
      * Superclass of internal download task.

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadSessionTask.java

@@ -72 +72 @@
         return null;
     }
+
+    /**
+     * Do not allow to load a session file via remotecontrol.
+     * 
+     * Session importers can be added by plugins and there is currently
+     * no way to ensure that these are safe for remotecontol.
+     * @return 
+     */
+    @Override
+    public boolean isSafeForRemotecontrolRequests() {
+        return Main.pref.getBoolean("remotecontrol.import.allow_session", false);
+    }
 }

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadTask.java

@@ -75 +75 @@
      * Returns true if the task is able to open the given URL, false otherwise.
      * @param url the url to download from
+     * @param isRemotecontrol True if download request comes from remotecontrol.
      * @return True if the task is able to open the given URL, false otherwise.
+     * Return false, if the request comes from remotecontrol, but the task is not
+     * safe for remotecontrol.
+     * A task is not safe for remotecontrol if it is possible to prepare a file
+     * for download which does something unintended, e.g. gain access to the
+     * local file system.
      */
-    boolean acceptsUrl(String url);
+    boolean acceptsUrl(String url, boolean isRemotecontrol);
 
     /**

trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/ImportHandler.java

@@ -125 +125 @@
         }
         // Find download tasks for the given URL
-        suitableDownloadTasks = Main.main.menu.openLocation.findDownloadTasks(urlString);
+        suitableDownloadTasks = Main.main.menu.openLocation.findDownloadTasks(urlString, true);
         if (suitableDownloadTasks.isEmpty()) {
             // It should maybe be better to reject the request in that case ?