Merge commit from fork

fix(loader): confine template resource loaders to their root (GHSA-fxwq-p235-p87r)

Author: jasmith-hs

src/main/java/com/hubspot/jinjava/loader/ClasspathResourceLocator.java

@@ -4,6 +4,8 @@
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import java.io.IOException;
 import java.nio.charset.Charset;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 
 public class ClasspathResourceLocator implements ResourceLocator {
 
@@ -13,10 +15,30 @@ public String getString(
     Charset encoding,
     JinjavaInterpreter interpreter
   ) throws IOException {
+    if (escapesClasspathRoot(fullName)) {
+      throw new ResourceNotFoundException("Path escapes classpath root: " + fullName);
+    }
+
     try {
       return Resources.toString(Resources.getResource(fullName), encoding);
     } catch (IllegalArgumentException e) {
       throw new ResourceNotFoundException("Couldn't find resource: " + fullName);
     }
   }
+
+  private static boolean escapesClasspathRoot(String fullName) {
+    Path path = Paths.get(fullName);
+
+    if (path.isAbsolute()) {
+      return true;
+    }
+
+    for (Path segment : path) {
+      if (segment.toString().equals("..")) {
+        return true;
+      }
+    }
+
+    return false;
+  }
 }

src/main/java/com/hubspot/jinjava/loader/FileLocator.java

@@ -21,6 +21,7 @@
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.nio.charset.Charset;
+import java.nio.file.Path;
 
 public class FileLocator implements ResourceLocator {
 
@@ -42,14 +43,15 @@ public FileLocator(File baseDir) throws FileNotFoundException {
     this.baseDir = baseDir;
   }
 
-  private File resolveFileName(String name) {
-    File f = new File(name);
+  private File resolveFileName(String name) throws IOException {
+    Path root = baseDir.getCanonicalFile().toPath();
+    File resolved = root.resolve(name).normalize().toFile().getCanonicalFile();
 
-    if (f.isAbsolute()) {
-      return f;
+    if (!resolved.toPath().startsWith(root)) {
+      throw new ResourceNotFoundException("Path escapes template root: " + name);
     }
 
-    return new File(baseDir, name);
+    return resolved;
   }
 
   @Override

src/test/java/com/hubspot/jinjava/loader/ClasspathResourceLocatorTest.java

@@ -21,4 +21,16 @@ public void testLoadFromClasspath() throws Exception {
   public void itThrowsNotFoundWhenNotFound() throws Exception {
     new ClasspathResourceLocator().getString("foo", StandardCharsets.UTF_8, interpreter);
   }
+
+  @Test(expected = ResourceNotFoundException.class)
+  public void itBlocksDotDotTraversal() throws Exception {
+    new ClasspathResourceLocator()
+      .getString("loader/cp/foo/../../dummy.html", StandardCharsets.UTF_8, interpreter);
+  }
+
+  @Test(expected = ResourceNotFoundException.class)
+  public void itBlocksAbsolutePaths() throws Exception {
+    new ClasspathResourceLocator()
+      .getString("/loader/cp/foo/bar.jinja", StandardCharsets.UTF_8, interpreter);
+  }
 }

src/test/java/com/hubspot/jinjava/loader/FileLocatorTest.java

@@ -2,31 +2,30 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-import com.google.common.io.Files;
 import com.hubspot.jinjava.BaseInterpretingTest;
-import com.hubspot.jinjava.Jinjava;
-import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import org.junit.Before;
 import org.junit.Test;
 
 public class FileLocatorTest extends BaseInterpretingTest {
 
   FileLocator locatorWorkingDir;
   FileLocator locatorTmpDir;
+  FileLocator locatorConfined;
 
   File first;
   File second;
+  File inside;
+  File secretOutside;
 
   @Before
   public void setUp() throws Exception {
     locatorWorkingDir = new FileLocator();
 
-    File tmpDir = java.nio.file.Files
-      .createTempDirectory(getClass().getSimpleName())
-      .toFile();
+    File tmpDir = Files.createTempDirectory(getClass().getSimpleName()).toFile();
     locatorTmpDir = new FileLocator(tmpDir);
 
     first = new File(tmpDir, "foo/first.jinja");
@@ -35,8 +34,21 @@ public void setUp() throws Exception {
     first.getParentFile().mkdirs();
     second.getParentFile().mkdirs();
 
-    Files.write("first", first, StandardCharsets.UTF_8);
-    Files.write("second", second, StandardCharsets.UTF_8);
+    Files.writeString(first.toPath(), "first");
+    Files.writeString(second.toPath(), "second");
+
+    File parent = Files
+      .createTempDirectory(getClass().getSimpleName() + "-confined")
+      .toFile();
+    File confinedDir = new File(parent, "base");
+    confinedDir.mkdirs();
+    locatorConfined = new FileLocator(confinedDir);
+
+    inside = new File(confinedDir, "inside.jinja");
+    Files.writeString(inside.toPath(), "inside");
+
+    secretOutside = new File(parent, "secret.txt");
+    Files.writeString(secretOutside.toPath(), "TOP_SECRET_FROM_OUTSIDE_DIR");
   }
 
   @Test
@@ -102,4 +114,38 @@ public void testNotFoundRel() throws Exception {
   public void testNotFoundAbs() throws Exception {
     locatorWorkingDir.getString("/blargh", StandardCharsets.UTF_8, interpreter);
   }
+
+  @Test
+  public void itAllowsInRootRelativePath() throws Exception {
+    assertThat(
+      locatorConfined.getString("inside.jinja", StandardCharsets.UTF_8, interpreter)
+    )
+      .isEqualTo("inside");
+  }
+
+  @Test
+  public void itAllowsInRootAbsolutePath() throws Exception {
+    assertThat(
+      locatorConfined.getString(
+        inside.getAbsolutePath(),
+        StandardCharsets.UTF_8,
+        interpreter
+      )
+    )
+      .isEqualTo("inside");
+  }
+
+  @Test(expected = ResourceNotFoundException.class)
+  public void itBlocksDotDotTraversalOutsideRoot() throws Exception {
+    locatorConfined.getString("../secret.txt", StandardCharsets.UTF_8, interpreter);
+  }
+
+  @Test(expected = ResourceNotFoundException.class)
+  public void itBlocksAbsolutePathOutsideRoot() throws Exception {
+    locatorConfined.getString(
+      secretOutside.getAbsolutePath(),
+      StandardCharsets.UTF_8,
+      interpreter
+    );
+  }
 }

src/test/java/com/hubspot/jinjava/loader/LoaderPathTraversalTest.java

@@ -0,0 +1,65 @@
+package com.hubspot.jinjava.loader;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.hubspot.jinjava.Jinjava;
+import com.hubspot.jinjava.interpret.RenderResult;
+import java.io.File;
+import java.nio.file.Files;
+import java.util.HashMap;
+import org.junit.Before;
+import org.junit.Test;
+
+public class LoaderPathTraversalTest {
+
+  private static final String SECRET = "TOP_SECRET_FROM_OUTSIDE_DIR";
+
+  private Jinjava jinjava;
+  private File secretOutside;
+
+  @Before
+  public void setUp() throws Exception {
+    File parent = Files.createTempDirectory(getClass().getSimpleName()).toFile();
+    File baseDir = new File(parent, "base");
+    baseDir.mkdirs();
+
+    secretOutside = new File(parent, "secret.txt");
+    Files.writeString(secretOutside.toPath(), SECRET);
+
+    jinjava = new Jinjava();
+    jinjava.setResourceLocator(new FileLocator(baseDir));
+  }
+
+  private String render(String template) {
+    RenderResult result = jinjava.renderForResult(template, new HashMap<>());
+    return result.getOutput();
+  }
+
+  @Test
+  public void itDoesNotDiscloseFileViaIncludeTraversal() {
+    assertThat(render("{% include \"../secret.txt\" %}")).doesNotContain(SECRET);
+  }
+
+  @Test
+  public void itDoesNotDiscloseFileViaExtendsTraversal() {
+    assertThat(render("{% extends \"../secret.txt\" %}")).doesNotContain(SECRET);
+  }
+
+  @Test
+  public void itDoesNotDiscloseFileViaImportTraversal() {
+    assertThat(render("{% import \"../secret.txt\" as leaked %}{{ leaked }}"))
+      .doesNotContain(SECRET);
+  }
+
+  @Test
+  public void itDoesNotDiscloseFileViaFromTraversal() {
+    assertThat(render("{% from \"../secret.txt\" import leaked %}{{ leaked }}"))
+      .doesNotContain(SECRET);
+  }
+
+  @Test
+  public void itDoesNotDiscloseFileViaAbsolutePathOutsideRoot() {
+    assertThat(render("{% include \"" + secretOutside.getAbsolutePath() + "\" %}"))
+      .doesNotContain(SECRET);
+  }
+}