Skip to content

Commit 3d5e272

Browse files
millemsmillems
authored
Fixed an issue where checksum validation only considered the first 4 bytes of the 16 byte checksum, creating the potential for corrupted downloads to go undetected. (#2646)
1 parent e416078 commit 3d5e272

File tree

6 files changed

+152
-42
lines changed

6 files changed

+152
-42
lines changed

‎.changes/next-release/bugfix-AmazonS3-421839e.json

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"category": "Amazon S3",
3+
"contributor": "",
4+
"type": "bugfix",
5+
"description": "Fixed an issue where checksum validation only considered the first 4 bytes of the 16 byte checksum, creating the potential for corrupted downloads to go undetected."
6+
}

‎pom.xml

+2-3
Original file line numberDiff line numberDiff line change
@@ -511,9 +511,8 @@
511511
<excludes>
512512
<exclude>*.internal.*</exclude>
513513

514-
<!-- Jackson removal -->
515-
<exclude>software.amazon.awssdk.core.util.json.JacksonUtils</exclude>
516-
<exclude>software.amazon.awssdk.protocols.json.*</exclude>
514+
<!-- Checksum bug fix -->
515+
<exclude>software.amazon.awssdk.services.s3.checksums.ChecksumValidatingInputStream</exclude>
517516
</excludes>
518517

519518
<excludeModules>

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingInputStream.java

+7-17
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,12 @@
1717

1818
import java.io.IOException;
1919
import java.io.InputStream;
20-
import java.nio.ByteBuffer;
20+
import java.util.Arrays;
2121
import software.amazon.awssdk.annotations.SdkInternalApi;
2222
import software.amazon.awssdk.core.checksums.SdkChecksum;
2323
import software.amazon.awssdk.core.exception.SdkClientException;
2424
import software.amazon.awssdk.http.Abortable;
25+
import software.amazon.awssdk.utils.BinaryUtils;
2526

2627
@SdkInternalApi
2728
public class ChecksumValidatingInputStream extends InputStream implements Abortable {
@@ -34,7 +35,7 @@ public class ChecksumValidatingInputStream extends InputStream implements Aborta
3435
private long lengthRead = 0;
3536
// Preserve the computed checksum because some InputStream readers (e.g., java.util.Properties) read more than once at the
3637
// end of the stream.
37-
private Integer computedChecksum;
38+
private byte[] computedChecksum;
3839

3940
/**
4041
* Creates an input stream using the specified Checksum, input stream, and length.
@@ -162,26 +163,15 @@ public void close() throws IOException {
162163
inputStream.close();
163164
}
164165

165-
/**
166-
* Gets the stream's checksum as an integer.
167-
*
168-
* @return checksum.
169-
*/
170-
public int getStreamChecksum() {
171-
ByteBuffer bb = ByteBuffer.wrap(streamChecksum);
172-
return bb.getInt();
173-
}
174-
175166
private void validateAndThrow() {
176-
int streamChecksumInt = getStreamChecksum();
177167
if (computedChecksum == null) {
178-
computedChecksum = ByteBuffer.wrap(checkSum.getChecksumBytes()).getInt();
168+
computedChecksum = checkSum.getChecksumBytes();
179169
}
180170

181-
if (streamChecksumInt != computedChecksum) {
171+
if (!Arrays.equals(computedChecksum, streamChecksum)) {
182172
throw SdkClientException.builder().message(
183-
String.format("Data read has a different checksum than expected. Was %d, but expected %d",
184-
computedChecksum, streamChecksumInt)).build();
173+
String.format("Data read has a different checksum than expected. Was 0x%s, but expected 0x%s",
174+
BinaryUtils.toHex(computedChecksum), BinaryUtils.toHex(streamChecksum))).build();
185175
}
186176
}
187177

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingPublisher.java

+4-5
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,11 @@ public void onError(Throwable t) {
132132
@Override
133133
public void onComplete() {
134134
if (strippedLength > 0) {
135-
int streamChecksumInt = ByteBuffer.wrap(streamChecksum).getInt();
136-
int computedChecksumInt = ByteBuffer.wrap(sdkChecksum.getChecksumBytes()).getInt();
137-
if (streamChecksumInt != computedChecksumInt) {
135+
byte[] computedChecksum = sdkChecksum.getChecksumBytes();
136+
if (!Arrays.equals(computedChecksum, streamChecksum)) {
138137
onError(SdkClientException.create(
139-
String.format("Data read has a different checksum than expected. Was %d, but expected %d",
140-
computedChecksumInt, streamChecksumInt)));
138+
String.format("Data read has a different checksum than expected. Was 0x%s, but expected 0x%s",
139+
BinaryUtils.toHex(computedChecksum), BinaryUtils.toHex(streamChecksum))));
141140
return; // Return after onError and not call onComplete below
142141
}
143142
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingInputStreamTest.java

+87
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.services.s3.checksums;
17+
18+
import static org.junit.Assert.assertArrayEquals;
19+
20+
import java.io.ByteArrayInputStream;
21+
import java.io.IOException;
22+
import java.io.InputStream;
23+
import java.util.Arrays;
24+
import org.junit.Assert;
25+
import org.junit.BeforeClass;
26+
import org.junit.Test;
27+
import software.amazon.awssdk.core.checksums.Md5Checksum;
28+
import software.amazon.awssdk.core.exception.SdkClientException;
29+
import software.amazon.awssdk.utils.IoUtils;
30+
31+
public class ChecksumValidatingInputStreamTest {
32+
private static final int TEST_DATA_SIZE = 32;
33+
private static final int CHECKSUM_SIZE = 16;
34+
35+
private static byte[] testData;
36+
private static byte[] testDataWithoutChecksum;
37+
38+
@BeforeClass
39+
public static void populateData() {
40+
testData = new byte[TEST_DATA_SIZE + CHECKSUM_SIZE];
41+
for (int i = 0; i < TEST_DATA_SIZE; i++) {
42+
testData[i] = (byte)(i & 0x7f);
43+
}
44+
45+
Md5Checksum checksum = new Md5Checksum();
46+
checksum.update(testData, 0, TEST_DATA_SIZE);
47+
byte[] checksumBytes = checksum.getChecksumBytes();
48+
49+
for (int i = 0; i < CHECKSUM_SIZE; i++) {
50+
testData[TEST_DATA_SIZE + i] = checksumBytes[i];
51+
}
52+
53+
testDataWithoutChecksum = Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE);
54+
}
55+
56+
@Test
57+
public void validChecksumSucceeds() throws IOException {
58+
InputStream validatingInputStream = newValidatingStream(testData);
59+
byte[] dataFromValidatingStream = IoUtils.toByteArray(validatingInputStream);
60+
61+
assertArrayEquals(testDataWithoutChecksum, dataFromValidatingStream);
62+
}
63+
64+
@Test
65+
public void invalidChecksumFails() throws IOException {
66+
for (int i = 0; i < testData.length; i++) {
67+
// Make sure that corruption of any byte in the test data causes a checksum validation failure.
68+
byte[] corruptedChecksumData = Arrays.copyOf(testData, testData.length);
69+
corruptedChecksumData[i] = (byte) ~corruptedChecksumData[i];
70+
71+
InputStream validatingInputStream = newValidatingStream(corruptedChecksumData);
72+
73+
try {
74+
IoUtils.toByteArray(validatingInputStream);
75+
Assert.fail("Corruption at byte " + i + " was not detected.");
76+
} catch (SdkClientException e) {
77+
// Expected
78+
}
79+
}
80+
}
81+
82+
private InputStream newValidatingStream(byte[] dataFromS3) {
83+
return new ChecksumValidatingInputStream(new ByteArrayInputStream(dataFromS3),
84+
new Md5Checksum(),
85+
TEST_DATA_SIZE + CHECKSUM_SIZE);
86+
}
87+
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/checksums/ChecksumValidatingPublisherTest.java

+46-17
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,13 @@
1616
package software.amazon.awssdk.services.s3.checksums;
1717

1818
import static org.junit.Assert.assertArrayEquals;
19-
import static org.junit.Assert.assertEquals;
2019
import static org.junit.Assert.assertFalse;
2120
import static org.junit.Assert.assertTrue;
2221
import static org.junit.Assert.fail;
2322

23+
import java.io.ByteArrayOutputStream;
24+
import java.io.IOException;
25+
import java.io.UncheckedIOException;
2426
import java.nio.ByteBuffer;
2527
import java.util.ArrayList;
2628
import java.util.Arrays;
@@ -31,6 +33,7 @@
3133
import org.reactivestreams.Subscriber;
3234
import org.reactivestreams.Subscription;
3335
import software.amazon.awssdk.core.checksums.Md5Checksum;
36+
import software.amazon.awssdk.utils.BinaryUtils;
3437

3538
/**
3639
* Unit test for ChecksumValidatingPublisher
@@ -39,6 +42,7 @@ public class ChecksumValidatingPublisherTest {
3942
private static int TEST_DATA_SIZE = 32; // size of the test data, in bytes
4043
private static final int CHECKSUM_SIZE = 16;
4144
private static byte[] testData;
45+
private static byte[] testDataWithoutChecksum;
4246

4347
@BeforeClass
4448
public static void populateData() {
@@ -52,34 +56,55 @@ public static void populateData() {
5256
for (int i = 0; i < CHECKSUM_SIZE; i++) {
5357
testData[TEST_DATA_SIZE + i] = checksumBytes[i];
5458
}
59+
60+
testDataWithoutChecksum = Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE);
5561
}
5662

5763
@Test
5864
public void testSinglePacket() {
5965
final TestPublisher driver = new TestPublisher();
60-
final TestSubscriber s = new TestSubscriber(Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE));
66+
final TestSubscriber s = new TestSubscriber();
6167
final ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), TEST_DATA_SIZE + CHECKSUM_SIZE);
6268
p.subscribe(s);
6369

6470
driver.doOnNext(ByteBuffer.wrap(testData));
6571
driver.doOnComplete();
6672

73+
assertArrayEquals(testDataWithoutChecksum, s.receivedData());
6774
assertTrue(s.hasCompleted());
6875
assertFalse(s.isOnErrorCalled());
6976
}
7077

78+
@Test
79+
public void testLastChecksumByteCorrupted() {
80+
TestPublisher driver = new TestPublisher();
81+
82+
TestSubscriber s = new TestSubscriber();
83+
ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), TEST_DATA_SIZE + CHECKSUM_SIZE);
84+
p.subscribe(s);
85+
86+
byte[] incorrectChecksumData = Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE);
87+
incorrectChecksumData[TEST_DATA_SIZE - 1] = (byte) ~incorrectChecksumData[TEST_DATA_SIZE - 1];
88+
driver.doOnNext(ByteBuffer.wrap(incorrectChecksumData));
89+
driver.doOnComplete();
90+
91+
assertFalse(s.hasCompleted());
92+
assertTrue(s.isOnErrorCalled());
93+
}
94+
7195
@Test
7296
public void testTwoPackets() {
7397
for (int i = 1; i < TEST_DATA_SIZE + CHECKSUM_SIZE - 1; i++) {
7498
final TestPublisher driver = new TestPublisher();
75-
final TestSubscriber s = new TestSubscriber(Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE));
99+
final TestSubscriber s = new TestSubscriber();
76100
final ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), TEST_DATA_SIZE + CHECKSUM_SIZE);
77101
p.subscribe(s);
78102

79103
driver.doOnNext(ByteBuffer.wrap(testData, 0, i));
80104
driver.doOnNext(ByteBuffer.wrap(testData, i, TEST_DATA_SIZE + CHECKSUM_SIZE - i));
81105
driver.doOnComplete();
82106

107+
assertArrayEquals(testDataWithoutChecksum, s.receivedData());
83108
assertTrue(s.hasCompleted());
84109
assertFalse(s.isOnErrorCalled());
85110
}
@@ -89,7 +114,7 @@ public void testTwoPackets() {
89114
public void testTinyPackets() {
90115
for (int packetSize = 1; packetSize < CHECKSUM_SIZE; packetSize++) {
91116
final TestPublisher driver = new TestPublisher();
92-
final TestSubscriber s = new TestSubscriber(Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE));
117+
final TestSubscriber s = new TestSubscriber();
93118
final ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), TEST_DATA_SIZE + CHECKSUM_SIZE);
94119
p.subscribe(s);
95120
int currOffset = 0;
@@ -100,6 +125,7 @@ public void testTinyPackets() {
100125
}
101126
driver.doOnComplete();
102127

128+
assertArrayEquals(testDataWithoutChecksum, s.receivedData());
103129
assertTrue(s.hasCompleted());
104130
assertFalse(s.isOnErrorCalled());
105131
}
@@ -109,7 +135,7 @@ public void testTinyPackets() {
109135
public void testUnknownLength() {
110136
// When the length is unknown, the last 16 bytes are treated as a checksum, but are later ignored when completing
111137
final TestPublisher driver = new TestPublisher();
112-
final TestSubscriber s = new TestSubscriber(Arrays.copyOfRange(testData, 0, TEST_DATA_SIZE));
138+
final TestSubscriber s = new TestSubscriber();
113139
final ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), 0);
114140
p.subscribe(s);
115141

@@ -122,6 +148,7 @@ public void testUnknownLength() {
122148
driver.doOnNext(ByteBuffer.wrap(randomChecksumData));
123149
driver.doOnComplete();
124150

151+
assertArrayEquals(testDataWithoutChecksum, s.receivedData());
125152
assertTrue(s.hasCompleted());
126153
assertFalse(s.isOnErrorCalled());
127154
}
@@ -130,7 +157,7 @@ public void testUnknownLength() {
130157
public void checksumValidationFailure_throwsSdkClientException_NotNPE() {
131158
final byte[] incorrectData = new byte[0];
132159
final TestPublisher driver = new TestPublisher();
133-
final TestSubscriber s = new TestSubscriber(Arrays.copyOfRange(incorrectData, 0, TEST_DATA_SIZE));
160+
final TestSubscriber s = new TestSubscriber();
134161
final ChecksumValidatingPublisher p = new ChecksumValidatingPublisher(driver, new Md5Checksum(), TEST_DATA_SIZE + CHECKSUM_SIZE);
135162
p.subscribe(s);
136163

@@ -142,13 +169,11 @@ public void checksumValidationFailure_throwsSdkClientException_NotNPE() {
142169
}
143170

144171
private class TestSubscriber implements Subscriber<ByteBuffer> {
145-
final byte[] expected;
146172
final List<ByteBuffer> received;
147173
boolean completed;
148174
boolean onErrorCalled;
149175

150-
TestSubscriber(byte[] expected) {
151-
this.expected = expected;
176+
TestSubscriber() {
152177
this.received = new ArrayList<>();
153178
this.completed = false;
154179
}
@@ -172,17 +197,21 @@ public void onError(Throwable t) {
172197

173198
@Override
174199
public void onComplete() {
175-
int matchPos = 0;
176-
for (ByteBuffer buffer : received) {
177-
byte[] bufferData = new byte[buffer.limit() - buffer.position()];
178-
buffer.get(bufferData);
179-
assertArrayEquals(Arrays.copyOfRange(expected, matchPos, matchPos + bufferData.length), bufferData);
180-
matchPos += bufferData.length;
181-
}
182-
assertEquals(expected.length, matchPos);
183200
completed = true;
184201
}
185202

203+
public byte[] receivedData() {
204+
try {
205+
ByteArrayOutputStream os = new ByteArrayOutputStream();
206+
for (ByteBuffer buffer : received) {
207+
os.write(BinaryUtils.copyBytesFrom(buffer));
208+
}
209+
return os.toByteArray();
210+
} catch (IOException e) {
211+
throw new UncheckedIOException(e);
212+
}
213+
}
214+
186215
public boolean hasCompleted() {
187216
return completed;
188217
}

0 commit comments

Comments
 (0)