From acff33106e56f9ee1d3da79a06f794151f17798d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 16 Oct 2013 17:03:03 -0400 Subject: [PATCH] [FIXED JENKINS-16936] Added SecureRequester extension point. --- changelog.html | 3 ++ core/src/main/java/hudson/model/Api.java | 23 ++++++--- .../jenkins/security/SecureRequester.java | 50 +++++++++++++++++++ .../security/csrf/DefaultCrumbIssuerTest.java | 3 ++ 4 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 core/src/main/java/jenkins/security/SecureRequester.java diff --git a/changelog.html b/changelog.html index 30159c9653..53b6520b84 100644 --- a/changelog.html +++ b/changelog.html @@ -58,6 +58,9 @@ Upcoming changes
  • Upgrade bundled plugin versions: ssh-slaves to 1.5, and credentials to 1.9.1 (issue 20071) +
  • + Extension point for secure users of REST APIs (permitting JSONP and primitive XPath). + (issue 16936)
  • Integer overflow could cause JavaScript functions to break in long-running Jenkins processes. (issue 20085) diff --git a/core/src/main/java/hudson/model/Api.java b/core/src/main/java/hudson/model/Api.java index 5bb9b8f8af..652fcf9e85 100644 --- a/core/src/main/java/hudson/model/Api.java +++ b/core/src/main/java/hudson/model/Api.java @@ -25,6 +25,8 @@ package hudson.model; import hudson.util.IOException2; import jenkins.model.Jenkins; +import jenkins.security.SecureRequester; + import org.dom4j.CharacterData; import org.dom4j.Document; import org.dom4j.DocumentException; @@ -59,6 +61,7 @@ import java.util.logging.Logger; * * @author Kohsuke Kawaguchi * @see Exported + * @see SecureRequester */ public class Api extends AbstractModelObject { /** @@ -155,12 +158,12 @@ public class Api extends AbstractModelObject { OutputStream o = rsp.getCompressedOutputStream(req); try { if (result instanceof CharacterData || result instanceof String || result instanceof Number || result instanceof Boolean) { - if (INSECURE) { + if (permit(req)) { rsp.setContentType("text/plain;charset=UTF-8"); String text = result instanceof CharacterData ? ((CharacterData) result).getText() : result.toString(); o.write(text.getBytes("UTF-8")); } else { - rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "primitive XPath result sets forbidden; can use -Dhudson.model.Api.INSECURE=true if you run without security"); + rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "primitive XPath result sets forbidden; implement jenkins.security.SecureRequester"); } return; } @@ -188,11 +191,11 @@ public class Api extends AbstractModelObject { * Exposes the bean as JSON. */ public void doJson(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException { - if (INSECURE || req.getParameter("jsonp") == null) { - setHeaders(rsp); + if (req.getParameter("jsonp") == null || permit(req)) { + setHeaders(rsp); rsp.serveExposedBean(req,bean, Flavor.JSON); } else { - rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "jsonp forbidden; can use -Dhudson.model.Api.INSECURE=true if you run without security"); + rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "jsonp forbidden; implement jenkins.security.SecureRequester"); } } @@ -204,6 +207,15 @@ public class Api extends AbstractModelObject { rsp.serveExposedBean(req,bean, Flavor.PYTHON); } + private boolean permit(StaplerRequest req) { + for (SecureRequester r : Jenkins.getInstance().getExtensionList(SecureRequester.class)) { + if (r.permit(req, bean)) { + return true; + } + } + return false; + } + private void setHeaders(StaplerResponse rsp) { rsp.setHeader("X-Jenkins", Jenkins.VERSION); rsp.setHeader("X-Jenkins-Session", Jenkins.SESSION_HASH); @@ -211,6 +223,5 @@ public class Api extends AbstractModelObject { private static final Logger LOGGER = Logger.getLogger(Api.class.getName()); private static final ModelBuilder MODEL_BUILDER = new ModelBuilder(); - private static final boolean INSECURE = "true".equals(System.getProperty("hudson.model.Api.INSECURE")); } diff --git a/core/src/main/java/jenkins/security/SecureRequester.java b/core/src/main/java/jenkins/security/SecureRequester.java new file mode 100644 index 0000000000..42c6ac1dbb --- /dev/null +++ b/core/src/main/java/jenkins/security/SecureRequester.java @@ -0,0 +1,50 @@ +package jenkins.security; + +import hudson.Extension; +import hudson.ExtensionPoint; +import hudson.model.Api; +import java.util.logging.Logger; +import jenkins.model.Jenkins; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import org.kohsuke.stapler.StaplerRequest; + +/** + * An extension point for authorizing REST API access to an object where an unsafe result type would be produced. + * Both JSONP and XPath with primitive result sets are considered unsafe due to CSRF attacks. + * A default implementation allows requests if a deprecated system property is set, or if Jenkins is unsecured anyway, + * but plugins may offer implementations which authorize scripted clients, requests from inside a trusted domain, etc. + * @see Api + * @since 1.537 + */ +public interface SecureRequester extends ExtensionPoint { + + /** + * Checks if a Jenkins object can be accessed by a given REST request. + * For instance, if the {@link StaplerRequest#getReferer} matches a given host, or + * anonymous read is allowed for the given object. + * @param req a request going through the REST API + * @param bean an exported object of some kind + * @return true if this requester should be trusted, false to reject + */ + boolean permit(StaplerRequest req, Object bean); + + @Restricted(NoExternalUse.class) + @Extension class Default implements SecureRequester { + + private static final String PROP = "hudson.model.Api.INSECURE"; + private static final boolean INSECURE = Boolean.getBoolean(PROP); + static { + if (INSECURE) { + Logger.getLogger(SecureRequester.class.getName()).warning(PROP + " system property is deprecated; implement SecureRequester instead"); + } + } + + @Override public boolean permit(StaplerRequest req, Object bean) { + return INSECURE || !Jenkins.getInstance().isUseSecurity(); + } + + } + +} \ No newline at end of file diff --git a/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java b/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java index 84b349c1af..9c180e19ee 100644 --- a/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java +++ b/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java @@ -11,6 +11,7 @@ import com.gargoylesoftware.htmlunit.html.HtmlPage; import java.net.HttpURLConnection; import org.jvnet.hudson.test.Bug; import org.jvnet.hudson.test.HudsonTestCase; +import org.jvnet.hudson.test.recipes.PresetData; /** * @@ -97,6 +98,7 @@ public class DefaultCrumbIssuerTest extends HudsonTestCase { submit(p.getFormByName("config")); } + @PresetData(PresetData.DataSet.ANONYMOUS_READONLY) public void testApiXml() throws Exception { WebClient wc = new WebClient(); assertXPathValue(wc.goToXml("crumbIssuer/api/xml"), "//crumbRequestField", jenkins.getCrumbIssuer().getCrumbRequestField()); @@ -116,6 +118,7 @@ public class DefaultCrumbIssuerTest extends HudsonTestCase { wc.assertFails("crumbIssuer/api/xml?xpath=concat(//crumbRequestField,'=',//crumb)", HttpURLConnection.HTTP_FORBIDDEN); // perhaps interpretable as JS number } + @PresetData(PresetData.DataSet.ANONYMOUS_READONLY) public void testApiJson() throws Exception { WebClient wc = new WebClient(); String json = wc.goTo("crumbIssuer/api/json", "application/json").getWebResponse().getContentAsString(); -- GitLab