Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-26838

Utility API to check if a file is physically inside a given directory

    • Icon: Improvement Improvement
    • Resolution: Unresolved
    • Icon: Minor Minor
    • core

      Commonly need to verify that file paths provided by a user are really relative and do not refer to locations outside a workspace or the like. Should have something in Util along the lines of

      public static boolean isInside(File root, File f) throws IOException {
          String path = f.getCanonicalPath();
          String rootPath = root.getCanonicalPath();
          return path.equals(rootPath) || path.startsWith(rootPath + File.separatorChar);
      }
      

      and a matching method in FilePath.

          [JENKINS-26838] Utility API to check if a file is physically inside a given directory

          Code changed in jenkins
          User: Jesse Glick
          Path:
          cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsScmFlowDefinition.java
          http://jenkins-ci.org/commit/workflow-plugin/c6b0c9fa4362b9f8d0903fdc0b46258f465e7210
          Log:
          JENKINS-26838 Noting.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsScmFlowDefinition.java http://jenkins-ci.org/commit/workflow-plugin/c6b0c9fa4362b9f8d0903fdc0b46258f465e7210 Log: JENKINS-26838 Noting.

          Daniel Beck added a comment -

          Daniel Beck added a comment - Could probably just use http://commons.apache.org/proper/commons-io/javadocs/api-release/org/apache/commons/io/FileUtils.html#directoryContains%28java.io.File,%20java.io.File%29 as Commons-IO 2.4 is a dependency of Jenkins/core.

          Jesse Glick added a comment -

          Does not suffice. First of all, it assumes the child exists, which many legitimate use cases cannot assume. Second, it is buggy:

          $ mkdir -p /tmp/basedir && touch /tmp/basedir-attacked && jrunscript -classpath ~/.m2/repository/commons-io/commons-io/2.4/commons-io-2.4.jar -e 'println(org.apache.commons.io.FileUtils.directoryContains(new java.io.File("/tmp/basedir"), new java.io.File("/tmp/basedir-attacked")))'
          true
          

          Jesse Glick added a comment - Does not suffice. First of all, it assumes the child exists, which many legitimate use cases cannot assume. Second, it is buggy: $ mkdir -p /tmp/basedir && touch /tmp/basedir-attacked && jrunscript -classpath ~/.m2/repository/commons-io/commons-io/2.4/commons-io-2.4.jar -e 'println(org.apache.commons.io.FileUtils.directoryContains( new java.io.File( "/tmp/basedir" ), new java.io.File( "/tmp/basedir-attacked" )))' true

          Code changed in jenkins
          User: Jesse Glick
          Path:
          cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsScmFlowDefinition.java
          http://jenkins-ci.org/commit/workflow-cps-plugin/3eaebdc53a1e0a5c7b97f9f2bca4956bf66c991d
          Log:
          JENKINS-26838 Noting.

          Originally-Committed-As: c6b0c9fa4362b9f8d0903fdc0b46258f465e7210

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: cps/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsScmFlowDefinition.java http://jenkins-ci.org/commit/workflow-cps-plugin/3eaebdc53a1e0a5c7b97f9f2bca4956bf66c991d Log: JENKINS-26838 Noting. Originally-Committed-As: c6b0c9fa4362b9f8d0903fdc0b46258f465e7210

          Jesse Glick added a comment -

          JENKINS-44657 introduced Util.isDescendant, which is close but not suitable for security purposes due to lack of canonicalization (Path.toRealPath).

          Jesse Glick added a comment - JENKINS-44657 introduced Util.isDescendant , which is close but not suitable for security purposes due to lack of canonicalization ( Path.toRealPath ).

            Unassigned Unassigned
            jglick Jesse Glick
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: