From d6e14b1eca6cb0ba406308a79f442196f1fd8b45 Mon Sep 17 00:00:00 2001 From: James Nord Date: Sat, 31 Jan 2015 12:44:12 +0000 Subject: [PATCH 1/5] [SECURITY-167] defend against XXE attacks. Added a new EntityResolver that will throw an exception if any attempts are made to load external entities. Made the transforer use SAX so that we can use out EntityResolover. As we can't defend against calls that have already parsed the xml (e.g. DOMSource) if we are parsed one of those throw an exception (which can be disabled with a System property. --- .../main/java/hudson/model/AbstractItem.java | 23 ++-- core/src/main/java/hudson/model/View.java | 17 +-- .../util/xml/RestrictiveEntityResolver.java | 31 +++++ .../main/java/jenkins/util/xml/XMLUtils.java | 79 +++++++++++++ .../test/java/jenkins/xml/XMLUtilsTest.java | 94 +++++++++++++++ .../java/hudson/model/AbstractItemTest.java | 111 ++++++++++++++++++ 6 files changed, 337 insertions(+), 18 deletions(-) create mode 100644 core/src/main/java/jenkins/util/xml/RestrictiveEntityResolver.java create mode 100644 core/src/main/java/jenkins/util/xml/XMLUtils.java create mode 100644 core/src/test/java/jenkins/xml/XMLUtilsTest.java create mode 100644 test/src/test/java/hudson/model/AbstractItemTest.java diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 20f21850db..3c3f8a9c12 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -43,6 +43,8 @@ import hudson.util.AlternativeUiTextProvider.Message; import hudson.util.AtomicFileWriter; import hudson.util.IOUtils; import jenkins.model.Jenkins; +import jenkins.util.xml.XMLUtils; + import org.apache.tools.ant.taskdefs.Copy; import org.apache.tools.ant.types.FileSet; import org.kohsuke.stapler.WebMethod; @@ -61,12 +63,11 @@ import org.kohsuke.stapler.HttpDeletable; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.CmdLineException; import org.kohsuke.stapler.interceptor.RequirePOST; +import org.xml.sax.SAXException; import javax.servlet.ServletException; import javax.xml.transform.Source; -import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; @@ -564,25 +565,24 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet } /** - * Updates Job by its XML definition. + * Updates an Item by its XML definition. + * @param source source of the Item's new definition. + * The source should be either a StreamSource or a SAXSource, other + * sources may not be handled. * @since 1.473 */ public void updateByXml(Source source) throws IOException { checkPermission(CONFIGURE); XmlFile configXmlFile = getConfigFile(); - AtomicFileWriter out = new AtomicFileWriter(configXmlFile.getFile()); + final AtomicFileWriter out = new AtomicFileWriter(configXmlFile.getFile()); try { try { - // this allows us to use UTF-8 for storing data, - // plus it checks any well-formedness issue in the submitted - // data - Transformer t = TransformerFactory.newInstance() - .newTransformer(); - t.transform(source, - new StreamResult(out)); + XMLUtils.safeTransform(source, new StreamResult(out)); out.close(); } catch (TransformerException e) { throw new IOException("Failed to persist config.xml", e); + } catch (SAXException e) { + throw new IOException("Failed to persist config.xml", e); } // try to reflect the changes by reloading @@ -598,6 +598,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet // if everything went well, commit this new version out.commit(); SaveableListener.fireOnChange(this, getConfigFile()); + } finally { out.abort(); // don't leave anything behind } diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java index 0d531ea86e..3677d6b686 100644 --- a/core/src/main/java/hudson/model/View.java +++ b/core/src/main/java/hudson/model/View.java @@ -58,6 +58,8 @@ import hudson.widgets.Widget; import jenkins.model.Jenkins; import jenkins.model.ModelObjectWithChildren; import jenkins.util.ProgressiveRendering; +import jenkins.util.xml.XMLUtils; + import net.sf.json.JSON; import net.sf.json.JSONArray; import net.sf.json.JSONObject; @@ -75,9 +77,7 @@ import org.kohsuke.stapler.interceptor.RequirePOST; import javax.servlet.ServletException; import javax.servlet.http.HttpServletResponse; import javax.xml.transform.Source; -import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; import java.io.BufferedInputStream; @@ -106,6 +106,7 @@ import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import static jenkins.model.Jenkins.*; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.xml.sax.SAXException; /** * Encapsulates the rendering of the list of {@link TopLevelItem}s @@ -1061,7 +1062,10 @@ public abstract class View extends AbstractModelObject implements AccessControll } /** - * Updates Job by its XML definition. + * Updates the View with the new XML definition. + * @param source source of the Item's new definition. + * The source should be either a StreamSource or SAXSource, other sources + * may not be handled. */ public void updateByXml(Source source) throws IOException { checkPermission(CONFIGURE); @@ -1070,13 +1074,12 @@ public abstract class View extends AbstractModelObject implements AccessControll // this allows us to use UTF-8 for storing data, // plus it checks any well-formedness issue in the submitted // data - Transformer t = TransformerFactory.newInstance() - .newTransformer(); - t.transform(source, - new StreamResult(out)); + XMLUtils.safeTransform(source, new StreamResult(out)); out.close(); } catch (TransformerException e) { throw new IOException("Failed to persist configuration.xml", e); + } catch (SAXException e) { + throw new IOException("Failed to persist configuration.xml", e); } // try to reflect the changes by reloading diff --git a/core/src/main/java/jenkins/util/xml/RestrictiveEntityResolver.java b/core/src/main/java/jenkins/util/xml/RestrictiveEntityResolver.java new file mode 100644 index 0000000000..87079b551e --- /dev/null +++ b/core/src/main/java/jenkins/util/xml/RestrictiveEntityResolver.java @@ -0,0 +1,31 @@ +package jenkins.util.xml; + +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +import java.io.IOException; + +/** + * An EntityResolver that will fail to resolve any entities. + * Useful in preventing External XML Entity injection attacks. + * + * @since TODO + */ +public final class RestrictiveEntityResolver implements EntityResolver { + + public final static RestrictiveEntityResolver INSTANCE = new RestrictiveEntityResolver(); + + private RestrictiveEntityResolver() { + // prevent multiple instantiation. + super(); + } + + /** + * Throws a SAXException if this tried to resolve any entity. + */ + @Override + public InputSource resolveEntity(String publicId, String systemId) throws SAXException, IOException { + throw new SAXException("Refusing to resolve entity with publicId(" + publicId + ") and systemId (" + systemId + ")"); + } +} diff --git a/core/src/main/java/jenkins/util/xml/XMLUtils.java b/core/src/main/java/jenkins/util/xml/XMLUtils.java new file mode 100644 index 0000000000..b235b4af1d --- /dev/null +++ b/core/src/main/java/jenkins/util/xml/XMLUtils.java @@ -0,0 +1,79 @@ +package jenkins.util.xml; + +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.XMLReaderFactory; + +import java.util.logging.LogManager; +import java.util.logging.Logger; +import javax.annotation.Nonnull; +import javax.xml.XMLConstants; +import javax.xml.transform.Result; +import javax.xml.transform.Source; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.sax.SAXSource; +import javax.xml.transform.sax.SAXTransformerFactory; + +/** + * Utilities useful when working with various XML types. + */ +public final class XMLUtils { + + private final static Logger LOGGER = LogManager.getLogManager().getLogger(XMLUtils.class.getName()); + + /** + * Transform the source to the output in a manner that is protected against XXE attacks. + * If the transform can not be completed safely then an IOException is thrown. + * Note - to turn off safety set the system property disableXXEPrevention to true. + * @param source The XML input to transform. - This should be a StreamSource or a + * SAXSource in order to be able to prevent XXE attacks. + * @param out The Result of transforming the source. + */ + public static void safeTransform(@Nonnull Source source, @Nonnull Result out) throws TransformerException, + SAXException { + + InputSource src = SAXSource.sourceToInputSource(source); + if (src != null) { + SAXTransformerFactory stFactory = (SAXTransformerFactory) TransformerFactory.newInstance(); + stFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + XMLReader xmlReader = XMLReaderFactory.createXMLReader(); + // defend against XXE + xmlReader.setEntityResolver(RestrictiveEntityResolver.INSTANCE); + SAXSource saxSource = new SAXSource(xmlReader, src); + _transform(saxSource, out); + } + else { + // for some reason we could not convert source + // this applies to DOMSource and StAXSource - and possibly 3rd party implementations... + // a DOMSource can already be compromised as it is parsed by the time it gets to us. + if (Boolean.getBoolean("disableXXEPrevention")) { + LOGGER.warning("Parsing XML with XXEPrevention disabled!"); + _transform(source, out); + } + else { + throw new TransformerException("Could not convert source of type " + source.getClass() + " and " + + "XXEPrevention is enabled."); + } + } + } + + /** + * potentially unsafe XML transformation. + * @param source The XML input to transform. + * @param out The Result of transforming the source. + */ + private static void _transform(Source source, Result out) throws TransformerException { + TransformerFactory factory = TransformerFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + + // this allows us to use UTF-8 for storing data, + // plus it checks any well-formedness issue in the submitted data. + Transformer t = factory.newTransformer(); + t.transform(source, out); + } + +} diff --git a/core/src/test/java/jenkins/xml/XMLUtilsTest.java b/core/src/test/java/jenkins/xml/XMLUtilsTest.java new file mode 100644 index 0000000000..4b53c930b8 --- /dev/null +++ b/core/src/test/java/jenkins/xml/XMLUtilsTest.java @@ -0,0 +1,94 @@ +/* + * The MIT License + * + * Copyright 2015 James Nord. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package jenkins.xml; + +import jenkins.util.xml.XMLUtils; + +import org.apache.commons.io.output.NullOutputStream; +import org.junit.Test; + +import java.io.StringReader; +import javax.xml.transform.TransformerException; +import javax.xml.transform.stream.StreamResult; +import javax.xml.transform.stream.StreamSource; + +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +public class XMLUtilsTest { + + @Test() + // SECURITY-167 + public void testSafeTransformDoesNotProcessForeignResources() throws Exception { + final String xml = "\n" + + "\n" + + "]>\n" + + "\n" + + " \n" + + " &foo;\n" + + " false\n" + + " \n" + + " \n" + + " true\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + + try { + XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(new NullOutputStream())); + fail("Exception should have been thrown"); + } catch (TransformerException ex) { + assertThat(ex.getMessage(), containsString("Refusing to resolve entity")); + } + + } + + + @Test() + // SECURITY-167 + public void testUpdateByXmlIDoesNotFail() throws Exception { + final String xml = "\n" + + "\n" + + " \n" + + " &\n" + + " false\n" + + " \n" + + " \n" + + " true\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(new NullOutputStream())); + } + +} diff --git a/test/src/test/java/hudson/model/AbstractItemTest.java b/test/src/test/java/hudson/model/AbstractItemTest.java new file mode 100644 index 0000000000..bd2c1af613 --- /dev/null +++ b/test/src/test/java/hudson/model/AbstractItemTest.java @@ -0,0 +1,111 @@ +/* + * The MIT License + * + * Copyright 2015 James Nord. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package hudson.model; + +import hudson.model.FreeStyleProject; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.w3c.dom.Document; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; +import org.w3c.dom.Text; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.stream.StreamSource; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.StringReader; + +import static org.hamcrest.core.Is.is; +import static org.hamcrest.core.IsNot.not; +import static org.hamcrest.core.IsNull.nullValue; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +public class AbstractItemTest { + + @Rule + public JenkinsRule jenkinsRule = new JenkinsRule(); + + @Test() + // SECURITY-167 + public void testUpdateByXmlIDoesNotProcessForeignResources() throws Exception { + final String xml = "\n" + + "\n" + + "]>\n" + + "\n" + + " \n" + + " &foo;\n" + + " false\n" + + " \n" + + " \n" + + " true\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + FreeStyleProject project = jenkinsRule.createFreeStyleProject("security-167"); + + try { + project.updateByXml((StreamSource) new StreamSource(new StringReader(xml))); + fail("Exception should have been thrown"); + } catch (IOException ex) { + assertThat(ex.getCause(), not(nullValue())); + assertThat(ex.getCause().getMessage(), containsString("Refusing to resolve entity")); + } + + } + + + @Test() + // SECURITY-167 + public void testUpdateByXmlIDoesNotFail() throws Exception { + final String xml = "\n" + + "\n" + + " \n" + + " &\n" + + " false\n" + + " \n" + + " \n" + + " true\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""; + + FreeStyleProject project = jenkinsRule.createFreeStyleProject("security-167"); + project.updateByXml((StreamSource) new StreamSource(new StringReader(xml))); + assertThat(project.getDescription(), is("&")); // the entity is transformed + } + +} From c58ea9fe79486f3998f5698262c6bae4646e35d3 Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 3 Feb 2015 15:59:01 +0000 Subject: [PATCH 2/5] [SECURITY-167] attempt to set JAXP properties. Atempt to set SAX specific properties to defend against XXE attacks. --- core/src/main/java/jenkins/util/xml/XMLUtils.java | 11 +++++++++++ core/src/test/java/jenkins/xml/XMLUtilsTest.java | 13 ++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/jenkins/util/xml/XMLUtils.java b/core/src/main/java/jenkins/util/xml/XMLUtils.java index b235b4af1d..ddfc60509b 100644 --- a/core/src/main/java/jenkins/util/xml/XMLUtils.java +++ b/core/src/main/java/jenkins/util/xml/XMLUtils.java @@ -41,7 +41,18 @@ public final class XMLUtils { stFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); XMLReader xmlReader = XMLReaderFactory.createXMLReader(); + try { + xmlReader.setFeature("http://xml.org/sax/features/external-general-entities", false); + } + catch (SAXException ignored) { /* ignored */ } + try { + xmlReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + } + catch (SAXException ignored) { /* ignored */ } // defend against XXE + // the above features should strip out entities - however the feature may not be supported depending + // on the xml implementation used and this is out of our control. + // So add a fallback plan if all else fails. xmlReader.setEntityResolver(RestrictiveEntityResolver.INSTANCE); SAXSource saxSource = new SAXSource(xmlReader, src); _transform(saxSource, out); diff --git a/core/src/test/java/jenkins/xml/XMLUtilsTest.java b/core/src/test/java/jenkins/xml/XMLUtilsTest.java index 4b53c930b8..18bd0052b3 100644 --- a/core/src/test/java/jenkins/xml/XMLUtilsTest.java +++ b/core/src/test/java/jenkins/xml/XMLUtilsTest.java @@ -30,6 +30,7 @@ import org.apache.commons.io.output.NullOutputStream; import org.junit.Test; import java.io.StringReader; +import java.io.StringWriter; import javax.xml.transform.TransformerException; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; @@ -61,9 +62,11 @@ public class XMLUtilsTest { ""; + StringWriter stringWriter = new StringWriter(); try { - XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(new NullOutputStream())); - fail("Exception should have been thrown"); + XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(stringWriter)); + // if no exception then JAXP is swallowing these - so there should be no entity in the description. + assertThat(stringWriter.toString(), containsString("")); } catch (TransformerException ex) { assertThat(ex.getMessage(), containsString("Refusing to resolve entity")); } @@ -88,7 +91,11 @@ public class XMLUtilsTest { " \n" + ""; - XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(new NullOutputStream())); + StringWriter stringWriter = new StringWriter(); + + XMLUtils.safeTransform(new StreamSource(new StringReader(xml)), new StreamResult(stringWriter)); + // make sure that normal entities are retained. + assertThat(stringWriter.toString(), containsString("&")); } } From 2a1b8542cb6a9b9f7d9508711dcb45c4c7599ddb Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 3 Feb 2015 18:31:52 +0000 Subject: [PATCH 3/5] [SECURITY-167] fix failing unit test. --- test/src/test/java/hudson/model/AbstractItemTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/src/test/java/hudson/model/AbstractItemTest.java b/test/src/test/java/hudson/model/AbstractItemTest.java index bd2c1af613..6c186fd939 100644 --- a/test/src/test/java/hudson/model/AbstractItemTest.java +++ b/test/src/test/java/hudson/model/AbstractItemTest.java @@ -45,6 +45,7 @@ import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsNot.not; import static org.hamcrest.core.IsNull.nullValue; import static org.hamcrest.core.StringContains.containsString; +import static org.hamcrest.text.IsEmptyString.isEmptyOrNullString; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; @@ -74,10 +75,11 @@ public class AbstractItemTest { ""; FreeStyleProject project = jenkinsRule.createFreeStyleProject("security-167"); - + project.setDescription("Wibble"); try { - project.updateByXml((StreamSource) new StreamSource(new StringReader(xml))); - fail("Exception should have been thrown"); + project.updateByXml(new StreamSource(new StringReader(xml))); + // if we didn't fail JAXP has thrown away the entity. + assertThat(project.getDescription(), isEmptyOrNullString()); } catch (IOException ex) { assertThat(ex.getCause(), not(nullValue())); assertThat(ex.getCause().getMessage(), containsString("Refusing to resolve entity")); @@ -88,7 +90,7 @@ public class AbstractItemTest { @Test() // SECURITY-167 - public void testUpdateByXmlIDoesNotFail() throws Exception { + public void testhamyXmlIDoesNotFail() throws Exception { final String xml = "\n" + "\n" + " \n" + From b579a53cfd8cda22691ffbf8afa0fdde5298eb0a Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 4 Feb 2015 10:35:03 +0000 Subject: [PATCH 4/5] [SECURITY-167] update log message when prevention is disabled. add stack trace so the offending plugin/code can be identified --- core/src/main/java/jenkins/util/xml/XMLUtils.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/jenkins/util/xml/XMLUtils.java b/core/src/main/java/jenkins/util/xml/XMLUtils.java index ddfc60509b..5aab9b5567 100644 --- a/core/src/main/java/jenkins/util/xml/XMLUtils.java +++ b/core/src/main/java/jenkins/util/xml/XMLUtils.java @@ -5,6 +5,7 @@ import org.xml.sax.SAXException; import org.xml.sax.XMLReader; import org.xml.sax.helpers.XMLReaderFactory; +import java.util.logging.Level; import java.util.logging.LogManager; import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -23,6 +24,7 @@ import javax.xml.transform.sax.SAXTransformerFactory; public final class XMLUtils { private final static Logger LOGGER = LogManager.getLogManager().getLogger(XMLUtils.class.getName()); + private final static String DISABLED_PROPERTY_NAME = XMLUtils.class.getName() + ".disableXXEPrevention"; /** * Transform the source to the output in a manner that is protected against XXE attacks. @@ -61,8 +63,12 @@ public final class XMLUtils { // for some reason we could not convert source // this applies to DOMSource and StAXSource - and possibly 3rd party implementations... // a DOMSource can already be compromised as it is parsed by the time it gets to us. - if (Boolean.getBoolean("disableXXEPrevention")) { - LOGGER.warning("Parsing XML with XXEPrevention disabled!"); + if (Boolean.getBoolean(DISABLED_PROPERTY_NAME)) { + LOGGER.log(Level.WARNING, "XML external entity (XXE) prevention has been disabled by the system " + + "property {0}=true Your system may be vulnerable to XXE attacks.", DISABLED_PROPERTY_NAME); + if (LOGGER.isLoggable(Level.FINE)) { + LOGGER.log(Level.FINE, "Caller stack trace: ", new Exception("XXE Prevention caller history")); + } _transform(source, out); } else { From 01a087d127d7a871eac02b03143464e2f4c6d48c Mon Sep 17 00:00:00 2001 From: Ryan Campbell Date: Thu, 29 Jan 2015 08:45:54 -0600 Subject: [PATCH 5/5] [SECURITY-166] Prevent the creation of anonymous, system or unknown users. These users can still be instantiated, as would happen if there is no security and an anonymous user triggers a build -- the anonymous user would correctly be created and added to the User list. This fix merely prevents the saving of that user, and therefore prevents them from logging in. There may be some plugins which trigger a build as the SYSTEM user, and that is not prohibited here. Also prevent full names of 'anonymous', 'system' or 'uknown'. As discussed on SECURITY-166 this may encumber auditing since full names are used in most places in the UI --- core/src/main/java/hudson/model/User.java | 38 +++++++- .../security/HudsonPrivateSecurityRealm.java | 8 ++ .../hudson/model/Messages.properties | 3 + .../HudsonPrivateSecurityRealm2Test.java | 93 +++++++++++++++++++ .../hudson/security/pages/SignupPage.java | 65 +++++++++++++ 5 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2Test.java create mode 100644 test/src/test/java/hudson/security/pages/SignupPage.java diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java index 3d9cdbb5b5..b09cf7ea49 100644 --- a/core/src/main/java/hudson/model/User.java +++ b/core/src/main/java/hudson/model/User.java @@ -34,6 +34,7 @@ import hudson.security.AccessControlled; import hudson.security.Permission; import hudson.security.SecurityRealm; import hudson.util.FormApply; +import hudson.util.FormValidation; import hudson.util.RunList; import hudson.util.XStream2; import jenkins.model.Jenkins; @@ -46,6 +47,8 @@ import org.acegisecurity.providers.UsernamePasswordAuthenticationToken; import org.acegisecurity.providers.anonymous.AnonymousAuthenticationToken; import org.acegisecurity.userdetails.UserDetails; import org.acegisecurity.userdetails.UsernameNotFoundException; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.springframework.dao.DataAccessException; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; @@ -102,7 +105,12 @@ import javax.annotation.Nonnull; */ @ExportedBean public class User extends AbstractModelObject implements AccessControlled, DescriptorByNameOwner, Saveable, Comparable, ModelObjectWithContextMenu { - + + /** + * These usernames should not be used by real users logging into Jenkins. Therefore, we prevent + * users with these names from being saved. + */ + public static final String[] ILLEGAL_PERSISTED_USERNAMES = new String[]{"anonymous", "system", "unknown"}; private transient final String id; private volatile String fullName; @@ -500,10 +508,36 @@ public class User extends AbstractModelObject implements AccessControlled, Descr return new File(Jenkins.getInstance().getRootDir(), "users"); } + /** + * Is the ID allowed? Some are prohibited for security reasons. See SECURITY-166. + *

+ * Note that this is only enforced when saving. These users are often created + * via the constructor (and even listed on /asynchPeople), but our goal is to + * prevent anyone from logging in as these users. Therefore, we prevent + * saving a User with one of these ids. + * + * @return true if the username or fullname is valid + */ + //TODO: Remove Restricted and add @since when merging to stable-rc + @Restricted(NoExternalUse.class) + public static boolean isIdOrFullnameAllowed(String id) { + for (String invalidId : ILLEGAL_PERSISTED_USERNAMES) { + if (id.equalsIgnoreCase(invalidId)) + return false; + } + return true; + } + /** * Save the settings to a file. */ - public synchronized void save() throws IOException { + public synchronized void save() throws IOException, FormValidation { + if (! isIdOrFullnameAllowed(id)) { + throw FormValidation.error(Messages.User_IllegalUsername(id)); + } + if (! isIdOrFullnameAllowed(fullName)) { + throw FormValidation.error(Messages.User_IllegalFullname(fullName)); + } if(BulkChange.contains(this)) return; getConfigFile().write(this); SaveableListener.fireOnChange(this, getConfigFile()); diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index f132fc7c5e..42bd998305 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -329,6 +329,14 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea if(si.email==null || !si.email.contains("@")) si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_InvalidEmailAddress(); + if (! User.isIdOrFullnameAllowed(si.username)) { + si.errorMessage = hudson.model.Messages.User_IllegalUsername(si.username); + } + + if (! User.isIdOrFullnameAllowed(si.fullname)) { + si.errorMessage = hudson.model.Messages.User_IllegalFullname(si.fullname); + } + if(si.errorMessage!=null) { // failed. ask the user to try again. req.setAttribute("data",si); diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties index 83fd5f48d9..ad5711956e 100644 --- a/core/src/main/resources/hudson/model/Messages.properties +++ b/core/src/main/resources/hudson/model/Messages.properties @@ -357,3 +357,6 @@ Jenkins.CheckDisplayName.NameNotUniqueWarning=The display name, "{0}", is used a Jenkins.CheckDisplayName.DisplayNameNotUniqueWarning=The display name, "{0}", is already in use by another job and could cause confusion and delay. Jenkins.NotAllowedName="{0}" is not allowed name + +User.IllegalUsername="{0}" is prohibited as a username for security reasons. +User.IllegalFullname="{0}" is prohibited as a full name for security reasons. \ No newline at end of file diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2Test.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2Test.java new file mode 100644 index 0000000000..0d87c08f45 --- /dev/null +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2Test.java @@ -0,0 +1,93 @@ +package hudson.security; + +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.User; +import hudson.security.pages.SignupPage; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +import java.util.Collections; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.*; + +/** + * Tests the signup page of {@link hudson.security.HudsonPrivateSecurityRealm} + * TODO: Add to {@link hudson.security.HudsonPrivateSecurityRealmTest} going forward + */ +public class HudsonPrivateSecurityRealm2Test { + + + @Rule + public JenkinsRule rule = new JenkinsRule(); + + @Test + public void signup() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + rule.jenkins.setSecurityRealm(securityRealm); + JenkinsRule.WebClient wc = rule.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername("alice"); + signup.enterPassword("alice"); + signup.enterFullName("Alice User"); + signup.enterEmail("alice@nowhere.com"); + HtmlPage success = signup.submit(rule); + assertThat(success.getElementById("main-panel").getTextContent(), containsString("Success")); + assertThat(success.getElementById("login-field").getTextContent(), containsString("Alice User")); + + assertEquals("Alice User", securityRealm.getUser("alice").getDisplayName()); + + } + + // @Issue("SECURITY-166") + @Test + public void anonymousCantSignup() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + rule.jenkins.setSecurityRealm(securityRealm); + JenkinsRule.WebClient wc = rule.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername("anonymous"); + signup.enterFullName("Bob"); + signup.enterPassword("nothing"); + signup.enterEmail("noone@nowhere.com"); + signup = new SignupPage(signup.submit(rule)); + signup.assertErrorContains("prohibited as a username"); + assertNull(User.get("anonymous", false, Collections.emptyMap())); + } + + // @Issue("SECURITY-166") + @Test + public void systemCantSignup() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + rule.jenkins.setSecurityRealm(securityRealm); + JenkinsRule.WebClient wc = rule.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername("system"); + signup.enterFullName("Bob"); + signup.enterPassword("nothing"); + signup.enterEmail("noone@nowhere.com"); + signup = new SignupPage(signup.submit(rule)); + signup.assertErrorContains("prohibited as a username"); + assertNull(User.get("system",false, Collections.emptyMap())); + } + + /** + * We don't allow prohibited fullnames since this may encumber auditing. + */ + // @Issue("SECURITY-166") + @Test + public void fullNameOfUnknownCantSignup() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + rule.jenkins.setSecurityRealm(securityRealm); + JenkinsRule.WebClient wc = rule.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername("unknown2"); + signup.enterPassword("unknown2"); + signup.enterFullName("unknown"); + signup.enterEmail("noone@nowhere.com"); + signup = new SignupPage(signup.submit(rule)); + signup.assertErrorContains("prohibited as a full name"); + assertNull(User.get("unknown2",false, Collections.emptyMap())); + } +} diff --git a/test/src/test/java/hudson/security/pages/SignupPage.java b/test/src/test/java/hudson/security/pages/SignupPage.java new file mode 100644 index 0000000000..5007bce1b5 --- /dev/null +++ b/test/src/test/java/hudson/security/pages/SignupPage.java @@ -0,0 +1,65 @@ +package hudson.security.pages; + +import com.gargoylesoftware.htmlunit.html.HtmlForm; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import org.jvnet.hudson.test.JenkinsRule; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.*; + +/** + * The the signup page for {@link hudson.security.HudsonPrivateSecurityRealm} + */ +public class SignupPage { + + public HtmlForm signupForm; + public final HtmlPage signupPage; + + public SignupPage(HtmlPage signupPage) { + this.signupPage = signupPage; + + assertNotNull("The sign up page has a username field.", this.signupPage.getElementById("username")); + for (HtmlForm signupForm : this.signupPage.getForms()) { + if (signupForm.getInputsByName("username").size() == 0) + continue; + this.signupForm = signupForm; + } + + } + + + + public void enterUsername(String username) { + signupForm.getInputByName("username").setValueAttribute(username); + } + + /** + * Enters the password in password1 and password2. + * You can then call {@link #enterPassword2(String)} if you want them to be different. + * @param password + */ + public void enterPassword(String password) { + signupForm.getInputByName("password1").setValueAttribute(password); + signupForm.getInputByName("password2").setValueAttribute(password); + } + + public void enterPassword2(String password2) { + signupForm.getInputByName("password2").setValueAttribute(password2); + } + + public void enterFullName(String fullName) { + signupForm.getInputByName("fullname").setValueAttribute(fullName); + } + + public void enterEmail(String email) { + signupForm.getInputByName("email").setValueAttribute(email); + } + + public HtmlPage submit(JenkinsRule rule) throws Exception { + return rule.submit(signupForm); + } + + public void assertErrorContains(String msg) { + assertThat(signupForm.getElementById("main-panel").getTextContent(),containsString(msg)); + } +}