Merge branch 'security' into security-rc

Conflicts:
	core/src/main/java/hudson/model/AbstractItem.java
	core/src/main/resources/hudson/model/Messages.properties

core/src/main/java/hudson/model/AbstractItem.java

@@ -45,6 +45,8 @@ import jenkins.model.DirectlyModifiableTopLevelItemGroup;
 import jenkins.model.Jenkins;
 import jenkins.security.NotReallyRoleSensitiveCallable;
 import org.acegisecurity.Authentication;
+import jenkins.util.xml.XMLUtils;
+
 import org.apache.tools.ant.taskdefs.Copy;
 import org.apache.tools.ant.types.FileSet;
 import org.kohsuke.stapler.WebMethod;
@@ -67,12 +69,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;
 
@@ -627,25 +628,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 <code>StreamSource</code> or a <code>SAXSource</code>, 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
@@ -667,6 +667,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
         }

core/src/main/java/hudson/model/User.java

@@ -35,6 +35,7 @@ import hudson.security.Permission;
 import hudson.security.SecurityRealm;
 import hudson.security.UserMayOrMayNotExistException;
 import hudson.util.FormApply;
+import hudson.util.FormValidation;
 import hudson.util.RunList;
 import hudson.util.XStream2;
 import jenkins.model.IdStrategy;
@@ -50,6 +51,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;
@@ -109,7 +112,12 @@ import javax.annotation.Nullable;
  */
 @ExportedBean
 public class User extends AbstractModelObject implements AccessControlled, DescriptorByNameOwner, Saveable, Comparable<User>, 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;
@@ -642,10 +650,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.
+     * <p/>
+     * 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());

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
@@ -1063,7 +1064,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 <code>StreamSource</code> or <code>SAXSource</code>, other sources
+     *               may not be handled.
      */
     public void updateByXml(Source source) throws IOException {
         checkPermission(CONFIGURE);
@@ -1072,13 +1076,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

core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java

@@ -337,6 +337,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);

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 + ")");
+    }
+}

core/src/main/java/jenkins/util/xml/XMLUtils.java

@@ -0,0 +1,96 @@
+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.Level;
+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());
+    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.
+     * If the transform can not be completed safely then an IOException is thrown.
+     * Note - to turn off safety set the system property <code>disableXXEPrevention</code> to <code>true</code>.
+     * @param source The XML input to transform. - This should be a <code>StreamSource</code> or a
+     *               <code>SAXSource</code> in order to be able to prevent XXE attacks.
+     * @param out The Result of transforming the <code>source</code>.
+     */
+    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();
+            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);
+        }
+        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(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 {
+                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 <code>source</code>.
+     */
+    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);
+    }
+
+}