CVE-2018-1000997

@@ -39,7 +39,7 @@ THE SOFTWARE.
 
   <properties>
     <staplerFork>true</staplerFork>
-    <stapler.version>1.250.1</stapler.version>
+    <stapler.version>1.250.2</stapler.version>
     <spring.version>2.5.6.SEC03</spring.version>
     <groovy.version>2.4.11</groovy.version>
     <!-- TODO: Actually many issues are being filtered by src/findbugs/findbugs-excludes.xml -->

@@ -0,0 +1,167 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2018, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+package jenkins.security.stapler;
+
+import com.gargoylesoftware.htmlunit.Page;
+import hudson.model.RootAction;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.TestExtension;
+
+import javax.annotation.CheckForNull;
+
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.junit.Assert.assertThat;
+
+public class Security867Test {
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+    
+    @Test
+    @Issue("SECURITY-867")
+    public void folderTraversalPrevented_avoidStealingSecretInView() throws Exception {
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+        
+        String publicContent = "Test OK";
+        String secretContent = "s3cr3t";
+        
+        // to validate the attack reproduction you can disable the protection
+        // Facet.ALLOW_VIEW_NAME_PATH_TRAVERSAL = true;
+        
+        // regular behavior
+        assertThat(getContentAndCheck200(wc, "rootAction1/public"), containsString(publicContent));
+        
+        // malicious usage prevention
+        
+        // looking for /jenkins/security/stapler/Security867Test/NotRootAction2/secret
+        assertThat(getContent(wc, "rootAction1/%2fjenkins%2fsecurity%2fstapler%2fSecurity867Test%2fNotRootAction2%2fsecret"), 
+                not(containsString(secretContent)));
+            
+        // looking for /jenkins\security\stapler\Security867Test\NotRootAction2\secret => 
+        // absolute path with backslash (initial forward one is required for absolute)
+        assertThat(getContent(wc,"rootAction1/%2fjenkins%5csecurity%5cstapler%5cSecurity867Test%5cNotRootAction2%5csecret"), 
+                not(containsString(secretContent)));
+            
+        // looking for ../NotRootAction2/secret => relative path
+        assertThat(getContent(wc,"rootAction1/%2e%2e%2fNotRootAction2%2fsecret"), 
+                not(containsString(secretContent)));
+            
+        // looking for ..\NotRootAction2\secret => relative path without forward slash
+        assertThat(getContent(wc, "rootAction1/%2e%2e%5cNotRootAction2%5csecret"),
+                not(containsString(secretContent)));
+    }
+    
+    private String getContent(JenkinsRule.WebClient wc, String url) throws Exception {
+        Page page = wc.goTo(url, null);
+        return page.getWebResponse().getContentAsString();
+    }
+    
+    private String getContentAndCheck200(JenkinsRule.WebClient wc, String url) throws Exception {
+        Page page = wc.goTo(url, null);
+        assertThat(page.getWebResponse().getStatusCode(), equalTo(200));
+        return page.getWebResponse().getContentAsString();
+    }
+    
+    @Test
+    @Issue("SECURITY-867")
+    public void folderTraversalPrevented_avoidStealingSecretFromDifferentObject() throws Exception {
+        JenkinsRule.WebClient wc = j.createWebClient();
+        wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+        
+        String action1Config = j.jenkins.getExtensionList(RootAction.class).get(RootAction1.class).getMyConfig();
+        String action3Config = j.jenkins.getExtensionList(RootAction.class).get(RootAction3.class).getMyConfig();
+        
+        // to validate the attack reproduction you can disable the protection
+        // Facet.ALLOW_VIEW_NAME_PATH_TRAVERSAL = true;
+        
+        // regular behavior, the config is only displayed in ActionRoot3
+        assertThat(getContentAndCheck200(wc, "rootAction1/public"), not(containsString(action1Config)));
+        assertThat(getContentAndCheck200(wc, "rootAction3/showConfig"), allOf(
+                containsString(action3Config),
+                not(containsString(action1Config))
+        ));
+        
+        // the main point here is the last node visited will be "it" for the view scope
+        // if we navigate by RootAction1, we pass it to the RootAction3's view
+        
+        // malicious usage prevention, looking for ../RootAction3/showConfig => relative path
+        // without the prevention, the config value of RootAction1 will be used here
+        assertThat(getContent(wc, "rootAction1/%2e%2e%2fRootAction3%2fshowConfig"), allOf(
+                not(containsString(action1Config)),
+                not(containsString(action3Config))
+        ));
+    }
+    
+    @TestExtension
+    public static class RootAction1 implements RootAction {
+        // not displayed in its own public.jelly
+        public String getMyConfig() {
+            return "config-1";
+        }
+        
+        @Override
+        public @CheckForNull String getIconFileName() {
+            return null;
+        }
+        
+        @Override
+        public @CheckForNull String getDisplayName() {
+            return null;
+        }
+        
+        @Override
+        public @CheckForNull String getUrlName() {
+            return "rootAction1";
+        }
+    }
+    
+    @TestExtension
+    public static class RootAction3 implements RootAction {
+        // displayed in its showConfig.jelly
+        public String getMyConfig() {
+            return "config-3";
+        }
+        
+        @Override
+        public @CheckForNull String getIconFileName() {
+            return null;
+        }
+        
+        @Override
+        public @CheckForNull String getDisplayName() {
+            return null;
+        }
+        
+        @Override
+        public @CheckForNull String getUrlName() {
+            return "rootAction3";
+        }
+    }
+}

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core">
+    <p>s3cr3t</p>
+</j:jelly>

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core">
+    <p>Test OK</p>
+</j:jelly>

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core">
+    <p>Config: ${it.myConfig}</p>
+</j:jelly>

@@ -110,6 +110,8 @@ public static <T> List<T> discoverExtensions(Class<T> type, ClassLoader... cls)
 
     public static final Logger LOGGER = Logger.getLogger(Facet.class.getName());
 
+    public static boolean ALLOW_VIEW_NAME_PATH_TRAVERSAL = Boolean.getBoolean(Facet.class.getName() + ".allowViewNamePathTraversal");
+    
     /**
      * Creates a {@link RequestDispatcher} that handles the given view, or
      * return null if no such view was found.
@@ -144,4 +146,20 @@ public RequestDispatcher createRequestDispatcher(RequestImpl request, Class type
     public Klass<?> getKlass(Object o) {
         return null;
     }
+    
+    /**
+     * Ensure the path that is passed is only the name of the file and not a path
+     */
+    protected boolean isBasename(String potentialPath){
+        if (ALLOW_VIEW_NAME_PATH_TRAVERSAL) {
+            return true;
+        } else {
+            if (potentialPath.contains("\\") || potentialPath.contains("/")) {
+                // prevent absolute path and folder traversal to find scripts
+                return false;
+            }
+            
+            return true;
+        }
+    }
 }

@@ -68,6 +68,11 @@ public boolean dispatch(RequestImpl req, ResponseImpl rsp, Object node) throws I
                 // and avoid serving both "foo" and "foo/" as relative URL semantics are drastically different
                 if (req.getRequestURI().endsWith("/"))      return false;
 
+                if (!isBasename(next)) {
+                    // potentially an attempt to make a folder traversal
+                    return false;
+                }
+
                 try {
                     Script script = tearOff.findScript(next);
                     if(script==null)
@@ -92,6 +97,7 @@ public boolean dispatch(RequestImpl req, ResponseImpl rsp, Object node) throws I
                 }
             }
             public String toString() {
+                // or TOKEN.gsp
                 return "TOKEN.groovy for url=/TOKEN";
             }
         });

@@ -77,6 +77,11 @@ public boolean dispatch(RequestImpl req, ResponseImpl rsp, Object node) throws I
                 // and avoid serving both "foo" and "foo/" as relative URL semantics are drastically different
                 if (req.tokens.endsWithSlash)      return false;
 
+                if (!isBasename(next)) {
+                    // potentially an attempt to make a folder traversal
+                    return false;
+                }
+
                 try {
                     Script script = tearOff.findScript(next+".jelly");
 

@@ -120,8 +120,20 @@ public boolean dispatch(RequestImpl req, ResponseImpl rsp, Object node) throws I
                     if (req.tokens.countRemainingTokens()>1)    return false;
                     // and avoid serving both "foo" and "foo/" as relative URL semantics are drastically different
                     if (req.getRequestURI().endsWith("/"))      return false;
-
-                    return invokeScript(req, rsp, node, next, tearOff.findScript(next));
+                    
+                    if (!isBasename(next)) {
+                        // potentially an attempt to make a folder traversal
+                        return false;
+                    }
+    
+                    Script script = tearOff.findScript(next);
+    
+                    if (script == null) {
+                        // no script found
+                        return false;
+                    }
+                    
+                    return invokeScript(req, rsp, node, next, script);
                 }
             });
         }

@@ -59,6 +59,11 @@ public boolean dispatch(RequestImpl req, ResponseImpl rsp, Object node) throws I
 
                 Stapler stapler = req.getStapler();
 
+                if (!isBasename(next)) {
+                    // potentially an attempt to make a folder traversal
+                    return false;
+                }
+
                 // check static resources
                 RequestDispatcher disp = createRequestDispatcher(req,node.getClass(),node,next);
                 if(disp==null) {