CVE-2015-7539

@@ -805,8 +805,7 @@ public File download(DownloadJob job, URL src) throws IOException {
 
                 if (sha1 != null) {
                     byte[] digest = sha1.digest();
-                    // need to trim because commons-codec 1.4 used in test chunked output and adds \r\n at the end
-                    job.computedSHA1 = Base64.encodeBase64String(digest).trim();
+                    job.computedSHA1 = Base64.encodeBase64String(digest);
                 }
                 return tmp;
             } catch (IOException e) {
@@ -1302,7 +1301,7 @@ private void verifyChecksums(String expectedSHA1, String actualSha1, File downlo
                 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);
+                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
             }
         }

@@ -524,10 +524,9 @@ public Entry(String sourceId, JSONObject o) {
             this.name = o.getString("name");
             this.version = o.getString("version");
 
-            String sha = Util.fixEmpty(o.optString("sha1"));
             // 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 = (sha == null) ? null : sha.trim();
+            this.sha1 = Util.fixEmptyAndTrim(o.optString("sha1"));
 
             String url = o.getString("url");
             if (!URI.create(url).isAbsolute()) {

@@ -64,6 +64,12 @@ THE SOFTWARE.
       <groupId>${project.groupId}</groupId>
       <artifactId>maven-plugin</artifactId>
       <version>${maven-plugin.version}</version>
+      <exclusions>
+        <exclusion>
+          <groupId>commons-codec</groupId>
+          <artifactId>commons-codec</artifactId>
+        </exclusion>
+      </exclusions>
     </dependency>
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>
@@ -136,6 +142,10 @@ THE SOFTWARE.
           <groupId>xml-apis</groupId>
           <artifactId>xml-apis</artifactId>
         </exclusion>
+        <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 -->

@@ -1288,6 +1288,26 @@ public Installing(int percentage) {
         }
     }
 
+    /**
+     * 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.
      */
@@ -1380,17 +1400,7 @@ public String toString() {
         @Override
         protected void replace(File dst, File src) throws IOException {
 
-            if (plugin.getSha1() != null) {
-                // we have an update site that provides SHA-1 checksums, and this is not a plugin file upload
-                if (getComputedSHA1() == 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 (!plugin.getSha1().equals(getComputedSHA1())) {
-                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + plugin.getSha1() + ", actual " + getComputedSHA1());
-                    // keep 'src' around for investigating what's going on
-                }
-            }
+            verifyChecksums(plugin.getSha1(), getComputedSHA1(), src);
 
             File bak = Util.changeExtension(dst, ".bak");
             bak.delete();
@@ -1525,16 +1535,7 @@ protected void onSuccess() {
         @Override
         protected void replace(File dst, File src) throws IOException {
             String expectedSHA1 = site.getData().core.getSha1();
-            if (expectedSHA1 != null) {
-                if (getComputedSHA1() == 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(getComputedSHA1())) {
-                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + expectedSHA1 + ", actual " + getComputedSHA1());
-                    // keep 'src' around for investigating what's going on
-                }
-            }
+            verifyChecksums(expectedSHA1, getComputedSHA1(), src);
             Lifecycle.get().rewriteHudsonWar(src);
         }
     }

@@ -510,7 +510,10 @@ public static class Entry {
         @Exported
         public final String url;
 
-        private final String sha1;
+
+        // non-private, non-final for test
+        @Restricted(NoExternalUse.class)
+        /* final */ String sha1;
 
         public Entry(String sourceId, JSONObject o) {
             this(sourceId, o, null);

@@ -25,12 +25,16 @@
 
 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));
+    }
+
 }

@@ -62,6 +62,7 @@
 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;
@@ -74,6 +75,7 @@
 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;
@@ -752,6 +754,15 @@ public void postValidate(DownloadJob job, File src) throws IOException {
          * @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;
@@ -765,6 +776,9 @@ public File download(DownloadJob job, URL src) throws IOException {
                 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();
@@ -778,6 +792,7 @@ public File download(DownloadJob job, URL src) throws IOException {
                 } catch (IOException e) {
                     throw new IOException("Failed to load "+src+" to "+tmp,e);
                 } finally {
+                    IOUtils.closeQuietly(out);
                     t.setName(oldName);
                 }
 
@@ -788,6 +803,11 @@ public File download(DownloadJob job, URL src) throws IOException {
                     throw new IOException("Inconsistent file length: expected "+total+" but only got "+tmp.length());
                 }
 
+                if (sha1 != null) {
+                    byte[] digest = sha1.digest();
+                    // need to trim because commons-codec 1.4 used in test chunked output and adds \r\n at the end
+                    job.computedSHA1 = Base64.encodeBase64String(digest).trim();
+                }
                 return tmp;
             } catch (IOException e) {
                 // assist troubleshooting in case of e.g. "too many redirects" by printing actual URL
@@ -1160,25 +1180,6 @@ protected void _run() throws IOException, InstallationStatus {
             File dst = getDestination();
             File tmp = config.download(this, src);
 
-            try {
-                MessageDigest sha1 = MessageDigest.getInstance("SHA-1");
-                DigestInputStream dis = new DigestInputStream(new FileInputStream(tmp), sha1);
-                byte[] unused = new byte[1024];
-                try {
-                    while (dis.read(unused) != -1)
-                        ; // do nothing, just read the entire file
-                } finally {
-                    dis.close();
-                }
-                byte[] digest = sha1.digest();
-                // need to trim because commons-codec 1.4 used in test chunked output and adds \r\n at the end
-                computedSHA1 = Base64.encodeBase64String(digest).trim();
-            } 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
-            }
-
             config.postValidate(this, tmp);
             config.install(this, tmp, dst);
         }
@@ -1380,6 +1381,7 @@ public String toString() {
         protected void replace(File dst, File src) throws IOException {
 
             if (plugin.getSha1() != null) {
+                // we have an update site that provides SHA-1 checksums, and this is not a plugin file upload
                 if (getComputedSHA1() == null) {
                     // refuse to install if SHA-1 could not be computed
                     throw new IOException("Failed to compute SHA-1 of downloaded file, refusing installation");

@@ -520,7 +520,12 @@ public Entry(String sourceId, JSONObject o) {
             this.sourceId = sourceId;
             this.name = o.getString("name");
             this.version = o.getString("version");
-            this.sha1 = Util.fixEmpty(o.optString("sha1"));
+
+            String sha = Util.fixEmpty(o.optString("sha1"));
+            // 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 = (sha == null) ? null : sha.trim();
+
             String url = o.getString("url");
             if (!URI.create(url).isAbsolute()) {
                 if (baseURL == null) {
@@ -537,6 +542,8 @@ public Entry(String sourceId, JSONObject o) {
          * @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;
         }

@@ -1104,9 +1104,6 @@ public abstract class DownloadJob extends UpdateCenterJob {
          */
         protected abstract void onSuccess();
 
-
-        private Authentication authentication;
-
         /**
          * During download, an attempt is made to compute the SHA-1 checksum of the file.
          *
@@ -1115,7 +1112,13 @@ public abstract class DownloadJob extends UpdateCenterJob {
         // TODO no new API in LTS, but remove for mainline
         @Restricted(NoExternalUse.class)
         @CheckForNull
-        protected String computedSHA1;
+        protected String getComputedSHA1() {
+            return computedSHA1;
+        }
+
+        private String computedSHA1;
+
+        private Authentication authentication;
 
         /**
          * Get the user that initiated this job
@@ -1168,7 +1171,8 @@ protected void _run() throws IOException, InstallationStatus {
                     dis.close();
                 }
                 byte[] digest = sha1.digest();
-                computedSHA1 = Base64.encodeBase64String(digest);
+                // need to trim because commons-codec 1.4 used in test chunked output and adds \r\n at the end
+                computedSHA1 = Base64.encodeBase64String(digest).trim();
             } 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
@@ -1374,30 +1378,33 @@ public String toString() {
          */
         @Override
         protected void replace(File dst, File src) throws IOException {
-        	File bak = Util.changeExtension(dst,".bak");
-        	
-            bak.delete();
-            final File legacy = getLegacyDestination();
-			if(legacy.exists()){
-            	legacy.renameTo(bak);
-            }else{
-            	dst.renameTo(bak);
-            }
-            legacy.delete();
 
-            if (plugin.sha1 != null) {
-                if (computedSHA1 == null) {
+            if (plugin.getSha1() != null) {
+                if (getComputedSHA1() == 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 (!plugin.sha1.equals(computedSHA1)) {
-                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + plugin.sha1 + ", actual " + computedSHA1);
+                if (!plugin.getSha1().equals(getComputedSHA1())) {
+                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + plugin.getSha1() + ", actual " + getComputedSHA1());
                     // keep 'src' around for investigating what's going on
                 }
             }
 
-            dst.delete(); // any failure up to here is no big deal
-            
+            File bak = Util.changeExtension(dst, ".bak");
+            bak.delete();
+
+            final File legacy = getLegacyDestination();
+            if (legacy.exists()) {
+                if (!legacy.renameTo(bak)) {
+                    legacy.delete();
+                }
+            }
+            if (dst.exists()) {
+                if (!dst.renameTo(bak)) {
+                    dst.delete();
+                }
+            }
+
             if(!src.renameTo(dst)) {
                 throw new IOException("Failed to rename "+src+" to "+dst);
             }
@@ -1515,14 +1522,14 @@ protected void onSuccess() {
 
         @Override
         protected void replace(File dst, File src) throws IOException {
-            String expectedSHA1 = site.getData().core.sha1;
+            String expectedSHA1 = site.getData().core.getSha1();
             if (expectedSHA1 != null) {
-                if (computedSHA1 == null) {
+                if (getComputedSHA1() == 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(computedSHA1)) {
-                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + expectedSHA1 + ", actual " + computedSHA1);
+                if (!expectedSHA1.equals(getComputedSHA1())) {
+                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + expectedSHA1 + ", actual " + getComputedSHA1());
                     // keep 'src' around for investigating what's going on
                 }
             }

@@ -510,12 +510,7 @@ public static class Entry {
         @Exported
         public final String url;
 
-        /**
-         * The base64 encoded binary SHA-1 checksum of the file.
-         * @since TODO
-         */
-        // TODO @Exported assuming we want this in the API
-        public final String sha1;
+        private final String sha1;
 
         public Entry(String sourceId, JSONObject o) {
             this(sourceId, o, null);
@@ -536,6 +531,16 @@ public Entry(String sourceId, JSONObject o) {
             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
+        public String getSha1() {
+            return sha1;
+        }
+
         /**
          * Checks if the specified "current version" is older than the version of this entry.
          *

@@ -54,6 +54,7 @@
 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;
@@ -64,13 +65,17 @@
 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;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.UnknownHostException;
+import java.security.DigestInputStream;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -1102,6 +1107,16 @@ public abstract class DownloadJob extends UpdateCenterJob {
 
         private Authentication authentication;
 
+        /**
+         * 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 computedSHA1;
+
         /**
          * Get the user that initiated this job
          */
@@ -1142,6 +1157,24 @@ protected void _run() throws IOException, InstallationStatus {
             File dst = getDestination();
             File tmp = config.download(this, src);
 
+            try {
+                MessageDigest sha1 = MessageDigest.getInstance("SHA-1");
+                DigestInputStream dis = new DigestInputStream(new FileInputStream(tmp), sha1);
+                byte[] unused = new byte[1024];
+                try {
+                    while (dis.read(unused) != -1)
+                        ; // do nothing, just read the entire file
+                } finally {
+                    dis.close();
+                }
+                byte[] digest = sha1.digest();
+                computedSHA1 = Base64.encodeBase64String(digest);
+            } 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
+            }
+
             config.postValidate(this, tmp);
             config.install(this, tmp, dst);
         }
@@ -1351,6 +1384,18 @@ protected void replace(File dst, File src) throws IOException {
             	dst.renameTo(bak);
             }
             legacy.delete();
+
+            if (plugin.sha1 != null) {
+                if (computedSHA1 == 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 (!plugin.sha1.equals(computedSHA1)) {
+                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + plugin.sha1 + ", actual " + computedSHA1);
+                    // keep 'src' around for investigating what's going on
+                }
+            }
+
             dst.delete(); // any failure up to here is no big deal
             
             if(!src.renameTo(dst)) {
@@ -1470,6 +1515,17 @@ protected void onSuccess() {
 
         @Override
         protected void replace(File dst, File src) throws IOException {
+            String expectedSHA1 = site.getData().core.sha1;
+            if (expectedSHA1 != null) {
+                if (computedSHA1 == 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(computedSHA1)) {
+                    throw new IOException("Downloaded file " + src.getAbsolutePath() + " does not match expected SHA-1, expected " + expectedSHA1 + ", actual " + computedSHA1);
+                    // keep 'src' around for investigating what's going on
+                }
+            }
             Lifecycle.get().rewriteHudsonWar(src);
         }
     }

@@ -27,6 +27,7 @@
 
 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;
@@ -507,6 +510,13 @@ public static class Entry {
         @Exported
         public final String url;
 
+        /**
+         * The base64 encoded binary SHA-1 checksum of the file.
+         * @since TODO
+         */
+        // TODO @Exported assuming we want this in the API
+        public final String sha1;
+
         public Entry(String sourceId, JSONObject o) {
             this(sourceId, o, null);
         }
@@ -515,6 +525,7 @@ public Entry(String sourceId, JSONObject o) {
             this.sourceId = sourceId;
             this.name = o.getString("name");
             this.version = o.getString("version");
+            this.sha1 = Util.fixEmpty(o.optString("sha1"));
             String url = o.getString("url");
             if (!URI.create(url).isAbsolute()) {
                 if (baseURL == null) {