Skip to content

Commit 334b8d0

Browse files
olamyolamy
committed
[SECURITY-3225] Possible exposure of system-scoped credentials during configuration validation
Signed-off-by: Olivier Lamy <olamy@apache.org>
1 parent e7e42a3 commit 334b8d0

6 files changed

Lines changed: 73 additions & 30 deletions

File tree

‎src/main/java/hudson/plugins/jira/JiraSite.java‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -694,22 +694,30 @@ public JiraSession getSession() {
694694
*/
695695
@Nullable
696696
public JiraSession getSession(Item item) {
697+
return getSession(item, false);
698+
}
699+
700+
JiraSession getSession(Item item, boolean uiValidation) {
697701
if (jiraSession == null) {
698-
jiraSession = createSession(item);
702+
jiraSession = createSession(item, uiValidation);
699703
}
700704
return jiraSession;
701705
}
702706

707+
JiraSession createSession(Item item) {
708+
return createSession(item, false);
709+
}
710+
703711
/**
704712
* Creates a remote access session to this Jira.
705713
*
706714
* @return null if remote access is not supported.
707715
*/
708-
JiraSession createSession(Item item) {
716+
JiraSession createSession(Item item, boolean uiValidation) {
709717
ItemGroup itemGroup = map(item);
710718
item = itemGroup instanceof Folder ? ((Folder) itemGroup) : item;
711719

712-
StandardUsernamePasswordCredentials credentials = resolveCredentials(item);
720+
StandardUsernamePasswordCredentials credentials = resolveCredentials(item, uiValidation);
713721

714722
if (credentials == null) {
715723
LOGGER.fine("no Jira credentials available for " + item);
@@ -735,8 +743,10 @@ Lock getProjectUpdateLock() {
735743
/**
736744
* This method only supports credential matching by credentialsId.
737745
* Older methods are not and will not be supported as the credentials should have been migrated already.
746+
* @param item can be <code>null</code> if top level
747+
* @param uiValidation if <code>true</code> and credentials not found at item level will not go up
738748
*/
739-
private StandardUsernamePasswordCredentials resolveCredentials(Item item) {
749+
private StandardUsernamePasswordCredentials resolveCredentials(Item item, boolean uiValidation) {
740750
if (credentialsId == null) {
741751
LOGGER.fine("credentialsId is null");
742752
return null; // remote access not supported
@@ -746,12 +756,17 @@ private StandardUsernamePasswordCredentials resolveCredentials(Item item) {
746756
.build();
747757

748758
if (item != null) {
749-
StandardUsernamePasswordCredentials creds = CredentialsMatchers.firstOrNull(
759+
StandardUsernamePasswordCredentials credentials = CredentialsMatchers.firstOrNull(
750760
CredentialsProvider.lookupCredentials(
751761
StandardUsernamePasswordCredentials.class, item, ACL.SYSTEM, req),
752762
CredentialsMatchers.withId(credentialsId));
753-
if (creds != null) {
754-
return creds;
763+
if (credentials != null) {
764+
return credentials;
765+
}
766+
// during UI validation of the configuration we definitely don't want to expose
767+
// global credentials
768+
if (uiValidation) {
769+
return null;
755770
}
756771
}
757772
return CredentialsMatchers.firstOrNull(
@@ -1371,9 +1386,11 @@ public FormValidation doValidate(
13711386
site.setReadTimeout(readTimeout);
13721387
site.setThreadExecutorNumber(threadExecutorNumber);
13731388
site.setUseBearerAuth(useBearerAuth);
1374-
JiraSession session = null;
13751389
try {
1376-
session = site.getSession(item);
1390+
JiraSession session = site.getSession(item, true);
1391+
if (session == null) {
1392+
return FormValidation.error("Cannot validate configuration");
1393+
}
13771394
session.getMyPermissions();
13781395
return FormValidation.ok("Success");
13791396
} catch (RestClientException e) {

‎src/test/java/hudson/plugins/jira/ChangingWorkflowTest.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import static org.hamcrest.MatcherAssert.assertThat;
55
import static org.hamcrest.Matchers.equalTo;
66
import static org.hamcrest.Matchers.nullValue;
7+
import static org.mockito.ArgumentMatchers.anyBoolean;
78
import static org.mockito.ArgumentMatchers.anyString;
89
import static org.mockito.ArgumentMatchers.eq;
910
import static org.mockito.ArgumentMatchers.isNull;
@@ -112,8 +113,9 @@ public void getActionIdReturnsIdWhenFoundIgnorecaseWorkflow() {
112113

113114
@Test
114115
public void addCommentsOnNonEmptyWorkflowAndNonEmptyComment() throws Exception {
116+
when(site.getSession(any(), anyBoolean())).thenCallRealMethod();
115117
when(site.getSession(any())).thenCallRealMethod();
116-
when(site.createSession(any())).thenReturn(mockSession);
118+
when(site.createSession(any(), anyBoolean())).thenReturn(mockSession);
117119
site.getSession(mockItem);
118120

119121
when(mockSession.getIssuesFromJqlSearch(anyString())).thenReturn(Arrays.asList(mock(Issue.class)));
@@ -130,7 +132,8 @@ public void addCommentsOnNonEmptyWorkflowAndNonEmptyComment() throws Exception {
130132
@Test
131133
public void addCommentsOnNullWorkflowAndNonEmptyComment() throws Exception {
132134
when(site.getSession(any())).thenCallRealMethod();
133-
when(site.createSession(any())).thenReturn(mockSession);
135+
when(site.getSession(any(), anyBoolean())).thenCallRealMethod();
136+
when(site.createSession(any(), anyBoolean())).thenReturn(mockSession);
134137
site.getSession(mockItem);
135138

136139
when(mockSession.getIssuesFromJqlSearch(anyString())).thenReturn(Arrays.asList(mock(Issue.class)));

‎src/test/java/hudson/plugins/jira/DescriptorImplTest.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ public void validateFormConnectionErrors() throws Exception {
125125
when(descriptor.getBuilder()).thenReturn(builder);
126126
when(builder.build()).thenReturn(site);
127127
when(build.getParent()).thenReturn(project);
128-
when(site.getSession(project)).thenReturn(session);
128+
when(site.getSession(project, true)).thenReturn(session);
129129
when(session.getMyPermissions()).thenThrow(RestClientException.class);
130130

131131
FormValidation validation = descriptor.doValidate(
@@ -142,7 +142,7 @@ public void validateFormConnectionErrors() throws Exception {
142142
project);
143143

144144
assertEquals(FormValidation.Kind.ERROR, validation.kind);
145-
verify(site).getSession(project);
145+
verify(site).getSession(project, true);
146146

147147
validation = descriptor.doValidate(
148148
"http://localhost:8080",
@@ -158,7 +158,7 @@ public void validateFormConnectionErrors() throws Exception {
158158
project);
159159
assertEquals(Messages.JiraSite_timeoutMinimunValue("1"), validation.getLocalizedMessage());
160160
assertEquals(FormValidation.Kind.ERROR, validation.kind);
161-
verify(site).getSession(project);
161+
verify(site).getSession(project, true);
162162

163163
validation = descriptor.doValidate(
164164
"http://localhost:8080",
@@ -175,7 +175,7 @@ public void validateFormConnectionErrors() throws Exception {
175175

176176
assertEquals(Messages.JiraSite_readTimeoutMinimunValue("1"), validation.getMessage());
177177
assertEquals(FormValidation.Kind.ERROR, validation.kind);
178-
verify(site).getSession(project);
178+
verify(site).getSession(project, true);
179179

180180
validation = descriptor.doValidate(
181181
"http://localhost:8080",
@@ -191,7 +191,7 @@ public void validateFormConnectionErrors() throws Exception {
191191
project);
192192
assertEquals(Messages.JiraSite_threadExecutorMinimunSize("1"), validation.getMessage());
193193
assertEquals(FormValidation.Kind.ERROR, validation.kind);
194-
verify(site).getSession(project);
194+
verify(site).getSession(project, true);
195195
}
196196

197197
@Test
@@ -200,7 +200,7 @@ public void validateFormConnectionOK() throws Exception {
200200

201201
when(descriptor.getBuilder()).thenReturn(builder);
202202
when(builder.build()).thenReturn(site);
203-
when(site.getSession(project)).thenReturn(session);
203+
when(site.getSession(project, true)).thenReturn(session);
204204
when(session.getMyPermissions()).thenReturn(mock(Permissions.class));
205205

206206
FormValidation validation = descriptor.doValidate(
@@ -217,7 +217,7 @@ public void validateFormConnectionOK() throws Exception {
217217
project);
218218

219219
verify(builder).build();
220-
verify(site).getSession(project);
220+
verify(site).getSession(project, true);
221221
assertEquals(FormValidation.Kind.OK, validation.kind);
222222
}
223223
}

‎src/test/java/hudson/plugins/jira/JiraCreateReleaseNotesTest.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import static org.junit.Assert.assertEquals;
55
import static org.junit.Assert.assertNotNull;
66
import static org.mockito.ArgumentMatchers.any;
7+
import static org.mockito.ArgumentMatchers.anyBoolean;
78
import static org.mockito.Mockito.doReturn;
89
import static org.mockito.Mockito.spy;
910
import static org.mockito.Mockito.verify;
@@ -89,9 +90,9 @@ public void createCommonMocks() throws IOException, InterruptedException {
8990
}
9091
});
9192

92-
when(site.createSession(any())).thenReturn(session);
93-
when(site.getSession(any())).thenCallRealMethod();
94-
site.getSession(mockItem);
93+
when(site.createSession(any(), anyBoolean())).thenReturn(session);
94+
when(site.getSession(any(), anyBoolean())).thenCallRealMethod();
95+
site.getSession(mockItem, false);
9596
}
9697

9798
@Test

‎src/test/java/hudson/plugins/jira/JiraFolderPropertyTest.java‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.util.ArrayList;
1111
import java.util.Arrays;
1212
import java.util.List;
13+
import java.util.stream.StreamSupport;
1314
import org.junit.Rule;
1415
import org.junit.Test;
1516
import org.jvnet.hudson.test.JenkinsRule;
@@ -37,14 +38,9 @@ public void configRoundtrip() throws Exception {
3738
}
3839

3940
public static CredentialsStore getFolderStore(Folder f) {
40-
Iterable<CredentialsStore> stores = CredentialsProvider.lookupStores(f);
41-
CredentialsStore folderStore = null;
42-
for (CredentialsStore s : stores) {
43-
if (s.getProvider() instanceof FolderCredentialsProvider && s.getContext() == f) {
44-
folderStore = s;
45-
break;
46-
}
47-
}
48-
return folderStore;
41+
return StreamSupport.stream(CredentialsProvider.lookupStores(f).spliterator(), false)
42+
.filter(s -> s.getProvider() instanceof FolderCredentialsProvider && s.getContext() == f)
43+
.findFirst()
44+
.orElse(null);
4945
}
5046
}

‎src/test/java/hudson/plugins/jira/JiraSiteSecurity1029Test.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package hudson.plugins.jira;
22

3+
import static org.hamcrest.CoreMatchers.containsString;
34
import static org.hamcrest.CoreMatchers.equalTo;
45
import static org.hamcrest.CoreMatchers.nullValue;
56
import static org.hamcrest.MatcherAssert.assertThat;
@@ -33,6 +34,7 @@
3334
import org.htmlunit.HttpMethod;
3435
import org.htmlunit.Page;
3536
import org.htmlunit.WebRequest;
37+
import org.htmlunit.WebResponse;
3638
import org.htmlunit.util.NameValuePair;
3739
import org.junit.After;
3840
import org.junit.Rule;
@@ -167,6 +169,30 @@ public void cannotLeakCredentials() throws Exception {
167169
assertThat(servlet.getPasswordAndReset(), nullValue());
168170
}
169171

172+
{ // as an user with just read access, I may not be able to leak any credentials with fake id in a folder
173+
Folder folder = j.jenkins.createProject(
174+
Folder.class, "folder" + j.jenkins.getItems().size());
175+
176+
JenkinsRule.WebClient wc = j.createWebClient();
177+
wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
178+
wc.withBasicApiToken(userFolderConfigure);
179+
180+
String jiraSiteValidateUrl = j.jenkins.getRootUrl() + folder.getUrl() + "descriptorByName/"
181+
+ JiraSite.class.getName() + "/validate";
182+
WebRequest request = new WebRequest(new URL(jiraSiteValidateUrl), HttpMethod.POST);
183+
request.setRequestParameters(Arrays.asList(
184+
new NameValuePair("threadExecutorNumber", "1"),
185+
new NameValuePair("url", serverUri.toString()),
186+
new NameValuePair("credentialsId", "aussie-beer-is-the-best"), // use a non existing id on purpose
187+
new NameValuePair("useHTTPAuth", "true")));
188+
189+
Page page = wc.getPage(request);
190+
WebResponse webResponse = page.getWebResponse();
191+
assertThat(webResponse.getStatusCode(), equalTo(200));
192+
assertThat(webResponse.getContentAsString(), containsString("Cannot validate configuration"));
193+
assertThat(servlet.getPasswordAndReset(), nullValue());
194+
}
195+
170196
{ // as an user with configure access, I can access
171197
Folder folder = j.jenkins.createProject(
172198
Folder.class, "folder" + j.jenkins.getItems().size());

0 commit comments

Comments
 (0)