From 581eb9ceb354b8a55c010d0547ff73cb6fd67a75 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Fri, 4 Sep 2015 15:45:43 +0200 Subject: [PATCH 01/12] [SECURITY-186] check user permissions when calling QUEUE API --- core/src/main/java/hudson/model/Queue.java | 108 ++++++++++++++++-- .../src/test/java/hudson/model/QueueTest.java | 77 ++++++++++++- 2 files changed, 174 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index 830eb851f6..6727ab16c9 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -109,6 +109,7 @@ import jenkins.security.QueueItemAuthenticatorConfiguration; import jenkins.util.AtmostOneTaskExecutor; import org.acegisecurity.AccessDeniedException; import org.acegisecurity.Authentication; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; import org.kohsuke.stapler.export.Exported; @@ -694,15 +695,68 @@ public class Queue extends ResourceController implements Saveable { */ @Exported(inline=true) public synchronized Item[] getItems() { - Item[] r = new Item[waitingList.size() + blockedProjects.size() + buildables.size() + pendings.size()]; - waitingList.toArray(r); - int idx = waitingList.size(); - for (BlockedItem p : blockedProjects.values()) - r[idx++] = p; - for (BuildableItem p : reverse(buildables.values())) - r[idx++] = p; - for (BuildableItem p : reverse(pendings.values())) - r[idx++] = p; + List r = new ArrayList(); + + for(WaitingItem p : waitingList) { + r = filterItemListBasedOnPermissions(r, p); + } + for (BlockedItem p : blockedProjects.values()){ + r = filterItemListBasedOnPermissions(r, p); + } + for (BuildableItem p : reverse(buildables.values())) { + r = filterItemListBasedOnPermissions(r, p); + } + for (BuildableItem p : reverse(pendings.values())) { + r= filterItemListBasedOnPermissions(r, p); + } + Item[] items = new Item[r.size()]; + r.toArray(items); + return items; + } + + private List filterItemListBasedOnPermissions(List r, Item t) { + if (t.task instanceof hudson.model.Item) { + if (((hudson.model.Item)t.task).hasPermission(hudson.model.Item.READ)) { + r.add(t); + } + } + return r; + } + + /** + * Returns an array of Item for which it is only visible the name of the task. + * + * Generally speaking the array is sorted such that the items that are most likely built sooner are + * at the end. + */ + @Restricted(NoExternalUse.class) + @Exported(inline=true) + public synchronized StubItem[] getDiscoverableItems() { + List r = new ArrayList(); + + for(WaitingItem p : waitingList) { + r = filterDiscoverableItemListBasedOnPermissions(r, p); + } + for (BlockedItem p : blockedProjects.values()){ + r = filterDiscoverableItemListBasedOnPermissions(r, p); + } + for (BuildableItem p : reverse(buildables.values())) { + r = filterDiscoverableItemListBasedOnPermissions(r, p); + } + for (BuildableItem p : reverse(pendings.values())) { + r= filterDiscoverableItemListBasedOnPermissions(r, p); + } + StubItem[] items = new StubItem[r.size()]; + r.toArray(items); + return items; + } + + private List filterDiscoverableItemListBasedOnPermissions(List r, Item t) { + if (t.task instanceof hudson.model.Item) { + if (!((hudson.model.Item)t.task).hasPermission(hudson.model.Item.READ) && ((hudson.model.Item)t.task).hasPermission(hudson.model.Item.DISCOVER)) { + r.add(new StubItem(new StubTask(t.task))); + } + } return r; } @@ -1581,6 +1635,42 @@ public class Queue extends ResourceController implements Saveable { } } + + /** + * A Stub class for {@link Task} which exposes only the name of the Task to be displayed when the user + * has DISCOVERY permissions only. + */ + @Restricted(NoExternalUse.class) + @ExportedBean(defaultVisibility = 999) + public static class StubTask { + + private String name; + + public StubTask(@Nonnull Queue.Task base) { + this.name = base.getName(); + } + + @Exported + public String getName() { + return name; + } + } + + /** + * A Stub class for {@link Item} which exposes only the name of the Task to be displayed when the user + * has DISCOVERY permissions only. + */ + @Restricted(NoExternalUse.class) + @ExportedBean(defaultVisibility = 999) + public class StubItem { + + @Exported public StubTask task; + + public StubItem(StubTask task) { + this.task = task; + } + + } /** * An optional interface for actions on Queue.Item. diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 93bcb5ba05..2af2d61507 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -23,9 +23,12 @@ */ package hudson.model; +import com.gargoylesoftware.htmlunit.html.DomElement; +import com.gargoylesoftware.htmlunit.html.DomNode; import com.gargoylesoftware.htmlunit.html.HtmlFileInput; import com.gargoylesoftware.htmlunit.html.HtmlForm; import com.gargoylesoftware.htmlunit.html.HtmlPage; +import com.gargoylesoftware.htmlunit.xml.XmlPage; import hudson.Launcher; import hudson.matrix.AxisList; import hudson.matrix.LabelAxis; @@ -39,11 +42,16 @@ import hudson.model.Queue.BlockedItem; import hudson.model.Queue.Executable; import hudson.model.Queue.WaitingItem; import hudson.model.queue.AbstractQueueTask; +import hudson.model.queue.CauseOfBlockage; +import hudson.model.queue.QueueTaskDispatcher; import hudson.model.queue.QueueTaskFuture; import hudson.model.queue.ScheduleResult; import hudson.model.queue.SubTask; import hudson.security.ACL; +import hudson.security.AuthorizationMatrixProperty; import hudson.security.GlobalMatrixAuthorizationStrategy; +import hudson.security.Permission; +import hudson.security.ProjectMatrixAuthorizationStrategy; import hudson.security.SparseACL; import hudson.slaves.DumbSlave; import hudson.slaves.DummyCloudImpl; @@ -58,7 +66,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; @@ -84,7 +95,9 @@ import org.jvnet.hudson.test.Bug; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.MockQueueItemAuthenticator; import org.jvnet.hudson.test.SequenceLock; +import org.jvnet.hudson.test.SleepBuilder; import org.jvnet.hudson.test.TestBuilder; +import org.jvnet.hudson.test.TestExtension; import org.mortbay.jetty.Server; import org.mortbay.jetty.bio.SocketConnector; import org.mortbay.jetty.servlet.ServletHandler; @@ -413,11 +426,11 @@ public class QueueTest extends HudsonTestCase { FreeStyleBuild b = v.waitForStart(); assertEquals(1,b.getNumber()); assertTrue(b.isBuilding()); - assertSame(p,b.getProject()); + assertSame(p, b.getProject()); ev.signal(); // let the build complete FreeStyleBuild b2 = assertBuildStatusSuccess(v); - assertSame(b,b2); + assertSame(b, b2); } @Inject @@ -579,4 +592,64 @@ public class QueueTest extends HudsonTestCase { } catch (CancellationException e) { } } + + public void testQueueApiOutputShouldBeFilteredByUserPermission() throws Exception { + + jenkins.setSecurityRealm(createDummySecurityRealm()); + ProjectMatrixAuthorizationStrategy str = new ProjectMatrixAuthorizationStrategy(); + str.add(Jenkins.READ, "bob"); + str.add(Jenkins.READ, "alice"); + str.add(Jenkins.READ, "james"); + jenkins.setAuthorizationStrategy(str); + + FreeStyleProject project = createFreeStyleProject("project"); + + Map> permissions = new HashMap>(); + permissions.put(Item.READ, Collections.singleton("bob")); + permissions.put(Item.DISCOVER, Collections.singleton("james")); + AuthorizationMatrixProperty prop1 = new AuthorizationMatrixProperty(permissions); + project.addProperty(prop1); + project.getBuildersList().add(new SleepBuilder(10)); + project.scheduleBuild2(0); + + WebClient webClient = new WebClient(); + webClient.login("bob", "bob"); + XmlPage p = webClient.goToXml("/queue/api/xml"); + + //bob has permission on the project and will be able to see it in the queue together with information such as the URL and the name. + for (DomNode element: p.getFirstChild().getFirstChild().getChildNodes()){ + if(element.getNodeName().equals("task")){ + assertEquals(((DomElement)element).getElementsByTagName("name").size(),1); + assertEquals(((DomElement) element).getElementsByTagName("name").item(0).getFirstChild().toString(), "project"); + assertEquals(((DomElement)element).getElementsByTagName("url").size(),1); + } + } + WebClient webClient2 = new WebClient(); + webClient2.login("alice"); + XmlPage p2 = webClient2.goToXml("/queue/api/xml"); + //alice does not have permission on the project and will not see it in the queue. + assertEquals("", p2.getContent()); + + WebClient webClient3 = new WebClient(); + webClient3.login("james"); + XmlPage p3 = webClient3.goToXml("/queue/api/xml"); + //james has DISCOVER permission on the project and will only be able to see the task name. + assertEquals("project", + p3.getContent()); + + } + + //we force the project not to be executed so that it stays in the queue + @TestExtension("testQueueApiOutputShouldBeFilteredByUserPermission") + public static class MyQueueTaskDispatcher extends QueueTaskDispatcher { + @Override + public CauseOfBlockage canTake(Node node, Queue.BuildableItem item) { + return new CauseOfBlockage() { + @Override + public String getShortDescription() { + return "blocked by canTake"; + } + }; + } + } } From d85bf665425091470363e14143a7d35745ea28c7 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 15 Oct 2015 10:13:19 +0200 Subject: [PATCH 02/12] [SECURITY-184] fix test failures checking null value --- core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java b/core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java index 4fd1e9bf1d..cfd52f998b 100644 --- a/core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java +++ b/core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java @@ -91,7 +91,7 @@ public class DefaultJnlpSlaveReceiver extends JnlpAgentReceiver { String handshakeSecret = handshake.getRequestProperty("Secret-Key"); // Verify that the slave secret matches the handshake secret. if (!computer.getJnlpMac().equals(handshakeSecret)) { - LOGGER.log(Level.WARNING, "An attempt was made to connect as {0} from {1} with an incorrect secret", new Object[]{nodeName, handshake.getSocket().getRemoteSocketAddress()}); + LOGGER.log(Level.WARNING, "An attempt was made to connect as {0} from {1} with an incorrect secret", new Object[]{nodeName, handshake.getSocket()!=null?handshake.getSocket().getRemoteSocketAddress():null}); return false; } else { return true; From 223c301dce83087220629997784f5b920d8064a9 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 3 Nov 2015 17:04:03 +0100 Subject: [PATCH 03/12] [security-184] solve NPE in test --- .../test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java b/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java index 9cc6425d34..73b4a2c51f 100644 --- a/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java +++ b/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java @@ -56,6 +56,8 @@ public class DefaultJnlpSlaveReceiverTest { when(mockComputer.getChannel()).thenReturn(null); when(mockChannel.getProperty(any(String.class))).thenReturn("some cookie"); when(mockHandshake.jnlpConnect(mockComputer)).thenReturn(mockChannel); + when(mockHandshake.getRequestProperty("Secret-Key")).thenReturn("mock-secret"); + when(mockComputer.getJnlpMac()).thenReturn("mock-secret"); assertTrue(receiver.handle("node", mockHandshake)); verify(mockHandshake).success(any(Properties.class)); From 6d15072f045d08acd65f7d96ea053492808170ff Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Tue, 3 Nov 2015 20:08:14 +0100 Subject: [PATCH 04/12] [security-184] solve NPE in testHandleTakeover --- .../test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java b/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java index 73b4a2c51f..7fe3d9253b 100644 --- a/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java +++ b/core/src/test/java/jenkins/slaves/DefaultJnlpSlaveReceiverTest.java @@ -79,6 +79,8 @@ public class DefaultJnlpSlaveReceiverTest { when(mockChannel.getProperty(any(String.class))).thenReturn("some cookie"); when(mockComputer.disconnect(any(ConnectionFromCurrentPeer.class))).thenReturn(mockFuture); when(mockHandshake.jnlpConnect(mockComputer)).thenReturn(mockChannel); + when(mockHandshake.getRequestProperty("Secret-Key")).thenReturn("mock-secret"); + when(mockComputer.getJnlpMac()).thenReturn("mock-secret"); assertTrue(receiver.handle("node", mockHandshake)); verify(mockFuture).get(15, TimeUnit.SECONDS); From abe561499bbba2e725804c1117fc957028bbd608 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 3 Nov 2015 19:46:28 -0800 Subject: [PATCH 05/12] [SECURITY-214] OfflineCause.toString() is a string, not necessarily HTML --- .../main/java/hudson/node_monitors/ResponseTimeMonitor.java | 4 ++-- .../src/main/resources/hudson/slaves/OfflineCause/cause.jelly | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java b/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java index fa864bae3c..8c8a3b87cd 100644 --- a/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java +++ b/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java @@ -188,7 +188,7 @@ public class ResponseTimeMonitor extends NodeMonitor { } /** - * HTML rendering of the data + * String rendering of the data */ @Override public String toString() { @@ -200,7 +200,7 @@ public class ResponseTimeMonitor extends NodeMonitor { // return buf.toString(); int fc = failureCount(); if(fc>0) - return Util.wrapToErrorSpan(Messages.ResponseTimeMonitor_TimeOut(fc)); + return Messages.ResponseTimeMonitor_TimeOut(fc); return getAverage()+"ms"; } diff --git a/core/src/main/resources/hudson/slaves/OfflineCause/cause.jelly b/core/src/main/resources/hudson/slaves/OfflineCause/cause.jelly index 414f642c0c..9edd263ce2 100644 --- a/core/src/main/resources/hudson/slaves/OfflineCause/cause.jelly +++ b/core/src/main/resources/hudson/slaves/OfflineCause/cause.jelly @@ -24,5 +24,5 @@ THE SOFTWARE. -

+

${it}

 From 33b55588a6a5f844a59f2cd8940d385c6d412eb5 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Wed, 4 Nov 2015 16:25:33 +0100 Subject: [PATCH 06/12] [security-186]fix relative path --- test/src/test/java/hudson/model/QueueTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index 2af2d61507..88eec3a221 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -614,7 +614,7 @@ public class QueueTest extends HudsonTestCase { WebClient webClient = new WebClient(); webClient.login("bob", "bob"); - XmlPage p = webClient.goToXml("/queue/api/xml"); + XmlPage p = webClient.goToXml("queue/api/xml"); //bob has permission on the project and will be able to see it in the queue together with information such as the URL and the name. for (DomNode element: p.getFirstChild().getFirstChild().getChildNodes()){ @@ -626,13 +626,13 @@ public class QueueTest extends HudsonTestCase { } WebClient webClient2 = new WebClient(); webClient2.login("alice"); - XmlPage p2 = webClient2.goToXml("/queue/api/xml"); + XmlPage p2 = webClient2.goToXml("queue/api/xml"); //alice does not have permission on the project and will not see it in the queue. assertEquals("", p2.getContent()); WebClient webClient3 = new WebClient(); webClient3.login("james"); - XmlPage p3 = webClient3.goToXml("/queue/api/xml"); + XmlPage p3 = webClient3.goToXml("queue/api/xml"); //james has DISCOVER permission on the project and will only be able to see the task name. assertEquals("project", p3.getContent()); From 6f1982982be1899d3c279b663ffe88d3b09387f5 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Wed, 4 Nov 2015 23:18:05 +0100 Subject: [PATCH 07/12] [SECURITY-192] - Migrate the unit tests to the new HtmlUnit version --- test/src/test/java/hudson/cli/CLIActionTest2.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/src/test/java/hudson/cli/CLIActionTest2.java b/test/src/test/java/hudson/cli/CLIActionTest2.java index 14ac8dfe92..0f3e33df4b 100644 --- a/test/src/test/java/hudson/cli/CLIActionTest2.java +++ b/test/src/test/java/hudson/cli/CLIActionTest2.java @@ -25,7 +25,7 @@ package hudson.cli; import com.gargoylesoftware.htmlunit.HttpMethod; import com.gargoylesoftware.htmlunit.Page; -import com.gargoylesoftware.htmlunit.WebRequestSettings; +import com.gargoylesoftware.htmlunit.WebRequest; import com.gargoylesoftware.htmlunit.WebResponse; import java.net.URL; import java.util.UUID; @@ -56,13 +56,13 @@ public class CLIActionTest2 { // The behavior changed due to SECURITY-192. index page is no longer accessible to anonymous, // so we check the access by emulating the CLI connection post request - WebRequestSettings settings = new WebRequestSettings(new URL(j.getURL(), "cli")); - settings.setHttpMethod(HttpMethod.POST); - settings.setAdditionalHeader("Session", UUID.randomUUID().toString()); - settings.setAdditionalHeader("Side", "download"); // We try to download something to init the duplex channel - settings = wc.addCrumb(settings); + WebRequest req = new WebRequest(new URL(j.getURL(), "cli")); + req.setHttpMethod(HttpMethod.POST); + req.setAdditionalHeader("Session", UUID.randomUUID().toString()); + req.setAdditionalHeader("Side", "download"); // We try to download something to init the duplex channel + req = wc.addCrumb(req); - Page page = wc.getPage(settings); + Page page = wc.getPage(req); WebResponse webResponse = page.getWebResponse(); assertEquals("We expect that the proper POST request from CLI gets processed successfully", 200, webResponse.getStatusCode()); From 054a329c59171ca12ff98f7063ce7fd053ee08bf Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 4 Nov 2015 17:40:20 -0500 Subject: [PATCH 08/12] [SECURITY-206] Need to call ChannelConfigurator on JNLP slave channels. --- .../slaves/JnlpSlaveAgentProtocol.java | 5 ++++ .../bugs/JnlpAccessWithSecuredHudsonTest.java | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java b/core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java index d82a42e51a..f69211cf8c 100644 --- a/core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java +++ b/core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java @@ -9,6 +9,7 @@ import hudson.remoting.Engine; import hudson.slaves.SlaveComputer; import jenkins.AgentProtocol; import jenkins.model.Jenkins; +import jenkins.security.ChannelConfigurator; import jenkins.security.HMACConfidentialKey; import org.jenkinsci.remoting.nio.NioChannelHub; @@ -117,6 +118,10 @@ public class JnlpSlaveAgentProtocol extends AgentProtocol { try { ChannelBuilder cb = createChannelBuilder(nodeName); + for (ChannelConfigurator cc : ChannelConfigurator.all()) { + cc.onChannelBuilding(cb, computer); + } + computer.setChannel(cb.withHeaderStream(log).build(socket), log, new Listener() { @Override diff --git a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java index 52dc53e89b..ec2fb33b19 100644 --- a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java +++ b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java @@ -26,13 +26,17 @@ package hudson.bugs; import com.gargoylesoftware.htmlunit.Page; import com.gargoylesoftware.htmlunit.html.HtmlPage; import com.gargoylesoftware.htmlunit.xml.XmlPage; +import hudson.cli.util.ScriptLoader; import hudson.model.Node.Mode; import hudson.model.Slave; +import hudson.remoting.Channel; import hudson.remoting.Launcher; import hudson.remoting.Which; import hudson.slaves.JNLPLauncher; import hudson.slaves.RetentionStrategy; import hudson.slaves.DumbSlave; +import jenkins.security.MasterToSlaveCallable; +import jenkins.security.s2m.AdminWhitelistRule; import org.dom4j.Document; import org.dom4j.Element; import org.dom4j.io.DOMReader; @@ -41,6 +45,7 @@ import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.recipes.PresetData; import org.jvnet.hudson.test.recipes.PresetData.DataSet; +import java.io.File; import java.net.HttpURLConnection; import java.net.URL; import java.util.Collections; @@ -112,6 +117,16 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase { for (int i = 0; i < /* one minute */600; i++) { if (slave.getComputer().isOnline()) { System.err.println("JNLP slave successfully connected"); + Channel channel = slave.getComputer().getChannel(); + assertFalse("SECURITY-206", channel.isRemoteClassLoadingAllowed()); + jenkins.getExtensionList(AdminWhitelistRule.class).get(AdminWhitelistRule.class).setMasterKillSwitch(false); + final File f = new File(jenkins.getRootDir(), "secrets/master.key"); // DefaultConfidentialStore + assertTrue(f.exists()); + try { + fail("SECURITY-206: " + channel.call(new Attack(f.getAbsolutePath()))); + } catch (SecurityException x) { + System.out.println("expected: " + x); + } return; } Thread.sleep(100); @@ -122,4 +137,15 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase { } } + private static class Attack extends MasterToSlaveCallable { + private final String path; + Attack(String path) { + this.path = path; + } + @Override + public String call() throws Exception { + return Channel.current().call(new ScriptLoader(path)); + } + } + } From 4a72e938d58598cd4bd3caa48ee9e8a3f60c30e4 Mon Sep 17 00:00:00 2001 From: Valentina Armenise Date: Thu, 5 Nov 2015 11:44:01 +0100 Subject: [PATCH 09/12] [security-186] getElementByTagName does not work --- test/src/test/java/hudson/model/QueueTest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java index c128f1cb27..8b2f9de1cc 100644 --- a/test/src/test/java/hudson/model/QueueTest.java +++ b/test/src/test/java/hudson/model/QueueTest.java @@ -937,10 +937,14 @@ public class QueueTest { //bob has permission on the project and will be able to see it in the queue together with information such as the URL and the name. for (DomNode element: p.getFirstChild().getFirstChild().getChildNodes()){ - if(element.getNodeName().equals("task")){ - assertEquals(((DomElement)element).getElementsByTagName("name").size(),1); - assertEquals(((DomElement) element).getElementsByTagName("name").item(0).getFirstChild().toString(), "project"); - assertEquals(((DomElement)element).getElementsByTagName("url").size(),1); + if (element.getNodeName().equals("task")) { + for (DomNode child: ((DomElement) element).getChildNodes()) { + if (child.getNodeName().equals("name")) { + assertEquals(child.asText(), "project"); + } else if (child.getNodeName().equals("url")) { + assertNotNull(child.asText()); + } + } } } webClient = r.createWebClient(); From 1449f4528416a751d70eb89b92cb9251d6da7e3f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 5 Nov 2015 07:13:30 -0500 Subject: [PATCH 10/12] For some reason secrets/master.key is not present during a CI build, though it is locally. --- .../test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java index ec2fb33b19..afc9f67640 100644 --- a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java +++ b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java @@ -120,7 +120,7 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase { Channel channel = slave.getComputer().getChannel(); assertFalse("SECURITY-206", channel.isRemoteClassLoadingAllowed()); jenkins.getExtensionList(AdminWhitelistRule.class).get(AdminWhitelistRule.class).setMasterKillSwitch(false); - final File f = new File(jenkins.getRootDir(), "secrets/master.key"); // DefaultConfidentialStore + final File f = new File(jenkins.getRootDir(), "config.xml"); assertTrue(f.exists()); try { fail("SECURITY-206: " + channel.call(new Attack(f.getAbsolutePath()))); From e77aa43b70030397352d7a8b4f0f8fa65429ef32 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 5 Nov 2015 07:47:23 -0500 Subject: [PATCH 11/12] Strengthening test case slightly to verify that the security exception is the expected one. --- .../test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java index afc9f67640..ccf5ff3031 100644 --- a/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java +++ b/test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java @@ -126,6 +126,7 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase { fail("SECURITY-206: " + channel.call(new Attack(f.getAbsolutePath()))); } catch (SecurityException x) { System.out.println("expected: " + x); + assertTrue(x.getMessage().contains("http://jenkins-ci.org/security-144")); } return; } From 52c7ca45da8096805cffbf8ebe2601828cb30d14 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 5 Nov 2015 09:13:14 -0500 Subject: [PATCH 12/12] Adding a warning to Javadoc. --- core/src/main/java/hudson/slaves/SlaveComputer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/slaves/SlaveComputer.java b/core/src/main/java/hudson/slaves/SlaveComputer.java index aaa28282f5..02d61e1ce4 100644 --- a/core/src/main/java/hudson/slaves/SlaveComputer.java +++ b/core/src/main/java/hudson/slaves/SlaveComputer.java @@ -432,7 +432,7 @@ public class SlaveComputer extends Computer { /** * Sets up the connection through an existing channel. - * + * @param channel the channel to use; warning: callers are expected to have called {@link ChannelConfigurator} already * @since 1.444 */ public void setChannel(Channel channel, OutputStream launchLog, Channel.Listener listener) throws IOException, InterruptedException {