Skip to content

Commit 24130a1

Browse files
MarkEWaiteMarkEWaite
authored
Merge pull request #1347 from jenkinsci/stable-1.5.x
Integrate 1.5.36 with SECURITY-2877 fix
2 parents f6ff8aa + a52a546 commit 24130a1

File tree

3 files changed

+188
-5
lines changed

3 files changed

+188
-5
lines changed

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
<packaging>hpi</packaging>
1515

1616
<properties>
17-
<revision>1.5.36</revision>
17+
<revision>1.5.37</revision>
1818
<changelist>-SNAPSHOT</changelist>
1919
<jenkins.version>2.346.3</jenkins.version>
2020
<spotbugs.threshold>High</spotbugs.threshold> <!-- TODO fix violations -->

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

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
package com.dabsquared.gitlabjenkins.webhook.build;
22

3+
import java.nio.charset.StandardCharsets;
4+
import java.security.MessageDigest;
5+
import java.security.NoSuchAlgorithmException;
36
import java.util.logging.Logger;
47

8+
import edu.umd.cs.findbugs.annotations.NonNull;
9+
510
import hudson.model.Item;
611
import hudson.model.Job;
7-
import hudson.security.Messages;
812
import hudson.security.Permission;
913
import hudson.util.HttpResponses;
1014
import jenkins.model.Jenkins;
@@ -34,21 +38,51 @@ public final void execute(StaplerResponse response) {
3438
protected abstract static class TriggerNotifier implements Runnable {
3539

3640
private final Item project;
37-
private final String secretToken;
41+
private final byte[] hashedSecretToken;
3842
private final Authentication authentication;
3943

4044
public TriggerNotifier(Item project, String secretToken, Authentication authentication) {
4145
this.project = project;
42-
this.secretToken = secretToken;
46+
/* secretToken may be null, but we want constant time comparison of tokens */
47+
/* Remember secretToken was passed as null, then handle it as non-matchinng later */
48+
this.hashedSecretToken = secretToken != null ? hashedBytes(secretToken) : null;
4349
this.authentication = authentication;
4450
}
4551

52+
@NonNull
53+
private static byte[] hashedBytes(@NonNull String token) {
54+
final String HASH_ALGORITHM = "SHA-256";
55+
try {
56+
MessageDigest digest = MessageDigest.getInstance(HASH_ALGORITHM);
57+
return digest.digest(token.getBytes(StandardCharsets.UTF_8));
58+
} catch (NoSuchAlgorithmException e) {
59+
throw new AssertionError("Hash algorithm " + HASH_ALGORITHM + " not found", e);
60+
}
61+
}
62+
63+
/* Constant time comparison of token argument and secretToken that was
64+
* passed to the constructor. If a null secretToken was passed to the
65+
* constructor, this method must still perform constant time comparison.
66+
*/
67+
private boolean tokenMatches(@NonNull String token) {
68+
byte[] tokenBytes = hashedBytes(token);
69+
if (hashedSecretToken != null) {
70+
return MessageDigest.isEqual(tokenBytes, hashedSecretToken);
71+
}
72+
73+
// assure the isEqual comparison compares same number of bytes
74+
byte [] secretTokenBytes = tokenBytes.clone();
75+
// change last byte to assure the isEqual comparison will not match
76+
secretTokenBytes[secretTokenBytes.length - 1] ^= 1 << 3;
77+
return MessageDigest.isEqual(tokenBytes, secretTokenBytes);
78+
}
79+
4680
public void run() {
4781
GitLabPushTrigger trigger = GitLabPushTrigger.getFromJob((Job<?, ?>) project);
4882
if (trigger != null) {
4983
if (StringUtils.isEmpty(trigger.getSecretToken())) {
5084
checkPermission(Item.BUILD, project);
51-
} else if (!StringUtils.equals(trigger.getSecretToken(), secretToken)) {
85+
} else if (!tokenMatches(trigger.getSecretToken())) {
5286
throw HttpResponses.errorWithoutStack(401, "Invalid token");
5387
}
5488
performOnPost(trigger);

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

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
/*
2+
* Test tokenMatches in the BuildWebHookAction class
3+
* Author: Mark Waite
4+
*/
5+
package com.dabsquared.gitlabjenkins.webhook.build;
6+
7+
import com.dabsquared.gitlabjenkins.connection.GitLabConnectionConfig;
8+
import com.dabsquared.gitlabjenkins.GitLabPushTrigger;
9+
10+
import edu.umd.cs.findbugs.annotations.NonNull;
11+
12+
import hudson.model.FreeStyleProject;
13+
import hudson.model.Item;
14+
import hudson.model.Project;
15+
import hudson.security.ACL;
16+
17+
import org.acegisecurity.Authentication;
18+
19+
import org.junit.Before;
20+
import org.junit.Test;
21+
import org.junit.Rule;
22+
import org.jvnet.hudson.test.JenkinsRule;
23+
24+
import org.kohsuke.stapler.HttpResponses.HttpResponseException;
25+
26+
import static org.junit.Assert.assertFalse;
27+
import static org.junit.Assert.assertThrows;
28+
import static org.junit.Assert.assertTrue;
29+
30+
/**
31+
* Test the BuildWebHookAction class
32+
*
33+
* @author Mark Waite
34+
*/
35+
public class BuildWebHookActionTest {
36+
37+
@Rule
38+
public JenkinsRule j = new JenkinsRule();
39+
40+
private FreeStyleProject project;
41+
private GitLabPushTrigger trigger;
42+
43+
public BuildWebHookActionTest() {
44+
}
45+
46+
@Before
47+
public void confgureGitLabConnection() throws Exception {
48+
j.get(GitLabConnectionConfig.class).setUseAuthenticatedEndpoint(true);
49+
}
50+
51+
@Before
52+
public void createFreeStyleProjectWithGitLabTrigger() throws Exception {
53+
project = j.createFreeStyleProject();
54+
trigger = new GitLabPushTrigger();
55+
project.addTrigger(trigger);
56+
}
57+
58+
// trigger token == action token, expected to succeed
59+
@Test
60+
public void testNotifierTokenMatches() throws Exception {
61+
String triggerToken = "testNotifierTokenMatches-token";
62+
trigger.setSecretToken(triggerToken);
63+
String actionToken = triggerToken;
64+
BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
65+
action.runNotifier();
66+
assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled);
67+
}
68+
69+
// trigger token != action token, expected to throw an exception
70+
@Test
71+
public void testNotifierTokenDoesNotMatchString() throws Exception {
72+
String triggerToken = "testNotifierTokenDoesNotMatchString-token";
73+
trigger.setSecretToken(triggerToken);
74+
String actionToken = triggerToken + "-no-match"; // Won't match
75+
BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
76+
assertThrows(HttpResponseException.class,
77+
() -> {
78+
action.runNotifier();
79+
}
80+
);
81+
assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled);
82+
}
83+
84+
// trigger token != null action token, expected to throw an exception
85+
@Test
86+
public void testNotifierTokenDoesNotMatchNull() throws Exception {
87+
String triggerToken = "testNotifierTokenDoesNotMatchNull-token";
88+
trigger.setSecretToken(triggerToken);
89+
String actionToken = null;
90+
BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
91+
assertThrows(HttpResponseException.class,
92+
() -> {
93+
action.runNotifier();
94+
}
95+
);
96+
assertFalse("performOnPost was called, unexpected token match?", action.performOnPostCalled);
97+
}
98+
99+
// null trigger token != action token, expected to succeed
100+
@Test
101+
public void testNullNotifierTokenAllowsAccess() throws Exception {
102+
// String triggerToken = null;
103+
// trigger.setSecretToken(triggerToken);
104+
String actionToken = "testNullNotifierTokenAllowsAccess-token";
105+
BuildWebHookActionImpl action = new BuildWebHookActionImpl(project, actionToken);
106+
action.runNotifier();
107+
assertTrue("performOnPost not called, token did not match?", action.performOnPostCalled);
108+
}
109+
110+
public class BuildWebHookActionImpl extends BuildWebHookAction {
111+
112+
// Used for the assertion that tokenMatches() returned true
113+
public boolean performOnPostCalled = false;
114+
115+
private final MyTriggerNotifier myNotifier;
116+
117+
public BuildWebHookActionImpl() {
118+
myNotifier = new MyTriggerNotifier(null, null, null);
119+
}
120+
121+
public BuildWebHookActionImpl(@NonNull Project project, @NonNull String token) {
122+
myNotifier = new MyTriggerNotifier(project, token, ACL.SYSTEM);
123+
}
124+
125+
public void runNotifier() {
126+
myNotifier.run();
127+
}
128+
129+
public class MyTriggerNotifier extends TriggerNotifier {
130+
131+
public MyTriggerNotifier(Item project, String secretToken, Authentication authentication) {
132+
super(project, secretToken, authentication);
133+
}
134+
135+
@Override
136+
protected void performOnPost(GitLabPushTrigger trigger) {
137+
performOnPostCalled = true;
138+
}
139+
}
140+
141+
@Override
142+
public void processForCompatibility() {
143+
}
144+
145+
@Override
146+
public void execute() {
147+
}
148+
}
149+
}

0 commit comments

Comments
 (0)