From 0b0efbce92ffa93084237a07b6361b73efaca6c7 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Mon, 15 Oct 2018 10:47:42 -0300 Subject: [PATCH 1/3] Add checksum sanity validation on template registration --- .../template/HypervisorTemplateAdapter.java | 3 ++ .../utils/security/DigestHelper.java | 31 +++++++++++++++++++ .../utils/security/DigestHelperTest.java | 24 ++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/server/src/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/com/cloud/template/HypervisorTemplateAdapter.java index bfa73af6bcd3..126bee54ccda 100644 --- a/server/src/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/com/cloud/template/HypervisorTemplateAdapter.java @@ -39,6 +39,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint; import org.apache.cloudstack.storage.command.TemplateOrVolumePostUploadCommand; +import org.apache.cloudstack.utils.security.DigestHelper; import org.apache.log4j.Logger; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.api.command.user.iso.DeleteIsoCmd; @@ -155,6 +156,7 @@ public TemplateProfile prepare(RegisterIsoCmd cmd) throws ResourceAllocationExce String url = profile.getUrl(); UriUtils.validateUrl(ImageFormat.ISO.getFileExtension(), url); if (cmd.isDirectDownload()) { + DigestHelper.checksumSanity(cmd.getChecksum()); Long templateSize = performDirectDownloadUrlValidation(url); profile.setSize(templateSize); } @@ -170,6 +172,7 @@ public TemplateProfile prepare(RegisterTemplateCmd cmd) throws ResourceAllocatio String url = profile.getUrl(); UriUtils.validateUrl(cmd.getFormat(), url); if (cmd.isDirectDownload()) { + DigestHelper.checksumSanity(cmd.getChecksum()); Long templateSize = performDirectDownloadUrlValidation(url); profile.setSize(templateSize); } diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java index 67adf74189dd..3da6b583cd28 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java @@ -16,6 +16,8 @@ // under the License. package org.apache.cloudstack.utils.security; +import org.apache.commons.lang.StringUtils; + import java.io.IOException; import java.io.InputStream; import java.math.BigInteger; @@ -93,4 +95,33 @@ public static boolean isAlgorithmSupported(String checksum) { } return true; } + + /** + * Checks if the algorithm prefix is provided on the checksum + */ + public static void checksumSanity(String checksum) { + if(StringUtils.isNotEmpty(checksum)) { + if (checksum.contains("{") && checksum.contains("}")) { + int s = checksum.indexOf('{'); + int e = checksum.indexOf('}'); + if (s == 0 && e > s+1) { // we have an algorithm name of at least 1 char + String algorithm = checksum.substring(s + 1, e); + Map map = creatPaddingLengths(); + if (!map.containsKey(algorithm)) { + throw new IllegalArgumentException("Algorithm was provided but it is not one of the supported algorithms"); + } + Integer expectedLength = map.get(algorithm); + ChecksumValue checksumValue = new ChecksumValue(checksum); + String digest = checksumValue.getChecksum(); + if (digest.length() != expectedLength) { + throw new IllegalArgumentException("Checksum digest length should be " + expectedLength + " instead of " + digest.length()); + } + } else { + throw new IllegalArgumentException("Please add an algorithm between {}"); + } + } else { + throw new IllegalArgumentException("Algorithm prefix missing on checksum, please prepend {ALG} to the checksum"); + } + } + } } diff --git a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java index 454088273762..89dfe169e9a0 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java @@ -97,6 +97,30 @@ public static void init() throws UnsupportedEncodingException { public void reset() throws IOException { inputStream.reset(); } + + @Test(expected = IllegalArgumentException.class) + public void testChecksumSanityNoPrefix() { + String checksum = "177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; + DigestHelper.checksumSanity(checksum); + } + + @Test(expected = IllegalArgumentException.class) + public void testChecksumSanityPrefixEmptyAlgorithm() { + String checksum = "{}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; + DigestHelper.checksumSanity(checksum); + } + + @Test(expected = IllegalArgumentException.class) + public void testChecksumSanityPrefixWrongAlgorithm() { + String checksum = "{MD5}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; + DigestHelper.checksumSanity(checksum); + } + + @Test(expected = IllegalArgumentException.class) + public void testChecksumSanityPrefixWrongChecksumLength() { + String checksum = "{SHA-256}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816XXXXX"; + DigestHelper.checksumSanity(checksum); + } } //Generated with love by TestMe :) Please report issues and submit feature requests at: http://weirddev.com/forum#!/testme \ No newline at end of file From e36768d29ff5b573446f4a28dd01c213ed685158 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Mon, 15 Oct 2018 17:04:09 -0300 Subject: [PATCH 2/3] Refactor --- .../utils/security/DigestHelper.java | 38 ++++++++----------- .../utils/security/DigestHelperTest.java | 18 ++++++--- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java index 3da6b583cd28..32e7ad8ecc29 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java @@ -72,9 +72,9 @@ private static String getPaddedDigestString(MessageDigest digest, BigInteger pwI return checksum; } - static final Map paddingLengths = creatPaddingLengths(); + static final Map paddingLengths = getChecksumLengthsMap(); - private static final Map creatPaddingLengths() { + private static final Map getChecksumLengthsMap() { Map map = new HashMap<>(); map.put("MD5", 32); map.put("SHA-1", 40); @@ -97,30 +97,22 @@ public static boolean isAlgorithmSupported(String checksum) { } /** - * Checks if the algorithm prefix is provided on the checksum + * Checksum sanity for not empty checksum. Expected format: {ALG}HASH + * If ALG is missing, MD5 is assumed as default + * Hash length is verified, depending on the algorithm. + * IllegalArgumentException is thrown in case of malformed checksums */ public static void checksumSanity(String checksum) { if(StringUtils.isNotEmpty(checksum)) { - if (checksum.contains("{") && checksum.contains("}")) { - int s = checksum.indexOf('{'); - int e = checksum.indexOf('}'); - if (s == 0 && e > s+1) { // we have an algorithm name of at least 1 char - String algorithm = checksum.substring(s + 1, e); - Map map = creatPaddingLengths(); - if (!map.containsKey(algorithm)) { - throw new IllegalArgumentException("Algorithm was provided but it is not one of the supported algorithms"); - } - Integer expectedLength = map.get(algorithm); - ChecksumValue checksumValue = new ChecksumValue(checksum); - String digest = checksumValue.getChecksum(); - if (digest.length() != expectedLength) { - throw new IllegalArgumentException("Checksum digest length should be " + expectedLength + " instead of " + digest.length()); - } - } else { - throw new IllegalArgumentException("Please add an algorithm between {}"); - } - } else { - throw new IllegalArgumentException("Algorithm prefix missing on checksum, please prepend {ALG} to the checksum"); + ChecksumValue checksumValue = new ChecksumValue(checksum); + String digest = checksumValue.getChecksum(); + Map map = getChecksumLengthsMap(); + if (!map.containsKey(checksumValue.getAlgorithm())) { + throw new IllegalArgumentException("Algorithm " + checksumValue.getAlgorithm() + " was provided but it is not one of the supported algorithms"); + } + Integer expectedLength = map.get(checksumValue.getAlgorithm()); + if (digest.length() != expectedLength) { + throw new IllegalArgumentException("Checksum digest length should be " + expectedLength + " instead of " + digest.length()); } } } diff --git a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java index 89dfe169e9a0..849e8860d348 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java @@ -32,8 +32,10 @@ public class DigestHelperTest { private final static String INPUT_STRING_NO2 = "01234567890123456789012345678901234567890123456789012345678901234567890123456789b\n"; private final static String INPUT_STRING_NO3 = "01234567890123456789012345678901234567890123456789012345678901234567890123456789h\n"; private final static String SHA256_CHECKSUM = "{SHA-256}c6ab15af7842d23d3c06c138b53a7d09c5e351a79c4eb3c8ca8d65e5ce8900ab"; + private final static String SHA256_NO_PREFIX_CHECKSUM = "c6ab15af7842d23d3c06c138b53a7d09c5e351a79c4eb3c8ca8d65e5ce8900ab"; private final static String SHA1_CHECKSUM = "{SHA-1}49e4b2f4292b63e88597c127d11bc2cc0f2ca0ff"; private final static String MD5_CHECKSUM = "{MD5}d141a8eeaf6bba779d1d1dc5102a81c5"; + private final static String MD5_NO_PREFIX_CHECKSUM = "d141a8eeaf6bba779d1d1dc5102a81c5"; private final static String ZERO_PADDED_MD5_CHECKSUM = "{MD5}0e51dfa74b87f19dd5e0124d6a2195e3"; private final static String ZERO_PADDED_SHA256_CHECKSUM = "{SHA-256}08b5ae0c7d7d45d8ed406d7c3c7da695b81187903694314d97f8a37752a6b241"; private static final String MD5 = "MD5"; @@ -99,26 +101,30 @@ public void reset() throws IOException { } @Test(expected = IllegalArgumentException.class) + public void testChecksumSanityNoPrefixWrongAlgorithm() { + DigestHelper.checksumSanity(SHA256_NO_PREFIX_CHECKSUM); + } + + @Test public void testChecksumSanityNoPrefix() { - String checksum = "177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; - DigestHelper.checksumSanity(checksum); + DigestHelper.checksumSanity(MD5_NO_PREFIX_CHECKSUM); } - @Test(expected = IllegalArgumentException.class) + @Test public void testChecksumSanityPrefixEmptyAlgorithm() { - String checksum = "{}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; + String checksum = "{}" + MD5_NO_PREFIX_CHECKSUM; DigestHelper.checksumSanity(checksum); } @Test(expected = IllegalArgumentException.class) public void testChecksumSanityPrefixWrongAlgorithm() { - String checksum = "{MD5}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816"; + String checksum = "{MD5}" + SHA256_NO_PREFIX_CHECKSUM; DigestHelper.checksumSanity(checksum); } @Test(expected = IllegalArgumentException.class) public void testChecksumSanityPrefixWrongChecksumLength() { - String checksum = "{SHA-256}177adf7019cf04933ccb9f0cff784b5b737de2b5f92a43c60c362f646dfee816XXXXX"; + String checksum = SHA256_CHECKSUM + "XXXXX"; DigestHelper.checksumSanity(checksum); } } From 6e35adb153a89c1e2f8ef70e92b1f4af81fd08e2 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Mon, 15 Oct 2018 17:07:13 -0300 Subject: [PATCH 3/3] Rename checksum sanity method --- .../com/cloud/template/HypervisorTemplateAdapter.java | 4 ++-- .../apache/cloudstack/utils/security/DigestHelper.java | 2 +- .../cloudstack/utils/security/DigestHelperTest.java | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/src/com/cloud/template/HypervisorTemplateAdapter.java b/server/src/com/cloud/template/HypervisorTemplateAdapter.java index 126bee54ccda..8aa21661675f 100644 --- a/server/src/com/cloud/template/HypervisorTemplateAdapter.java +++ b/server/src/com/cloud/template/HypervisorTemplateAdapter.java @@ -156,7 +156,7 @@ public TemplateProfile prepare(RegisterIsoCmd cmd) throws ResourceAllocationExce String url = profile.getUrl(); UriUtils.validateUrl(ImageFormat.ISO.getFileExtension(), url); if (cmd.isDirectDownload()) { - DigestHelper.checksumSanity(cmd.getChecksum()); + DigestHelper.validateChecksumString(cmd.getChecksum()); Long templateSize = performDirectDownloadUrlValidation(url); profile.setSize(templateSize); } @@ -172,7 +172,7 @@ public TemplateProfile prepare(RegisterTemplateCmd cmd) throws ResourceAllocatio String url = profile.getUrl(); UriUtils.validateUrl(cmd.getFormat(), url); if (cmd.isDirectDownload()) { - DigestHelper.checksumSanity(cmd.getChecksum()); + DigestHelper.validateChecksumString(cmd.getChecksum()); Long templateSize = performDirectDownloadUrlValidation(url); profile.setSize(templateSize); } diff --git a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java index 32e7ad8ecc29..40b0c1c8defc 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java @@ -102,7 +102,7 @@ public static boolean isAlgorithmSupported(String checksum) { * Hash length is verified, depending on the algorithm. * IllegalArgumentException is thrown in case of malformed checksums */ - public static void checksumSanity(String checksum) { + public static void validateChecksumString(String checksum) { if(StringUtils.isNotEmpty(checksum)) { ChecksumValue checksumValue = new ChecksumValue(checksum); String digest = checksumValue.getChecksum(); diff --git a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java index 849e8860d348..4a6e3f7960a7 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/security/DigestHelperTest.java @@ -102,30 +102,30 @@ public void reset() throws IOException { @Test(expected = IllegalArgumentException.class) public void testChecksumSanityNoPrefixWrongAlgorithm() { - DigestHelper.checksumSanity(SHA256_NO_PREFIX_CHECKSUM); + DigestHelper.validateChecksumString(SHA256_NO_PREFIX_CHECKSUM); } @Test public void testChecksumSanityNoPrefix() { - DigestHelper.checksumSanity(MD5_NO_PREFIX_CHECKSUM); + DigestHelper.validateChecksumString(MD5_NO_PREFIX_CHECKSUM); } @Test public void testChecksumSanityPrefixEmptyAlgorithm() { String checksum = "{}" + MD5_NO_PREFIX_CHECKSUM; - DigestHelper.checksumSanity(checksum); + DigestHelper.validateChecksumString(checksum); } @Test(expected = IllegalArgumentException.class) public void testChecksumSanityPrefixWrongAlgorithm() { String checksum = "{MD5}" + SHA256_NO_PREFIX_CHECKSUM; - DigestHelper.checksumSanity(checksum); + DigestHelper.validateChecksumString(checksum); } @Test(expected = IllegalArgumentException.class) public void testChecksumSanityPrefixWrongChecksumLength() { String checksum = SHA256_CHECKSUM + "XXXXX"; - DigestHelper.checksumSanity(checksum); + DigestHelper.validateChecksumString(checksum); } }