Merge pull request #1347 from jenkinsci/stable-1.5.x

Integrate 1.5.36 with SECURITY-2877 fix

pom.xml

@@ -14,7 +14,7 @@
   <packaging>hpi</packaging>
 
   <properties>
-    <revision>1.5.36</revision>
+    <revision>1.5.37</revision>
     <changelist>-SNAPSHOT</changelist>
     <jenkins.version>2.346.3</jenkins.version>
     <spotbugs.threshold>High</spotbugs.threshold> <!-- TODO fix violations -->

src/main/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookAction.java

@@ -1,10 +1,14 @@
 package com.dabsquared.gitlabjenkins.webhook.build;
 
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.logging.Logger;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+
 import hudson.model.Item;
 import hudson.model.Job;
-import hudson.security.Messages;
 import hudson.security.Permission;
 import hudson.util.HttpResponses;
 import jenkins.model.Jenkins;
@@ -34,21 +38,51 @@ public final void execute(StaplerResponse response) {
     protected abstract static class TriggerNotifier implements Runnable {
 
         private final Item project;
-        private final String secretToken;
+        private final byte[] hashedSecretToken;
         private final Authentication authentication;
 
         public TriggerNotifier(Item project, String secretToken, Authentication authentication) {
             this.project = project;
-            this.secretToken = secretToken;
+            /* secretToken may be null, but we want constant time comparison of tokens */
+            /* Remember secretToken was passed as null, then handle it as non-matchinng later */
+            this.hashedSecretToken = secretToken != null ? hashedBytes(secretToken) : null;
             this.authentication = authentication;
         }
 
+        @NonNull
+        private static byte[] hashedBytes(@NonNull String token) {
+            final String HASH_ALGORITHM = "SHA-256";
+            try {
+                MessageDigest digest = MessageDigest.getInstance(HASH_ALGORITHM);
+                return digest.digest(token.getBytes(StandardCharsets.UTF_8));
+            } catch (NoSuchAlgorithmException e) {
+                throw new AssertionError("Hash algorithm " + HASH_ALGORITHM + " not found", e);
+            }
+        }
+
+        /* Constant time comparison of token argument and secretToken that was
+         * passed to the constructor. If a null secretToken was passed to the
+         * constructor, this method must still perform constant time comparison.
+         */
+        private boolean tokenMatches(@NonNull String token) {
+            byte[] tokenBytes = hashedBytes(token);
+            if (hashedSecretToken != null) {
+                return MessageDigest.isEqual(tokenBytes, hashedSecretToken);
+            }
+
+            // assure the isEqual comparison compares same number of bytes
+            byte [] secretTokenBytes = tokenBytes.clone();
+            // change last byte to assure the isEqual comparison will not match
+            secretTokenBytes[secretTokenBytes.length - 1] ^= 1 << 3;
+            return MessageDigest.isEqual(tokenBytes, secretTokenBytes);
+        }
+
         public void run() {
             GitLabPushTrigger trigger = GitLabPushTrigger.getFromJob((Job<?, ?>) project);
             if (trigger != null) {
                 if (StringUtils.isEmpty(trigger.getSecretToken())) {
                     checkPermission(Item.BUILD, project);
-                } else if (!StringUtils.equals(trigger.getSecretToken(), secretToken)) {
+                } else if (!tokenMatches(trigger.getSecretToken())) {
                     throw HttpResponses.errorWithoutStack(401, "Invalid token");
                 }
                 performOnPost(trigger);

src/test/java/com/dabsquared/gitlabjenkins/webhook/build/BuildWebHookActionTest.java

@@ -0,0 +1,149 @@
+/*
+ * Test tokenMatches in the BuildWebHookAction class
+ * Author: Mark Waite
+ */
+package com.dabsquared.gitlabjenkins.webhook.build;
+
+import com.dabsquared.gitlabjenkins.connection.GitLabConnectionConfig;
+import com.dabsquared.gitlabjenkins.GitLabPushTrigger;
+
+import edu.umd.cs.findbugs.annotations.NonNull;
+
+import hudson.model.FreeStyleProject;
+import hudson.model.Item;
+import hudson.model.Project;
+import hudson.security.ACL;
+
+import org.acegisecurity.Authentication;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.Rule;
+import org.jvnet.hudson.test.JenkinsRule;
+
+import org.kohsuke.stapler.HttpResponses.HttpResponseException;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Test the BuildWebHookAction class
+ *
+ * @author Mark Waite
+ */
+public class BuildWebHookActionTest {
+
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    private FreeStyleProject project;
+    private GitLabPushTrigger trigger;
+
+    public BuildWebHookActionTest() {
+    }
+
+    @Before
+    public void confgureGitLabConnection() throws Exception {
+        j.get(GitLabConnectionConfig.class).setUseAuthenticatedEndpoint(true);
+    }
+
+    @Before
+    public void createFreeStyleProjectWithGitLabTrigger() throws Exception {
+        project = j.createFreeStyleProject();
+        trigger = new GitLabPushTrigger();
+        project.addTrigger(trigger);
+    }
+
+    // trigger token == action token, expected to succeed
+    @Test
+    public void testNotifierTokenMatches() throws Exception {
+        String triggerToken = "testNotifierTokenMatches-token";
+        trigger.setSecretToken(triggerToken);
+        String actionToken = triggerToken;
+        BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
+        action.runNotifier();
+        assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled);
+    }
+
+    // trigger token != action token, expected to throw an exception
+    @Test
+    public void testNotifierTokenDoesNotMatchString() throws Exception {
+        String triggerToken = "testNotifierTokenDoesNotMatchString-token";
+        trigger.setSecretToken(triggerToken);
+        String actionToken = triggerToken + "-no-match"; // Won't match
+        BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
+        assertThrows(HttpResponseException.class,
+                () -> {
+                    action.runNotifier();
+                }
+        );
+        assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled);
+    }
+
+    // trigger token != null action token, expected to throw an exception
+    @Test
+    public void testNotifierTokenDoesNotMatchNull() throws Exception {
+        String triggerToken = "testNotifierTokenDoesNotMatchNull-token";
+        trigger.setSecretToken(triggerToken);
+        String actionToken = null;
+        BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
+        assertThrows(HttpResponseException.class,
+                () -> {
+                    action.runNotifier();
+                }
+        );
+        assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled);
+    }
+
+    // null trigger token != action token, expected to succeed
+    @Test
+    public void testNullNotifierTokenAllowsAccess() throws Exception {
+        // String triggerToken = null;
+        // trigger.setSecretToken(triggerToken);
+        String actionToken = "testNullNotifierTokenAllowsAccess-token";
+        BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
+        action.runNotifier();
+        assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled);
+    }
+
+    public class BuildWebHookActionImpl extends BuildWebHookAction {
+
+        // Used for the assertion that tokenMatches() returned true
+        public boolean performOnPostCalled = false;
+
+        private final MyTriggerNotifier myNotifier;
+
+        public BuildWebHookActionImpl() {
+            myNotifier = new MyTriggerNotifier(null, null, null);
+        }
+
+        public BuildWebHookActionImpl(@NonNull Project project, @NonNull String token) {
+            myNotifier = new MyTriggerNotifier(project, token, ACL.SYSTEM);
+        }
+
+        public void runNotifier() {
+            myNotifier.run();
+        }
+
+        public class MyTriggerNotifier extends TriggerNotifier {
+
+            public MyTriggerNotifier(Item project, String secretToken, Authentication authentication) {
+                super(project, secretToken, authentication);
+            }
+
+            @Override
+            protected void performOnPost(GitLabPushTrigger trigger) {
+                performOnPostCalled = true;
+            }
+        }
+
+        @Override
+        public void processForCompatibility() {
+        }
+
+        @Override
+        public void execute() {
+        }
+    }
+}