Skip to content

Commit

Permalink
[SECURITY-234] Getters instead of fields; trim base64 to fix test
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-beck committed Nov 27, 2015
1 parent 11479a2 commit 9ec8835
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 31 deletions.
57 changes: 32 additions & 25 deletions core/src/main/java/hudson/model/UpdateCenter.java
Expand Up @@ -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.
*
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
}
}
Expand Down
17 changes: 11 additions & 6 deletions core/src/main/java/hudson/model/UpdateSite.java
Expand Up @@ -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);
Expand All @@ -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.
*
Expand Down

0 comments on commit 9ec8835

Please sign in to comment.