Merge branch 'security-stable-1.609' into security-stable-1.625

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

@@ -54,6 +54,7 @@ import jenkins.model.Jenkins;
 import jenkins.util.io.OnMaster;
 import org.acegisecurity.Authentication;
 import org.acegisecurity.context.SecurityContext;
+import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.io.input.CountingInputStream;
 import org.apache.commons.io.output.NullOutputStream;
 import org.jvnet.localizer.Localizable;
@@ -61,9 +62,11 @@ import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 
+import javax.annotation.Nonnull;
 import javax.net.ssl.SSLHandshakeException;
 import javax.servlet.ServletException;
 import java.io.File;
+import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -71,6 +74,10 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.UnknownHostException;
+import java.security.DigestInputStream;
+import java.security.DigestOutputStream;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -748,6 +755,15 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
          * @see DownloadJob
          */
         public File download(DownloadJob job, URL src) throws IOException {
+            MessageDigest sha1 = null;
+            try {
+                sha1 = MessageDigest.getInstance("SHA-1");
+            } catch (NoSuchAlgorithmException ignored) {
+                // Irrelevant as the Java spec says SHA-1 must exist. Still, if this fails
+                // the DownloadJob will just have computedSha1 = null and that is expected
+                // to be handled by caller
+            }
+
             CountingInputStream in = null;
             OutputStream out = null;
             URLConnection con = null;
@@ -761,6 +777,9 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
                 File dst = job.getDestination();
                 File tmp = new File(dst.getPath()+".tmp");
                 out = new FileOutputStream(tmp);
+                if (sha1 != null) {
+                    out = new DigestOutputStream(out, sha1);
+                }
 
                 LOGGER.info("Downloading "+job.getName());
                 Thread t = Thread.currentThread();
@@ -774,6 +793,7 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
                 } catch (IOException e) {
                     throw new IOException("Failed to load "+src+" to "+tmp,e);
                 } finally {
+                    IOUtils.closeQuietly(out);
                     t.setName(oldName);
                 }
 
@@ -784,6 +804,10 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
                     throw new IOException("Inconsistent file length: expected "+total+" but only got "+tmp.length());
                 }
 
+                if (sha1 != null) {
+                    byte[] digest = sha1.digest();
+                    job.computedSHA1 = Base64.encodeBase64String(digest);
+                }
                 return tmp;
             } catch (IOException e) {
                 // assist troubleshooting in case of e.g. "too many redirects" by printing actual URL
@@ -1104,6 +1128,19 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
          */
         protected abstract void onSuccess();
 
+        /**
+         * During download, an attempt is made to compute the SHA-1 checksum of the file.
+         *
+         * @since TODO
+         */
+        // TODO no new API in LTS, but remove for mainline
+        @Restricted(NoExternalUse.class)
+        @CheckForNull
+        protected String getComputedSHA1() {
+            return computedSHA1;
+        }
+
+        private String computedSHA1;
 
         private Authentication authentication;
 
@@ -1255,6 +1292,26 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
         }
     }
 
+    /**
+     * If expectedSHA1 is non-null, ensure that actualSha1 is the same value, otherwise throw.
+     *
+     * Utility method for InstallationJob and HudsonUpgradeJob.
+     *
+     * @throws IOException when checksums don't match, or actual checksum was null.
+     */
+    private void verifyChecksums(String expectedSHA1, String actualSha1, File downloadedFile) throws IOException {
+        if (expectedSHA1 != null) {
+            if (actualSha1 == null) {
+                // refuse to install if SHA-1 could not be computed
+                throw new IOException("Failed to compute SHA-1 of downloaded file, refusing installation");
+            }
+            if (!expectedSHA1.equals(actualSha1)) {
+                throw new IOException("Downloaded file " + downloadedFile.getAbsolutePath() + " does not match expected SHA-1, expected '" + expectedSHA1 + "', actual '" + actualSha1 + "'");
+                // keep 'downloadedFile' around for investigating what's going on
+            }
+        }
+    }
+
     /**
      * Represents the state of the installation activity of one plugin.
      */
@@ -1347,18 +1404,24 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
          */
         @Override
         protected void replace(File dst, File src) throws IOException {
-        	File bak = Util.changeExtension(dst,".bak");
-        	
+
+            verifyChecksums(plugin.getSha1(), getComputedSHA1(), src);
+
+            File bak = Util.changeExtension(dst, ".bak");
             bak.delete();
+
             final File legacy = getLegacyDestination();
-			if(legacy.exists()){
-            	legacy.renameTo(bak);
-            }else{
-            	dst.renameTo(bak);
+            if (legacy.exists()) {
+                if (!legacy.renameTo(bak)) {
+                    legacy.delete();
+                }
             }
-            legacy.delete();
-            dst.delete(); // any failure up to here is no big deal
-            
+            if (dst.exists()) {
+                if (!dst.renameTo(bak)) {
+                    dst.delete();
+                }
+            }
+
             if(!src.renameTo(dst)) {
                 throw new IOException("Failed to rename "+src+" to "+dst);
             }
@@ -1476,6 +1539,8 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas
 
         @Override
         protected void replace(File dst, File src) throws IOException {
+            String expectedSHA1 = site.getData().core.getSha1();
+            verifyChecksums(expectedSHA1, getComputedSHA1(), src);
             Lifecycle.get().rewriteHudsonWar(src);
         }
     }

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

@@ -27,6 +27,7 @@ package hudson.model;
 
 import hudson.PluginManager;
 import hudson.PluginWrapper;
+import hudson.Util;
 import hudson.lifecycle.Lifecycle;
 import hudson.model.UpdateCenter.UpdateCenterJob;
 import hudson.util.FormValidation;
@@ -126,6 +127,8 @@ public class UpdateSite {
      */
     private final String url;
 
+
+
     public UpdateSite(String id, String url) {
         this.id = id;
         this.url = url;
@@ -508,6 +511,11 @@ public class UpdateSite {
         @Exported
         public final String url;
 
+
+        // non-private, non-final for test
+        @Restricted(NoExternalUse.class)
+        /* final */ String sha1;
+
         public Entry(String sourceId, JSONObject o) {
             this(sourceId, o, null);
         }
@@ -516,6 +524,11 @@ public class UpdateSite {
             this.sourceId = sourceId;
             this.name = o.getString("name");
             this.version = o.getString("version");
+
+            // Trim this to prevent issues when the other end used Base64.encodeBase64String that added newlines
+            // to the end in old commons-codec. Not the case on updates.jenkins-ci.org, but let's be safe.
+            this.sha1 = Util.fixEmptyAndTrim(o.optString("sha1"));
+
             String url = o.getString("url");
             if (!URI.create(url).isAbsolute()) {
                 if (baseURL == null) {
@@ -526,6 +539,18 @@ public class UpdateSite {
             this.url = url;
         }
 
+        /**
+         * The base64 encoded binary SHA-1 checksum of the file.
+         * Can be null if not provided by the update site.
+         * @since TODO
+         */
+        // TODO @Exported assuming we want this in the API
+        // TODO No new API in LTS, remove for mainline
+        @Restricted(NoExternalUse.class)
+        public String getSha1() {
+            return sha1;
+        }
+
         /**
          * Checks if the specified "current version" is older than the version of this entry.
          *

test/pom.xml

@@ -73,6 +73,10 @@ THE SOFTWARE.
           <groupId>org.apache.httpcomponents</groupId>
           <artifactId>httpcore</artifactId>
         </exclusion>
+        <exclusion>
+          <groupId>commons-codec</groupId>
+          <artifactId>commons-codec</artifactId>
+        </exclusion>
       </exclusions>
     </dependency>
     <dependency>
@@ -140,6 +144,12 @@ THE SOFTWARE.
       <groupId>net.sourceforge.htmlunit</groupId>
       <artifactId>htmlunit</artifactId>
       <version>2.18</version>
+      <exclusions>
+        <exclusion>
+          <groupId>commons-codec</groupId>
+          <artifactId>commons-codec</artifactId>
+        </exclusion>
+      </exclusions>
     </dependency>
     <dependency><!-- we exclude this transient dependency from htmlunit, which we actually need in the test -->
       <groupId>xalan</groupId>

test/src/test/java/hudson/model/UpdateCenter2Test.java

@@ -25,12 +25,16 @@ package hudson.model;
 
 import hudson.model.UpdateCenter.DownloadJob;
 import hudson.model.UpdateCenter.DownloadJob.Success;
+import hudson.model.UpdateCenter.DownloadJob.Failure;
 import static org.junit.Assert.*;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.RandomlyFails;
 
+import java.io.IOException;
+
 /**
  *
  *
@@ -58,4 +62,17 @@ public class UpdateCenter2Test {
         assertEquals(Messages.UpdateCenter_n_a(), j.jenkins.getUpdateCenter().getLastUpdatedString());
     }
 
+    @Issue("SECURITY-234")
+    @Test public void installInvalidChecksum() throws Exception {
+        UpdateSite.neverUpdate = false;
+        j.jenkins.pluginManager.doCheckUpdatesServer(); // load the metadata
+        String wrongChecksum = "ABCDEFG1234567890";
+
+        // usually the problem is the file having a wrong checksum, but changing the expected one works just the same
+        j.jenkins.getUpdateCenter().getSite("default").getPlugin("changelog-history").sha1 = wrongChecksum;
+        DownloadJob job = (DownloadJob) j.jenkins.getUpdateCenter().getPlugin("changelog-history").deploy().get();
+        assertTrue(job.status instanceof Failure);
+        assertTrue("error message references checksum", ((Failure) job.status).problem.getMessage().contains(wrongChecksum));
+    }
+
 }