Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hash escape hatch password in configuration - fix CVE-2023-50770 #287

Merged
merged 1 commit into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import java.util.Random;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import jenkins.security.SecurityListener;
Expand Down Expand Up @@ -109,6 +110,7 @@
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.security.crypto.bcrypt.BCrypt;

import static org.apache.commons.lang.StringUtils.isNotBlank;

Expand Down Expand Up @@ -254,7 +256,7 @@
this.postLogoutRedirectUrl = postLogoutRedirectUrl;
this.escapeHatchEnabled = Util.fixNull(escapeHatchEnabled, Boolean.FALSE);
this.escapeHatchUsername = Util.fixEmpty(escapeHatchUsername);
this.escapeHatchSecret = Secret.fromString(escapeHatchSecret);
this.setEscapeHatchSecret(Secret.fromString(escapeHatchSecret));
this.escapeHatchGroup = Util.fixEmpty(escapeHatchGroup);
}

Expand Down Expand Up @@ -309,6 +311,8 @@
this.setEmailFieldName(this.emailFieldName);
this.setFullNameFieldName(this.fullNameFieldName);
this.setTokenFieldToCheckKey(this.tokenFieldToCheckKey);
// ensure escapeHatchSecret is encrypted
this.setEscapeHatchSecret(this.escapeHatchSecret);
return this;
}

Expand Down Expand Up @@ -627,9 +631,25 @@

@DataBoundSetter
public void setEscapeHatchSecret(Secret escapeHatchSecret) {
if (escapeHatchSecret != null) {

Check warning on line 634 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 634 is only partially covered, one branch is missing
// ensure escapeHatchSecret is BCrypt hash
String escapeHatchString = Secret.toString(escapeHatchSecret);

final Pattern BCryptPattern = Pattern.compile("\\A\\$[^$]+\\$\\d+\\$[./0-9A-Za-z]{53}");
if (!BCryptPattern.matcher(escapeHatchString).matches()) {
this.escapeHatchSecret = Secret.fromString(BCrypt.hashpw(escapeHatchString, BCrypt.gensalt()));
return;
}
}
this.escapeHatchSecret = escapeHatchSecret;
}

protected boolean checkEscapeHatch(String username, String password) {
final boolean isUsernameMatch = username.equals(this.escapeHatchUsername);
final boolean isPasswordMatch = BCrypt.checkpw(password, Secret.toString(this.escapeHatchSecret));
return isUsernameMatch & isPasswordMatch;
}

@DataBoundSetter
public void setEscapeHatchGroup(String escapeHatchGroup) {
this.escapeHatchGroup = Util.fixEmpty(escapeHatchGroup);
Expand Down Expand Up @@ -702,8 +722,8 @@

if (authentication instanceof UsernamePasswordAuthenticationToken && escapeHatchEnabled) {
randomWait(); // to slowdown brute forcing
if ( authentication.getPrincipal().toString().equals(escapeHatchUsername) &&
authentication.getCredentials().toString().equals(Secret.toString(escapeHatchSecret))) {
if (checkEscapeHatch(authentication.getPrincipal().toString(),
authentication.getCredentials().toString())) {

Check warning on line 726 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines

Lines 725-726 are not covered by tests

Check warning on line 726 in src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java#L726

Added line #L726 was not covered by tests
List<GrantedAuthority> grantedAuthorities = new ArrayList<>();
grantedAuthorities.add(SecurityRealm.AUTHENTICATED_AUTHORITY2);
if (isNotBlank(escapeHatchGroup)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
<f:entry title="${%Post logout redirect URL}" field="postLogoutRedirectUrl">
<f:textbox/>
</f:entry>
<f:entry title="${%Enable Proof Key for Code Exchang (PKCE)}" field="pkceEnabled">
<f:entry title="${%Enable Proof Key for Code Exchange (PKCE)}" field="pkceEnabled">
<f:checkbox/>
</f:entry>
<f:entry title="${%Disable Nonce verification}" field="nonceDisabled">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public void testConfig() {
assertEquals("emailFieldName", oicSecurityRealm.getEmailFieldName());
assertTrue(oicSecurityRealm.isEscapeHatchEnabled());
assertEquals("escapeHatchGroup", oicSecurityRealm.getEscapeHatchGroup());
assertEquals("escapeHatchSecret", Secret.toString(oicSecurityRealm.getEscapeHatchSecret()));
assertEquals("$2a$10$fxteEkfDqwqkmUelZmTxlu9WESjVDKQhp6jsqB1AgsLQ2dC6jikga", Secret.toString(oicSecurityRealm.getEscapeHatchSecret()));
assertEquals("escapeHatchUsername", oicSecurityRealm.getEscapeHatchUsername());
assertEquals("fullNameFieldName", oicSecurityRealm.getFullNameFieldName());
assertEquals("groupsFieldName", oicSecurityRealm.getGroupsFieldName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.springframework.security.crypto.bcrypt.BCrypt;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

public class OicSecurityRealmTest {

Expand Down Expand Up @@ -113,4 +117,39 @@ public void testShouldReturnRootUrlWhenRedirectUrlIsInvalid() throws IOException
assertEquals(rootUrl, realm.getValidRedirectUrl("http://localhost/bar/"));
assertEquals(rootUrl, realm.getValidRedirectUrl("http://localhost/jenkins/../bar/"));
}

@Test
public void testShouldCheckEscapeHatchWithPlainPassword() throws IOException {
final String escapeHatchUsername = "aUsername";
final String escapeHatchPassword = "aSecretPassword";

TestRealm realm = new TestRealm.Builder(wireMockRule)
.WithMinimalDefaults()
.WithEscapeHatch(true, escapeHatchUsername, escapeHatchPassword, "Group")
.build();

assertEquals(escapeHatchUsername, realm.getEscapeHatchUsername());
assertNotEquals(escapeHatchPassword, Secret.toString(realm.getEscapeHatchSecret()));
assertTrue(realm.doCheckEscapeHatch(escapeHatchUsername, escapeHatchPassword));
assertFalse(realm.doCheckEscapeHatch("otherUsername", escapeHatchPassword));
assertFalse(realm.doCheckEscapeHatch(escapeHatchUsername, "wrongPassword"));
}

@Test
public void testShouldCheckEscapeHatchWithHashedPassword() throws IOException {
final String escapeHatchUsername = "aUsername";
final String escapeHatchPassword = "aSecretPassword";
final String escapeHatchCryptedPassword = BCrypt.hashpw(escapeHatchPassword, BCrypt.gensalt());

TestRealm realm = new TestRealm.Builder(wireMockRule)
.WithMinimalDefaults()
.WithEscapeHatch(true, escapeHatchUsername, escapeHatchCryptedPassword, "Group")
.build();

assertEquals(escapeHatchUsername, realm.getEscapeHatchUsername());
assertEquals(escapeHatchCryptedPassword, Secret.toString(realm.getEscapeHatchSecret()));
assertTrue(realm.doCheckEscapeHatch(escapeHatchUsername, escapeHatchPassword));
assertFalse(realm.doCheckEscapeHatch("otherUsername", escapeHatchPassword));
assertFalse(realm.doCheckEscapeHatch(escapeHatchUsername, "wrongPassword"));
}
}
4 changes: 4 additions & 0 deletions src/test/java/org/jenkinsci/plugins/oic/TestRealm.java
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,8 @@ public String getStringFieldFromJMESPath(Object object, String jmespathField) {
public Object readResolve() {
return super.readResolve();
}

public boolean doCheckEscapeHatch(String username, String password) {
return super.checkEscapeHatch(username, password);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jenkins:
emailFieldName: emailFieldName
escapeHatchEnabled: true
escapeHatchGroup: escapeHatchGroup
escapeHatchSecret: escapeHatchSecret
escapeHatchSecret: "$2a$10$fxteEkfDqwqkmUelZmTxlu9WESjVDKQhp6jsqB1AgsLQ2dC6jikga"
escapeHatchUsername: escapeHatchUsername
fullNameFieldName: fullNameFieldName
groupsFieldName: groupsFieldName
Expand Down