Skip to content

Commit cf6f932

Browse files
jentfoojentfoo
authored
fix(sdk): Fuzz testing and protocol fixes (#214)
This change includes a variety of fixes found with fuzz testing. * Protocol Exception improvements - `NullPointerExceptions` have been converted to other exception types. Please provde feedback on if other exception types should be used for these cases. In `Fuzzing.java` now also serves to define in testing what exception types are acceptable. The `catch` specifically lists the types of exceptions that were discovered for each API call, and `throws` are checked exceptions that are not expected to be possible. As a future improvement we may want to refine this list further and better document what exceptions happen under what conditions. For now I thought it was best to start with just the `NullPointerException` cases. Since these cases were numerous, these changes span multiple commits, with each commit focused on a specific area of the protocol. * Protocol DoS Fixes - The only memory consumption issue discovered was the [counterpart found in the go sdk](opentdf/platform#1536). A matching fix with the same defaults was implemented here in the java sdk. * Finally the testing itself is added as `Fuzzing.java` executed through `sdk/fuzz.sh`. This script is long running, and there are occasional Jazzer failures which are not believed to be real deficiencies (timeouts when `.position()` is called on the stream). For that reason this testing needs to be done manually, and not expected to be included in CI * A few optimizations and clarity improvements were also included, as they were noticed while generally trying to get familiar with the codebase.
1 parent cb6f757 commit cf6f932

25 files changed

+341
-121
lines changed

‎sdk/fuzz.sh‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
#!/bin/bash
2+
set -e
3+
4+
tests=("fuzzNanoTDF", "fuzzTDF", "fuzzZipRead")
5+
base_seed_dir="src/test/resources/io/opentdf/platform/sdk/FuzzingInputs/"
6+
7+
for test in "${tests[@]}"; do
8+
seed_dir="${base_seed_dir}${test}"
9+
echo "Running $test fuzzing with seeds from $seed_dir"
10+
mvn verify -P fuzz -Djazzer.testDir=$seed_dir
11+
done

‎sdk/pom.xml‎

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99
<version>0.7.5</version>
1010
</parent>
1111
<packaging>jar</packaging>
12+
<properties>
13+
<jazzer.version>0.22.1</jazzer.version>
14+
<jazzer.baseurl>https://github.com/CodeIntelligenceTesting/jazzer/releases/download/v${jazzer.version}</jazzer.baseurl>
15+
</properties>
1216
<dependencies>
1317
<!-- Logging Dependencies -->
1418
<dependency>
@@ -121,6 +125,18 @@
121125
<version>4.13.2</version>
122126
<scope>test</scope>
123127
</dependency>
128+
<dependency>
129+
<groupId>com.code-intelligence</groupId>
130+
<artifactId>jazzer-api</artifactId>
131+
<version>${jazzer.version}</version>
132+
<scope>test</scope>
133+
</dependency>
134+
<dependency>
135+
<groupId>com.code-intelligence</groupId>
136+
<artifactId>jazzer-junit</artifactId>
137+
<version>${jazzer.version}</version>
138+
<scope>test</scope>
139+
</dependency>
124140
<dependency>
125141
<groupId>org.apache.commons</groupId>
126142
<artifactId>commons-compress</artifactId>
@@ -307,4 +323,110 @@
307323
</plugin>
308324
</plugins>
309325
</build>
310-
</project>
326+
<!--profile to execute fuzz test -->
327+
<profiles>
328+
<profile>
329+
<id>fuzz</id>
330+
<activation>
331+
<activeByDefault>false</activeByDefault>
332+
</activation>
333+
<properties>
334+
<skipTests>true</skipTests>
335+
</properties>
336+
<build>
337+
<plugins>
338+
<!-- Plugin to download and unpack the Jazzer agent -->
339+
<plugin>
340+
<groupId>org.apache.maven.plugins</groupId>
341+
<artifactId>maven-antrun-plugin</artifactId>
342+
<version>3.1.0</version>
343+
<executions>
344+
<execution>
345+
<id>download-and-unpack-jazzer</id>
346+
<phase>process-test-classes</phase>
347+
<configuration>
348+
<target>
349+
<condition property="jazzer.os" value="windows">
350+
<os family="windows"/>
351+
</condition>
352+
<condition property="jazzer.os" value="macos">
353+
<os family="mac"/>
354+
</condition>
355+
<condition property="jazzer.os" value="linux">
356+
<os family="unix"/>
357+
</condition>
358+
<echo message="Detected OS: ${jazzer.os}"/>
359+
<mkdir dir="${project.build.directory}/jazzer"/>
360+
<get src="${jazzer.baseurl}/jazzer-${jazzer.os}.tar.gz" dest="${project.build.directory}/jazzer/jazzer.tar.gz"/>
361+
<untar compression="gzip" src="${project.build.directory}/jazzer/jazzer.tar.gz" dest="${project.build.directory}/jazzer"/>
362+
</target>
363+
</configuration>
364+
<goals>
365+
<goal>run</goal>
366+
</goals>
367+
</execution>
368+
</executions>
369+
</plugin>
370+
371+
<!-- Copy dependencies to a directory for Jazzer to reference -->
372+
<plugin>
373+
<groupId>org.apache.maven.plugins</groupId>
374+
<artifactId>maven-dependency-plugin</artifactId>
375+
<version>3.4.0</version>
376+
<executions>
377+
<execution>
378+
<id>copy-dependencies</id>
379+
<phase>process-test-classes</phase>
380+
<goals>
381+
<goal>copy-dependencies</goal>
382+
</goals>
383+
<configuration>
384+
<outputDirectory>${project.build.directory}/dependency-jars</outputDirectory>
385+
<includeScope>test</includeScope>
386+
</configuration>
387+
</execution>
388+
</executions>
389+
</plugin>
390+
391+
<!-- Plugin to execute Jazzer -->
392+
<plugin>
393+
<groupId>org.apache.maven.plugins</groupId>
394+
<artifactId>maven-antrun-plugin</artifactId>
395+
<version>3.1.0</version>
396+
<executions>
397+
<execution>
398+
<id>run-jazzer-fuzzing</id>
399+
<phase>verify</phase>
400+
<configuration>
401+
<target>
402+
<path id="project.classpath">
403+
<pathelement location="${project.build.directory}/classes"/>
404+
<pathelement location="${project.build.directory}/test-classes"/>
405+
<fileset dir="${project.build.directory}/dependency-jars">
406+
<include name="**/*.jar"/>
407+
</fileset>
408+
</path>
409+
<pathconvert property="project.classpath.string" pathsep="${path.separator}">
410+
<path refid="project.classpath"/>
411+
</pathconvert>
412+
<property environment="env"/>
413+
414+
<chmod file="${project.build.directory}/jazzer/jazzer" perm="777"/>
415+
416+
<exec executable="bash">
417+
<arg value="-c"/>
418+
<arg value="if [ -z &quot;${JAVA_HOME}&quot; ]; then JAVA_HOME=$(dirname $(dirname $(which java))); fi; DYLD_LIBRARY_PATH=$(find &quot;${JAVA_HOME}&quot; -type d | grep 'libexec/openjdk.jdk/Contents/Home/lib/server' 2&gt;/dev/null | head -n 1); if [ -z &quot;${DYLD_LIBRARY_PATH}&quot; ]; then DYLD_LIBRARY_PATH=&quot;${JAVA_HOME}/lib/server&quot;; fi; export DYLD_LIBRARY_PATH=${DYLD_LIBRARY_PATH}; ${project.build.directory}/jazzer/jazzer --cp='${project.classpath.string}' --target_class='io.opentdf.platform.sdk.Fuzzing' --instrumentation_includes='io.opentdf.platform.sdk.**' ${jazzer.testDir}"/>
419+
</exec>
420+
</target>
421+
</configuration>
422+
<goals>
423+
<goal>run</goal>
424+
</goals>
425+
</execution>
426+
</executions>
427+
</plugin>
428+
</plugins>
429+
</build>
430+
</profile>
431+
</profiles>
432+
</project>

‎sdk/src/main/java/io/opentdf/platform/sdk/Config.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ public class Config {
2121

2222
public static final int TDF3_KEY_SIZE = 2048;
2323
public static final int DEFAULT_SEGMENT_SIZE = 2 * 1024 * 1024; // 2mb
24+
public static final int MAX_SEGMENT_SIZE = DEFAULT_SEGMENT_SIZE * 2;
25+
public static final int MIN_SEGMENT_SIZE = 16 * 1024; // not currently enforced in parsing due to existing payloads in testing
2426
public static final String KAS_PUBLIC_KEY_PATH = "/kas_public_key";
2527
public static final String DEFAULT_MIME_TYPE = "application/octet-stream";
2628
public static final int MAX_COLLECTION_ITERATION = (1 << 24) - 1;
@@ -228,6 +230,12 @@ public static Consumer<TDFConfig> withMetaData(String metaData) {
228230
}
229231

230232
public static Consumer<TDFConfig> withSegmentSize(int size) {
233+
if (size > MAX_SEGMENT_SIZE) {
234+
throw new IllegalArgumentException("Segment size " + size + " exceeds the maximum " + MAX_SEGMENT_SIZE);
235+
} else if (size < MIN_SEGMENT_SIZE) {
236+
throw new IllegalArgumentException("Segment size " + size + " is under the minimum " + MIN_SEGMENT_SIZE);
237+
}
238+
231239
return (TDFConfig config) -> config.defaultSegmentSize = size;
232240
}
233241

‎sdk/src/main/java/io/opentdf/platform/sdk/Manifest.java‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ static public class Payload {
260260
public String url;
261261
public String protocol;
262262
public String mimeType;
263-
public Boolean isEncrypted;
263+
public boolean isEncrypted;
264264

265265
@Override
266266
public boolean equals(Object o) {
@@ -473,8 +473,41 @@ public Manifest deserialize(JsonElement json, Type typeOfT, JsonDeserializationC
473473
}
474474
}
475475

476-
private static Manifest readManifest(Reader reader) {
477-
return gson.fromJson(reader, Manifest.class);
476+
protected static Manifest readManifest(Reader reader) {
477+
Manifest result = gson.fromJson(reader, Manifest.class);
478+
if (result == null) {
479+
throw new IllegalArgumentException("Manifest is null");
480+
} else if (result.payload == null) {
481+
throw new IllegalArgumentException("Manifest with null payload");
482+
} else if (result.encryptionInformation == null) {
483+
throw new IllegalArgumentException("Manifest with null encryptionInformation");
484+
} else if (result.encryptionInformation.integrityInformation == null) {
485+
throw new IllegalArgumentException("Manifest with null integrityInformation");
486+
} else if (result.encryptionInformation.integrityInformation.rootSignature == null) {
487+
throw new IllegalArgumentException("Manifest with null rootSignature");
488+
} else if (result.encryptionInformation.integrityInformation.rootSignature.algorithm == null
489+
|| result.encryptionInformation.integrityInformation.rootSignature.signature == null) {
490+
throw new IllegalArgumentException("Manifest with invalid rootSignature");
491+
} else if (result.encryptionInformation.integrityInformation.segments == null) {
492+
throw new IllegalArgumentException("Manifest with null segments");
493+
} else if (result.encryptionInformation.keyAccessObj == null) {
494+
throw new IllegalArgumentException("Manifest with null keyAccessObj");
495+
} else if (result.encryptionInformation.policy == null) {
496+
throw new IllegalArgumentException("Manifest with null policy");
497+
}
498+
499+
for (Manifest.Segment segment : result.encryptionInformation.integrityInformation.segments) {
500+
if (segment == null || segment.hash == null) {
501+
throw new IllegalArgumentException("Invalid integrity segment");
502+
}
503+
}
504+
for (Manifest.KeyAccess keyAccess : result.encryptionInformation.keyAccessObj) {
505+
if (keyAccess == null) {
506+
throw new IllegalArgumentException("Invalid null KeyAccess in manifest");
507+
}
508+
}
509+
510+
return result;
478511
}
479512

480513
static PolicyObject readPolicyObject(Reader reader) {

‎sdk/src/main/java/io/opentdf/platform/sdk/NanoTDF.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ PolicyObject createPolicyObject(List<String> attributes) {
252252
PolicyObject policyObject = new PolicyObject();
253253
policyObject.body = new PolicyObject.Body();
254254
policyObject.uuid = UUID.randomUUID().toString();
255-
policyObject.body.dataAttributes = new ArrayList<>();
255+
policyObject.body.dataAttributes = new ArrayList<>(attributes.size());
256256
policyObject.body.dissem = new ArrayList<>();
257257

258258
for (String attribute : attributes) {

‎sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public SDKBuilder sslFactoryFromDirectory(String certsDirPath) throws Exception
7474
File certsDir = new File(certsDirPath);
7575
File[] certFiles = certsDir.listFiles((dir, name) -> name.endsWith(".pem") || name.endsWith(".crt"));
7676
logger.info("Loading certificates from: " + certsDir.getAbsolutePath());
77-
List<InputStream> certStreams = new ArrayList<>();
77+
List<InputStream> certStreams = new ArrayList<>(certFiles.length);
7878
for (File certFile : certFiles) {
7979
certStreams.add(new FileInputStream(certFile));
8080
}

‎sdk/src/main/java/io/opentdf/platform/sdk/TDF.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.io.InputStream;
2424
import java.io.InputStreamReader;
2525
import java.io.OutputStream;
26+
import java.io.StringReader;
2627
import java.nio.channels.SeekableByteChannel;
2728
import java.nio.charset.StandardCharsets;
2829
import java.security.*;
@@ -188,7 +189,7 @@ PolicyObject createPolicyObject(List<Autoconfigure.AttributeValueFQN> attributes
188189
PolicyObject policyObject = new PolicyObject();
189190
policyObject.body = new PolicyObject.Body();
190191
policyObject.uuid = UUID.randomUUID().toString();
191-
policyObject.body.dataAttributes = new ArrayList<>();
192+
policyObject.body.dataAttributes = new ArrayList<>(attributes.size());
192193
policyObject.body.dissem = new ArrayList<>();
193194

194195
for (Autoconfigure.AttributeValueFQN attribute : attributes) {
@@ -208,7 +209,6 @@ private void prepareManifest(Config.TDFConfig tdfConfig, SDK.KAS kas) {
208209
PolicyObject policyObject = createPolicyObject(tdfConfig.attributes);
209210
String base64PolicyObject = encoder
210211
.encodeToString(gson.toJson(policyObject).getBytes(StandardCharsets.UTF_8));
211-
List<byte[]> symKeys = new ArrayList<>();
212212
Map<String, Config.KASInfo> latestKASInfo = new HashMap<>();
213213
if (tdfConfig.splitPlan == null || tdfConfig.splitPlan.isEmpty()) {
214214
// Default split plan: Split keys across all KASes
@@ -261,6 +261,7 @@ private void prepareManifest(Config.TDFConfig tdfConfig, SDK.KAS kas) {
261261
}
262262
}
263263

264+
List<byte[]> symKeys = new ArrayList<>(splitIDs.size());
264265
for (String splitID : splitIDs) {
265266
// Symmetric key
266267
byte[] symKey = new byte[GCM_KEY_SIZE];
@@ -358,6 +359,10 @@ public void readPayload(OutputStream outputStream) throws TDFReadFailed,
358359
MessageDigest digest = MessageDigest.getInstance("SHA-256");
359360

360361
for (Manifest.Segment segment : manifest.encryptionInformation.integrityInformation.segments) {
362+
if (segment.encryptedSegmentSize > Config.MAX_SEGMENT_SIZE) {
363+
throw new IllegalStateException("Segment size " + segment.encryptedSegmentSize + " exceeded limit " + Config.MAX_SEGMENT_SIZE);
364+
} // MIN_SEGMENT_SIZE NOT validated out due to tests needing small segment sizes with existing payloads
365+
361366
byte[] readBuf = new byte[(int) segment.encryptedSegmentSize];
362367
int bytesRead = tdfReader.readPayloadBytes(readBuf);
363368

@@ -520,8 +525,7 @@ public TDFObject createTDF(InputStream payload,
520525
tdfObject.manifest.payload.url = TDFWriter.TDF_PAYLOAD_FILE_NAME;
521526
tdfObject.manifest.payload.isEncrypted = true;
522527

523-
List<Manifest.Assertion> signedAssertions = new ArrayList<>();
524-
528+
List<Manifest.Assertion> signedAssertions = new ArrayList<>(tdfConfig.assertionConfigList.size());
525529
for (var assertionConfig : tdfConfig.assertionConfigList) {
526530
var assertion = new Manifest.Assertion();
527531
assertion.id = assertionConfig.id;
@@ -585,7 +589,8 @@ public Reader loadTDF(SeekableByteChannel tdf, SDK.KAS kas,
585589

586590
TDFReader tdfReader = new TDFReader(tdf);
587591
String manifestJson = tdfReader.manifest();
588-
Manifest manifest = gson.fromJson(manifestJson, Manifest.class);
592+
// use Manifest.readManifest in order to validate the Manifest input
593+
Manifest manifest = Manifest.readManifest(new StringReader(manifestJson));
589594
byte[] payloadKey = new byte[GCM_KEY_SIZE];
590595
String unencryptedMetadata = null;
591596

0 commit comments

Comments
 (0)