Repository: redpen-cc/redpen
Pull request #557 (2016-03-03 22:40:07)

Improve redpen-server security
Introduce secure mode to Configuration.findFile(), which is needed in redpen-server to disallow loading of arbitrary files with user-provided config.

User can specify file paths using "dict" attribute for many validators, forcing the server to read these files from its own filesystem, potentially giving the possibility to the user to derive whether some content is there or not.


Patch:
diff --git a/redpen-core/src/main/java/cc/redpen/config/Configuration.java b/redpen-core/src/main/java/cc/redpen/config/Configuration.java
index d2838dc5..ba1ab8ec 100644
--- a/redpen-core/src/main/java/cc/redpen/config/Configuration.java
+++ b/redpen-core/src/main/java/cc/redpen/config/Configuration.java
@@ -42,18 +42,17 @@ public class Configuration implements Serializable, Cloneable {
     private List<ValidatorConfiguration> validatorConfigs = new ArrayList<>();
     private final String lang;
     private transient RedPenTokenizer tokenizer;
-    private File home = new File(Optional.ofNullable(System.getProperty("REDPEN_HOME", System.getenv("REDPEN_HOME"))).orElse(""));
-    private File base;
+    private final File home = new File(Optional.ofNullable(System.getProperty("REDPEN_HOME", System.getenv("REDPEN_HOME"))).orElse(""));
+    private final File base;
+    private final boolean secure;
 
-    /**
-     * Constructor.
-     */
-    Configuration(File base, SymbolTable symbolTable, List<ValidatorConfiguration> validatorConfigs, String lang) {
+    Configuration(File base, SymbolTable symbolTable, List<ValidatorConfiguration> validatorConfigs, String lang, boolean secure) {
         this.base = base;
         this.symbolTable = symbolTable;
 
         this.validatorConfigs.addAll(validatorConfigs);
         this.lang = lang;
+        this.secure = secure;
         initTokenizer();
     }
 
@@ -135,19 +134,37 @@ public class Configuration implements Serializable, Cloneable {
      * @throws RedPenException if file doesn't exist in either place
      */
     public File findFile(String relativePath) throws RedPenException {
-        File file = new File(relativePath);
-        if (file.exists()) return file;
+        File file;
+        if (!secure) {
+            file = new File(relativePath);
+            if (file.exists()) return file;
+        }
 
         if (base != null) {
             file = new File(base, relativePath);
-            if (file.exists()) return file;
+            if (secureExists(file, base)) return file;
         }
 
         file = new File(home, relativePath);
-        if (file.exists()) return file;
+        if (secureExists(file, home)) return file;
+
+        throw new RedPenException(relativePath + " is not under " +
+          (!secure ? "working directory (" + new File("").getAbsoluteFile() + "), " : "") +
+          (base != null ? "base (" + base + "), " : "") +
+          "$REDPEN_HOME (" + home.getAbsolutePath() + ").");
+    }
+
+    private boolean secureExists(File file, File base) {
+        try {
+            return file.exists() && (!secure || file.getCanonicalPath().startsWith(base.getCanonicalPath()));
+        }
+        catch (IOException e) {
+            return false;
+        }
+    }
 
-        throw new RedPenException(String.format("%s is not under working directory (%s)" + (base != null ? ", base (" + base + ")" : "")  + " or $REDPEN_HOME (%s).",
-          relativePath, new File("").getAbsoluteFile(), home.getAbsolutePath()));
+    public boolean isSecure() {
+        return secure;
     }
 
     /**
@@ -212,6 +229,7 @@ public class Configuration implements Serializable, Cloneable {
         private String lang = "en";
         private Optional<String> variant = Optional.empty();
         private File base;
+        private boolean secure;
 
         private void checkBuilt() {
             if (built) throw new IllegalStateException("Configuration already built.");
@@ -247,10 +265,19 @@ public class Configuration implements Serializable, Cloneable {
             return this;
         }
 
+      /**
+       * Enables secure mode suitable for servers, where validator properties can come from end-users.
+       */
+        public ConfigurationBuilder secure() {
+            checkBuilt();
+            secure = true;
+            return this;
+        }
+
         public Configuration build() {
             checkBuilt();
             built = true;
-            return new Configuration(base, new SymbolTable(lang, variant, customSymbols), this.validatorConfigs, this.lang);
+            return new Configuration(base, new SymbolTable(lang, variant, customSymbols), this.validatorConfigs, this.lang, this.secure);
         }
     }
 }
diff --git a/redpen-core/src/main/java/cc/redpen/config/ConfigurationLoader.java b/redpen-core/src/main/java/cc/redpen/config/ConfigurationLoader.java
index 985086b0..00b4a1d7 100644
--- a/redpen-core/src/main/java/cc/redpen/config/ConfigurationLoader.java
+++ b/redpen-core/src/main/java/cc/redpen/config/ConfigurationLoader.java
@@ -40,7 +40,7 @@ import static cc.redpen.config.Configuration.ConfigurationBuilder;
  */
 public class ConfigurationLoader {
     private static final Logger LOG = LoggerFactory.getLogger(ConfigurationLoader.class);
-    private ConfigurationBuilder configBuilder = new ConfigurationBuilder();
+    private boolean secure;
 
     private static Symbol createSymbol(Element element) throws RedPenException {
         if (!element.hasAttribute("name") || !element.hasAttribute("value")) {
@@ -165,7 +165,8 @@ public class ConfigurationLoader {
     public Configuration load(InputStream stream, File base) throws RedPenException {
         Document doc = toDocument(stream);
 
-        configBuilder = new ConfigurationBuilder().setBaseDir(base);
+        ConfigurationBuilder configBuilder = new ConfigurationBuilder().setBaseDir(base);
+        if (secure) configBuilder.secure();
         Element rootElement = getRootNode(doc, "redpen-conf");
 
         String language = rootElement.getAttribute("lang");
@@ -196,7 +197,7 @@ public class ConfigurationLoader {
             LOG.warn("There is no validators block");
         } else {
             NodeList validatorElementList = validatorConfigElementList.item(0).getChildNodes();
-            extractValidatorConfigurations(validatorElementList);
+            extractValidatorConfigurations(configBuilder, validatorElementList);
         }
 
         // extract symbol configurations
@@ -206,12 +207,12 @@ public class ConfigurationLoader {
         if (!variant.isEmpty()) configBuilder.setVariant(variant);
 
         if (symbolTableConfigElementList != null) {
-            extractSymbolConfig(symbolTableConfigElementList, language);
+            extractSymbolConfig(configBuilder, symbolTableConfigElementList, language);
         }
         return configBuilder.build();
     }
 
-    private void extractValidatorConfigurations(NodeList validatorElementList) {
+    private void extractValidatorConfigurations(ConfigurationBuilder configBuilder, NodeList validatorElementList) {
         ValidatorConfiguration currentConfiguration;
         for (int i = 0; i < validatorElementList.getLength(); i++) {
             Node nNode = validatorElementList.item(i);
@@ -249,7 +250,7 @@ public class ConfigurationLoader {
         }
     }
 
-    private void extractSymbolConfig(NodeList symbolTableConfigElementList, String language) throws RedPenException {
+    private void extractSymbolConfig(ConfigurationBuilder configBuilder, NodeList symbolTableConfigElementList, String language) throws RedPenException {
         configBuilder.setLanguage(language);
 
         NodeList symbolTableElementList =
@@ -298,4 +299,9 @@ public class ConfigurationLoader {
         LOG.info("Succeeded to load configuration file");
         return rootElement;
     }
+
+    public ConfigurationLoader secure() {
+        secure = true;
+        return this;
+    }
 }
diff --git a/redpen-server/src/main/java/cc/redpen/server/api/RedPenResource.java b/redpen-server/src/main/java/cc/redpen/server/api/RedPenResource.java
index d7672851..606d4a48 100644
--- a/redpen-server/src/main/java/cc/redpen/server/api/RedPenResource.java
+++ b/redpen-server/src/main/java/cc/redpen/server/api/RedPenResource.java
@@ -101,7 +101,7 @@ public class RedPenResource {
         if (config == null) {
             redPen = new RedPenService(context).getRedPen(lang);
         } else {
-            redPen = new RedPen(new ConfigurationLoader().loadFromString(config));
+            redPen = new RedPen(new ConfigurationLoader().secure().loadFromString(config));
         }
         Document parsedDocument = redPen.parse(DocumentParser.of(documentParser), document);
         List<ValidationError> errors = redPen.validate(parsedDocument);
diff --git a/redpen-server/src/main/java/cc/redpen/server/api/RedPenService.java b/redpen-server/src/main/java/cc/redpen/server/api/RedPenService.java
index 02f4914d..84b1dd49 100644
--- a/redpen-server/src/main/java/cc/redpen/server/api/RedPenService.java
+++ b/redpen-server/src/main/java/cc/redpen/server/api/RedPenService.java
@@ -21,6 +21,7 @@ package cc.redpen.server.api;
 import cc.redpen.RedPen;
 import cc.redpen.RedPenException;
 import cc.redpen.config.Configuration;
+import cc.redpen.config.ConfigurationLoader;
 import cc.redpen.config.ValidatorConfiguration;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -47,7 +48,7 @@ public class RedPenService {
     private final static String DEFAULT_INTERNAL_CONFIG_PATH = "/conf/redpen-conf.xml";
     private final static String DEFAULT_LANGUAGE = "default";
 
-    private static Map<String, RedPen> langRedPenMap = new HashMap<>();
+    static Map<String, RedPen> langRedPenMap = new HashMap<>();
 
     /**
      * Create redpens for the given context
@@ -60,15 +61,15 @@ public class RedPenService {
                 LOG.info("Creating RedPen instances");
                 try {
                     for (String fileName : findConfFiles()) {
-                        RedPen redPen = new RedPen(fileName);
-                        Configuration config = redPen.getConfiguration();
+                        Configuration config = new ConfigurationLoader().secure().loadFromResource(fileName);
+                        RedPen redPen = new RedPen(config);
                         langRedPenMap.put(config.getKey(), redPen);
                     }
 
                     String configPath = context != null ? context.getInitParameter("redpen.conf.path") : null;
                     if (configPath != null) {
                         LOG.info("Config Path is set to \"{}\"", configPath);
-                        RedPen defaultRedPen = new RedPen(configPath);
+                        RedPen defaultRedPen = new RedPen(new ConfigurationLoader().secure().loadFromResource(configPath));
                         langRedPenMap.put(DEFAULT_LANGUAGE, defaultRedPen);
                     } else {
                         // if config path is not set, fallback to default config path
@@ -108,8 +109,7 @@ public class RedPenService {
      * @return a configured redpen instance
      */
     public RedPen getRedPen(String lang, Map<String, Map<String, String>> validatorProperties) {
-        Configuration.ConfigurationBuilder configBuilder = Configuration.builder();
-        configBuilder.setLanguage(lang);
+        Configuration.ConfigurationBuilder configBuilder = Configuration.builder(lang).secure();
 
         // add the validators and their properties
         validatorProperties.forEach((validatorName, props) -> {


Test patch:
diff --git a/redpen-core/src/test/java/cc/redpen/config/ConfigurationTest.java b/redpen-core/src/test/java/cc/redpen/config/ConfigurationTest.java
index 7a90468d..669780fd 100644
--- a/redpen-core/src/test/java/cc/redpen/config/ConfigurationTest.java
+++ b/redpen-core/src/test/java/cc/redpen/config/ConfigurationTest.java
@@ -79,19 +79,19 @@ public class ConfigurationTest {
     @Test
     public void keyIsLangAndType() throws Exception {
         SymbolTable symbolTable = new SymbolTable("ja", Optional.of("hankaku"), emptyList());
-        assertEquals("ja.hankaku", new Configuration(new File(""), symbolTable, emptyList(), "ja").getKey());
+        assertEquals("ja.hankaku", new Configuration(new File(""), symbolTable, emptyList(), "ja", false).getKey());
     }
 
     @Test
     public void keyIsLangOnlyIfTypeIsMissing() throws Exception {
         SymbolTable symbolTable = new SymbolTable("en", Optional.empty(), emptyList());
-        assertEquals("en", new Configuration(new File(""), symbolTable, emptyList(), "en").getKey());
+        assertEquals("en", new Configuration(new File(""), symbolTable, emptyList(), "en", false).getKey());
     }
 
     @Test
     public void keyIsLangOnlyForZenkaku() throws Exception {
         SymbolTable symbolTable = new SymbolTable("ja", Optional.of("zenkaku"), emptyList());
-        assertEquals("ja", new Configuration(new File(""), symbolTable, emptyList(), "ja").getKey());
+        assertEquals("ja", new Configuration(new File(""), symbolTable, emptyList(), "ja", false).getKey());
     }
 
     @Test
@@ -131,7 +131,7 @@ public class ConfigurationTest {
             fail("Expecting RedPenException");
         }
         catch (RedPenException e) {
-            assertEquals("hello.xml is not under working directory (" + new File("").getAbsoluteFile() + ") or $REDPEN_HOME (" + new File("src").getAbsoluteFile() + ").", e.getMessage());
+            assertEquals("hello.xml is not under working directory (" + new File("").getAbsoluteFile() + "), $REDPEN_HOME (" + new File("src").getAbsoluteFile() + ").", e.getMessage());
         }
     }
 
@@ -143,7 +143,32 @@ public class ConfigurationTest {
             fail("Expecting RedPenException");
         }
         catch (RedPenException e) {
-            assertEquals("hello.xml is not under working directory (" + new File("").getAbsoluteFile() + "), base (some/base/dir) or $REDPEN_HOME (" + new File("src").getAbsoluteFile() + ").", e.getMessage());
+            assertEquals("hello.xml is not under working directory (" + new File("").getAbsoluteFile() + "), base (some/base/dir), $REDPEN_HOME (" + new File("src").getAbsoluteFile() + ").", e.getMessage());
+        }
+    }
+
+    @Test
+    public void findFile_workingDirectorySecureMode() throws Exception {
+        String localFile = new File(".").list()[0];
+        try {
+            System.setProperty("REDPEN_HOME", "");
+            Configuration.builder().secure().build().findFile(localFile);
+            fail("Secure mode should not allow files from working directory");
+        }
+        catch (RedPenException e) {
+            assertEquals(localFile + " is not under $REDPEN_HOME (" + new File("").getAbsoluteFile() + ").", e.getMessage());
+        }
+    }
+
+    @Test
+    public void findFile_secureMode() throws Exception {
+        try {
+            System.setProperty("REDPEN_HOME", "");
+            Configuration.builder().secure().build().findFile("/etc/passwd");
+            fail("Secure mode should not allow file locations outside config paths");
+        }
+        catch (RedPenException e) {
+            assertEquals("/etc/passwd is not under $REDPEN_HOME (" + new File("").getAbsoluteFile() + ").", e.getMessage());
         }
     }
 
diff --git a/redpen-server/src/test/java/cc/redpen/server/api/RedPenResourceTest.java b/redpen-server/src/test/java/cc/redpen/server/api/RedPenResourceTest.java
index 9d5958ca..a9eb14f8 100644
--- a/redpen-server/src/test/java/cc/redpen/server/api/RedPenResourceTest.java
+++ b/redpen-server/src/test/java/cc/redpen/server/api/RedPenResourceTest.java
@@ -30,6 +30,9 @@ import javax.ws.rs.core.MediaType;
 import java.io.FileNotFoundException;
 import java.util.Set;
 
+import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
+import static javax.ws.rs.core.MediaType.WILDCARD;
+
 public class RedPenResourceTest extends MockServletInvocationTest {
 
     @Override
@@ -45,7 +48,7 @@ public class RedPenResourceTest extends MockServletInvocationTest {
 
     public void testRun() throws Exception {
         MockHttpServletRequest request =
-                constructMockRequest("POST", "/document/validate", MediaType.WILDCARD);
+                constructMockRequest("POST", "/document/validate", WILDCARD);
         request.setContent(("document=Foobar").getBytes());
         MockServletContext context = new MockServletContext();
         context.addInitParameter("redpen.conf.path", "conf/redpen-conf.xml");
@@ -57,8 +60,7 @@ public class RedPenResourceTest extends MockServletInvocationTest {
     }
 
     public void testRunWithErrors() throws Exception {
-        MockHttpServletRequest request =
-                constructMockRequest("POST", "/document/validate", MediaType.WILDCARD);
+        MockHttpServletRequest request = constructMockRequest("POST", "/document/validate", WILDCARD);
         request.setContent(("document=foobar.foobar").getBytes()); //NOTE: need space between periods.
         MockServletContext context = new MockServletContext();
         context.addInitParameter("redpen.conf.path", "conf/redpen-conf.xml");
@@ -74,8 +76,7 @@ public class RedPenResourceTest extends MockServletInvocationTest {
     }
 
     public void testRunWithoutContent() throws Exception {
-        MockHttpServletRequest request =
-                constructMockRequest("POST", "/document/validate", MediaType.WILDCARD);
+        MockHttpServletRequest request = constructMockRequest("POST", "/document/validate", WILDCARD);
         request.setContent(("").getBytes()); //NOTE: need space between periods.
         MockServletContext context = new MockServletContext();
         context.addInitParameter("redpen.conf.path", "conf/redpen-conf.xml");
@@ -84,8 +85,7 @@ public class RedPenResourceTest extends MockServletInvocationTest {
     }
 
     public void testRunWithOnlyFormName() throws Exception {
-        MockHttpServletRequest request =
-                constructMockRequest("POST", "/document/validate", MediaType.WILDCARD);
+        MockHttpServletRequest request = constructMockRequest("POST", "/document/validate", WILDCARD);
         request.setContent(("document=").getBytes()); //NOTE: need space between periods.
         MockServletContext context = new MockServletContext();
         context.addInitParameter("redpen.conf.path", "conf/redpen-conf.xml");
@@ -95,21 +95,31 @@ public class RedPenResourceTest extends MockServletInvocationTest {
     }
 
     public void testJSValidatorRuns() throws Exception {
-        MockHttpServletRequest request =
-            constructMockRequest("POST", "/document/validate/json", MediaType.WILDCARD, MediaType.APPLICATION_JSON);
-        request.setContent(String.format("{\"document\":\"Test, this is a test.\",\"format\":\"json2\",\"documentParser\":\"PLAIN\",\"config\":{\"lang\":\"en\",\"validators\":{\"JavaScript\":{\"properties\":{\"script-path\":\"%s\"}}}}}", "./src/test/resources/js").getBytes());
-        MockServletContext context = new MockServletContext();
+        System.setProperty("REDPEN_HOME", "src/test");
+        MockHttpServletRequest request = constructMockRequest("POST", "/document/validate/json", WILDCARD, APPLICATION_JSON);
+        request.setContent(String.format("{\"document\":\"Test, this is a test.\",\"format\":\"json2\",\"documentParser\":\"PLAIN\",\"config\":{\"lang\":\"en\",\"validators\":{\"JavaScript\":{\"properties\":{\"script-path\":\"%s\"}}}}}", "resources/js").getBytes());
         MockHttpServletResponse response = invoke(request);
 
         assertEquals("HTTP status", HttpStatus.OK.getCode(), response.getStatus());
-        final JSONArray errors = new JSONObject(response.getContentAsString()).getJSONArray("errors");
+        JSONArray errors = new JSONObject(response.getContentAsString()).getJSONArray("errors");
         assertTrue(errors.length() > 0);
         for (int i=0; i<errors.length(); ++i) {
-            final JSONObject o = errors.getJSONObject(i).getJSONArray("errors").getJSONObject(0);
+            JSONObject o = errors.getJSONObject(i).getJSONArray("errors").getJSONObject(0);
             assertEquals("[pass.js] called", o.getString("message"));
         }
     }
 
+    public void testJSValidatorDoesntRunFromNonHomeDir() throws Exception {
+        System.setProperty("REDPEN_HOME", ".");
+        MockHttpServletRequest request = constructMockRequest("POST", "/document/validate/json", WILDCARD, APPLICATION_JSON);
+        request.setContent(String.format("{\"document\":\"Test, this is a test.\",\"format\":\"json2\",\"documentParser\":\"PLAIN\",\"config\":{\"lang\":\"en\",\"validators\":{\"JavaScript\":{\"properties\":{\"script-path\":\"%s\"}}}}}", "resources/js").getBytes());
+        MockHttpServletResponse response = invoke(request);
+
+        assertEquals("HTTP status", HttpStatus.OK.getCode(), response.getStatus());
+        JSONArray errors = new JSONObject(response.getContentAsString()).getJSONArray("errors");
+        assertEquals(0, errors.length());
+    }
+
     public void testDetectLanguage() throws Exception {
         assertEquals("en", new RedPenResource().detectLanguage("Hello World").getString("key"));
         assertEquals("ja", new RedPenResource().detectLanguage("こんにちは世界").getString("key"));
diff --git a/redpen-server/src/test/java/cc/redpen/server/api/RedPenServiceTest.java b/redpen-server/src/test/java/cc/redpen/server/api/RedPenServiceTest.java
index 5b60ffe6..618b8dff 100644
--- a/redpen-server/src/test/java/cc/redpen/server/api/RedPenServiceTest.java
+++ b/redpen-server/src/test/java/cc/redpen/server/api/RedPenServiceTest.java
@@ -1,13 +1,22 @@
 package cc.redpen.server.api;
 
 import cc.redpen.RedPen;
+import org.junit.Before;
 import org.junit.Test;
+import org.springframework.mock.web.MockServletContext;
 
 import java.util.Map;
 
+import static java.util.Collections.emptyMap;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 public class RedPenServiceTest {
+  @Before
+  public void setUp() {
+    RedPenService.langRedPenMap.clear();
+  }
+
   @Test
   public void twoConfigurationsByDefault() throws Exception {
     RedPenService service = new RedPenService(null);
@@ -24,5 +33,22 @@ public class RedPenServiceTest {
 
     assertEquals("ja", redPens.get("ja.hankaku").getConfiguration().getLang());
     assertEquals("hankaku", redPens.get("ja.hankaku").getConfiguration().getVariant());
+
+    assertTrue(redPens.values().stream().allMatch(r -> r.getConfiguration().isSecure()));
+  }
+
+  @Test
+  public void canSpecifyDifferentDefaultConfiguration() throws Exception {
+    MockServletContext context = new MockServletContext();
+    context.addInitParameter("redpen.conf.path", "/conf/redpen-conf-ja.xml");
+    RedPen defaultRedPen = new RedPenService(context).getRedPen("default");
+    assertEquals("ja", defaultRedPen.getConfiguration().getKey());
+    assertTrue(defaultRedPen.getConfiguration().isSecure());
+  }
+
+  @Test
+  public void redPensWithCustomPropertiesAreAlsoSecure() throws Exception {
+    RedPen en = new RedPenService(null).getRedPen("en", emptyMap());
+    assertTrue(en.getConfiguration().isSecure());
   }
 }
\ No newline at end of file