diff --git a/oss-licenses-plugin/src/main/groovy/com/google/android/gms/oss/licenses/plugin/LicensesTask.groovy b/oss-licenses-plugin/src/main/groovy/com/google/android/gms/oss/licenses/plugin/LicensesTask.groovy index 39dd26ef..0c33c350 100644 --- a/oss-licenses-plugin/src/main/groovy/com/google/android/gms/oss/licenses/plugin/LicensesTask.groovy +++ b/oss-licenses-plugin/src/main/groovy/com/google/android/gms/oss/licenses/plugin/LicensesTask.groovy @@ -216,6 +216,11 @@ abstract class LicensesTask extends DefaultTask { * license texts at the specified offsets from the corresponding license text file, * and registers them with the task's aggregated license tracker. * + * A record whose key is malformed is logged and skipped rather than aborting the build. The + * caller, {@link #addEmbeddedLicenses(File)}, deliberately tolerates unreadable third-party + * artifacts, and an unchecked exception thrown from here would escape that handler and fail a + * consumer's build over a single bad entry. + * * @param licensesZip the ZipFile representation of the dependency archive * @param jsonFile the ZipEntry for the third-party license JSON metadata file * @param txtFile the ZipEntry for the third-party license text file (.txt) @@ -234,47 +239,79 @@ abstract class LicensesTask extends DefaultTask { int startValue = entry.value.start int lengthValue = entry.value.length - if (!embeddedLicenses.contains(key)) { + // A malformed key must not fail the build: addEmbeddedLicenses() deliberately + // tolerates unreadable third-party artifacts, so skip the record and keep going. + Dependency dependency + try { + dependency = new Dependency(key, key) + } catch (IllegalArgumentException | NullPointerException e) { + logger.warn("Skipping malformed license key in ${jsonFile.name}: ${e.message}") + continue + } + + if (!embeddedLicenses.contains(dependency.key)) { licensesZip.getInputStream(txtFile).withCloseable { byte[] content = getBytesFromInputStream( it, startValue, lengthValue) - embeddedLicenses.add(key) - appendDependency(key, content) + embeddedLicenses.add(dependency.key) + appendDependency(dependency, content) } } } } + /** + * Reads the license text occupying {@code length} bytes at {@code offset}, always closing + * {@code stream} before returning. + * + * Why is a {@code length} of zero read to the end of the stream rather than treated as an + * empty range? Groovy coerces an absent JSON {@code length} field to {@code 0}, so zero means + * "the artifact did not say", not "no bytes". Returning an empty array would silently drop the + * attribution text for every artifact with incomplete metadata. + * + * @param stream the license text stream, closed on every return path including failures + * @param offset the byte offset at which this dependency's license text begins + * @param length the number of bytes to read, or zero to read to the end of the stream + * @return the license text bytes + * @throws IllegalArgumentException if {@code offset} or {@code length} is negative + * @throws RuntimeException if the stream cannot be read + */ protected static byte[] getBytesFromInputStream( InputStream stream, long offset, int length) { - try { - byte[] buffer = new byte[1024] - ByteArrayOutputStream textArray = new ByteArrayOutputStream() - - stream.skip(offset) - int bytesRemaining = length > 0 ? length : Integer.MAX_VALUE - int bytes = 0 - - while (bytesRemaining > 0 - && (bytes = - stream.read( - buffer, - 0, - Math.min(bytesRemaining, buffer.length))) - != -1) { - textArray.write(buffer, 0, bytes) - bytesRemaining -= bytes + return stream.withCloseable { InputStream is -> + if (offset < 0 || length < 0) { + throw new IllegalArgumentException("offset and length must be non-negative: offset=$offset, length=$length") } - stream.close() + try { + byte[] buffer = new byte[1024] + ByteArrayOutputStream textArray = new ByteArrayOutputStream() + + is.skip(offset) + // A length of 0 means the license metadata omitted the field. Read to the end + // of the stream, as before, rather than silently shipping an empty attribution. + int bytesRemaining = length > 0 ? length : Integer.MAX_VALUE + int bytes = 0 + + while (bytesRemaining > 0 + && (bytes = + is.read( + buffer, + 0, + Math.min(bytesRemaining, buffer.length))) + != -1) { + textArray.write(buffer, 0, bytes) + bytesRemaining -= bytes + } - return textArray.toByteArray() - } catch (Exception e) { - throw new RuntimeException(FAIL_READING_LICENSES_ERROR, e) + return textArray.toByteArray() + } catch (Exception e) { + throw new RuntimeException(FAIL_READING_LICENSES_ERROR, e) + } } } @@ -351,15 +388,65 @@ abstract class LicensesTask extends DefaultTask { return new ArtifactInfo(entry.group, entry.name, entry.version) } + /** + * One attribution record: the key that identifies a dependency for deduplication, and the + * display name shown in the consuming app's license menu. + * + * Both values originate from dependency-authored metadata — a Maven POM {@code } element + * or a key in an AAR's third_party_licenses.json — and are sanitized here, at the only point + * where an instance can be created. The metadata file these records are written to is newline + * delimited, so an unsanitized line break would terminate a record early and let the remainder + * forge a second, fully attacker-controlled entry. + * + * Why are the fields {@code final}? A non-final Groovy property generates a public setter and + * enables the map constructor, either of which would write an unsanitized value straight past + * the constructor. + */ protected static class Dependency { - String key - String name - + final String key + final String name + + /** + * @param key identifies the dependency for deduplication; rejected when blank, as there is + * no substitute for it and a blank key silently collapses distinct dependencies + * @param name the display name, falling back to the sanitized key when blank, because a + * record with no display name attributes nothing; every current caller already + * supplies a non-blank name, so the fallback is defensive only + * @throws IllegalArgumentException if {@code key} is blank once sanitized + * @throws NullPointerException if {@code key} or {@code name} is null + */ Dependency(String key, String name) { - this.key = key - this.name = name + this.key = sanitize(key, "key") + if (this.key.isEmpty()) { + throw new IllegalArgumentException("key cannot be empty") + } + String sanitizedName = sanitize(name, "name") + this.name = sanitizedName.isEmpty() ? this.key : sanitizedName + } + + /** + * Collapses each run of line breaks in {@code value} to a single space, then trims. + * + * {@code \R} matches every Unicode line break — LF, CR, CRLF, vertical tab, form feed, + * NEL, and the U+2028 and U+2029 separators — so an unusual encoding cannot evade this. + * {@code strip} handles the resulting edges and, unlike {@code trim}, is Unicode-aware. + * + * @param value the raw, dependency-authored string + * @param fieldName the field being sanitized, used in the null-check message + * @return the single-line, trimmed value, empty only if {@code value} was blank + */ + private static String sanitize(String value, String fieldName) { + return Objects.requireNonNull(value, "$fieldName cannot be null") + .replaceAll(/\R+/, ' ') + .strip() } + /** + * Renders this dependency's line in the newline-delimited metadata file. + * + * @param offset the "start:length" pair locating this dependency's license text + * @return one record; the name is sanitized, so the line cannot be split + */ String buildLicensesMetadata(String offset) { return "$offset $name" } diff --git a/oss-licenses-plugin/src/test/java/com/google/android/gms/oss/licenses/plugin/LicensesTaskTest.java b/oss-licenses-plugin/src/test/java/com/google/android/gms/oss/licenses/plugin/LicensesTaskTest.java index b38741ca..f7202658 100644 --- a/oss-licenses-plugin/src/test/java/com/google/android/gms/oss/licenses/plugin/LicensesTaskTest.java +++ b/oss-licenses-plugin/src/test/java/com/google/android/gms/oss/licenses/plugin/LicensesTaskTest.java @@ -23,6 +23,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.gson.Gson; @@ -177,6 +178,77 @@ public void testAddLicensesFromPom_withDuplicate() throws IOException { assertEquals(expected, content); } + /** + * A POM {@code } is attacker-controlled. No line break encoding that survives an XML + * parser may forge an extra record in the line-delimited metadata file. + */ + @Test + public void testAddLicensesFromPom_sanitizesNewlinesInName() throws IOException { + File deps7 = getResourceFile("dependencies/groupG/deps7.pom"); + licensesTask.initOutputDir(); + licensesTask.addLicensesFromPom(deps7, "groupG", "deps7"); + licensesTask.writeMetadata(); + + byte[] licenseUrl = "http://www.opensource.org/licenses/mit-license.php".getBytes(UTF_8); + // Asserting the whole file, rather than a substring, is what proves no second record exists. + String expected = + "0:" + + licenseUrl.length + + " Forged Library 0:120 Fake Attribution 0:130 Another Fake CR Injected LS Injected" + + LINE_BREAK; + String content = + new String(Files.readAllBytes(licensesTask.getLicensesMetadata().toPath()), UTF_8); + assertThat(licensesTask.licensesMap.size(), is(1)); + assertEquals(expected, content); + } + + /** + * With multiple licenses the map key is composed from the attacker-controlled {@code } + * name, so newlines must be stripped from the key as well as the display name. An unsanitized + * key would split one dependency across two dedup entries. + */ + @Test + public void testAddLicensesFromPom_sanitizesNewlinesInMultipleLicenseKeys() throws IOException { + File deps8 = getResourceFile("dependencies/groupG/deps8.pom"); + licensesTask.initOutputDir(); + licensesTask.addLicensesFromPom(deps8, "groupG", "deps8"); + licensesTask.writeMetadata(); + + assertThat(licensesTask.licensesMap.size(), is(2)); + assertTrue(licensesTask.licensesMap.containsKey("groupG:deps8 MIT 0:120 Forged Key")); + assertTrue(licensesTask.licensesMap.containsKey("groupG:deps8 Apache License 2.0")); + + byte[] mit = "http://www.opensource.org/licenses/mit-license.php".getBytes(UTF_8); + byte[] apache = "https://www.apache.org/licenses/LICENSE-2.0".getBytes(UTF_8); + int secondOffset = mit.length + LINE_BREAK.getBytes(UTF_8).length; + String expected = + "0:" + mit.length + " Multi License Library" + + LINE_BREAK + + secondOffset + ":" + apache.length + " Multi License Library" + + LINE_BREAK; + String content = + new String(Files.readAllBytes(licensesTask.getLicensesMetadata().toPath()), UTF_8); + assertEquals(expected, content); + } + + /** + * A POM whose {@code } is blank once parsed is attributed by its Maven coordinate. The + * record must never be blank, because a blank display name attributes nothing. + */ + @Test + public void testAddLicensesFromPom_blankNameIsAttributedByCoordinate() throws IOException { + File deps9 = getResourceFile("dependencies/groupG/deps9.pom"); + licensesTask.initOutputDir(); + licensesTask.addLicensesFromPom(deps9, "groupG", "deps9"); + licensesTask.writeMetadata(); + + byte[] licenseUrl = "http://www.opensource.org/licenses/mit-license.php".getBytes(UTF_8); + String expected = "0:" + licenseUrl.length + " groupG:deps9" + LINE_BREAK; + String content = + new String(Files.readAllBytes(licensesTask.getLicensesMetadata().toPath()), UTF_8); + assertEquals(expected, content); + } + private File getResourceFile(String resourcePath) { return new File(getClass().getClassLoader().getResource(resourcePath).getFile()); } @@ -447,6 +519,119 @@ public void testWriteMetadata() throws IOException { assertEquals(expected, content); } + @Test + public void testWriteMetadata_sanitizesNewlinesInName() throws IOException { + byte[] licenseA = "licenseA".getBytes(UTF_8); + byte[] licenseB = "licenseB".getBytes(UTF_8); + byte[] licenseC = "licenseC".getBytes(UTF_8); + + licensesTask.initOutputDir(); + licensesTask.appendDependency( + new LicensesTask.Dependency("test:foo", "Dependency 1\n0:120 Forged Entry"), licenseA); + licensesTask.appendDependency( + new LicensesTask.Dependency("test:bar", "\r\nDependency 2\r\nSpoofed\r\n"), licenseB); + licensesTask.appendDependency( + new LicensesTask.Dependency("test:baz\nkey", "\r\n \n\r"), licenseC); + licensesTask.writeMetadata(); + + int lineBreakBytes = LINE_BREAK.getBytes(UTF_8).length; + int secondOffset = licenseA.length + lineBreakBytes; + int thirdOffset = secondOffset + licenseB.length + lineBreakBytes; + String expected = + "0:" + licenseA.length + " Dependency 1 0:120 Forged Entry" + + LINE_BREAK + + secondOffset + ":" + licenseB.length + " Dependency 2 Spoofed" + + LINE_BREAK + + thirdOffset + ":" + licenseC.length + " test:baz key" + + LINE_BREAK; + String content = + new String(Files.readAllBytes(licensesTask.getLicensesMetadata().toPath()), UTF_8); + assertEquals(expected, content); + } + + @Test(expected = IllegalArgumentException.class) + public void testDependency_emptyKeyThrowsException() { + new LicensesTask.Dependency(" \r\n\t ", "Valid Name"); + } + + @Test + public void testGetBytesFromInputStream_zeroLengthReadsToEnd() { + // A zero length means the license metadata omitted the field, so the whole stream is read. + InputStream inputStream = new ByteArrayInputStream("test".getBytes(UTF_8)); + byte[] content = LicensesTask.getBytesFromInputStream(inputStream, 0, 0); + assertEquals("test", new String(content, UTF_8)); + } + + @Test(expected = IllegalArgumentException.class) + public void testGetBytesFromInputStream_negativeLengthThrowsException() { + InputStream inputStream = new ByteArrayInputStream("test".getBytes(UTF_8)); + LicensesTask.getBytesFromInputStream(inputStream, 0, -1); + } + + @Test(expected = IllegalArgumentException.class) + public void testGetBytesFromInputStream_negativeOffsetThrowsException() { + InputStream inputStream = new ByteArrayInputStream("test".getBytes(UTF_8)); + LicensesTask.getBytesFromInputStream(inputStream, -1, 1); + } + + @Test + public void testGetBytesFromInputStream_invalidBoundsClosesStream() throws IOException { + InputStream inputStream = mock(InputStream.class); + try { + LicensesTask.getBytesFromInputStream(inputStream, -1, 1); + fail("This test should throw IllegalArgumentException."); + } catch (IllegalArgumentException expected) { + // Expected. + } + verify(inputStream).close(); + } + + @Test + public void testAddEmbeddedLicenses_sanitizesAndDeduplicatesKeysWithNewlines() throws IOException { + File artifactFile = temporaryFolder.newFile("newline-keys.aar"); + writeLicenseZip( + artifactFile, + "{\"foo\\nbar\": {\"start\": 0, \"length\": 4}, \"foo bar\": {\"start\": 0, \"length\": 4}}"); + + licensesTask.initOutputDir(); + licensesTask.addEmbeddedLicenses(artifactFile); + + assertThat(licensesTask.embeddedLicenses.size(), is(1)); + assertTrue(licensesTask.embeddedLicenses.contains("foo bar")); + assertThat(licensesTask.licensesMap.size(), is(1)); + assertTrue(licensesTask.licensesMap.containsKey("foo bar")); + } + + // A dependency author must not be able to break a consumer's build with a blank license key. + @Test + public void testAddEmbeddedLicenses_blankKeyIsSkippedNotFatal() throws IOException { + File artifactFile = temporaryFolder.newFile("blank-key.aar"); + writeLicenseZip( + artifactFile, + "{\"\\n\": {\"start\": 0, \"length\": 4}, \"valid\": {\"start\": 0, \"length\": 4}}"); + + licensesTask.initOutputDir(); + licensesTask.addEmbeddedLicenses(artifactFile); + + assertThat(licensesTask.embeddedLicenses.size(), is(1)); + assertTrue(licensesTask.embeddedLicenses.contains("valid")); + assertThat(licensesTask.licensesMap.size(), is(1)); + assertTrue(licensesTask.licensesMap.containsKey("valid")); + } + + /** Writes a minimal AAR containing the given {@code third_party_licenses.json} content. */ + private void writeLicenseZip(File artifactFile, String jsonContent) throws IOException { + try (ZipOutputStream output = new ZipOutputStream(new FileOutputStream(artifactFile))) { + output.putNextEntry(new ZipEntry("third_party_licenses.json")); + output.write(jsonContent.getBytes(UTF_8)); + output.closeEntry(); + + output.putNextEntry(new ZipEntry("third_party_licenses.txt")); + output.write("test".getBytes(UTF_8)); + output.closeEntry(); + } + } + @Test public void testDependenciesWithNameDuplicatedNames() throws IOException { File deps6 = getResourceFile("dependencies/groupF/deps6.pom"); diff --git a/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps7.pom b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps7.pom new file mode 100644 index 00000000..54d06f4b --- /dev/null +++ b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps7.pom @@ -0,0 +1,23 @@ + + 4.0.0 + + groupG + groupG-deps7 + 1 + + Forged Library +0:120 Fake Attribution 0:130 Another Fake CR Injected
LS Injected + groupG deps7 + + + + MIT License + http://www.opensource.org/licenses/mit-license.php + + + diff --git a/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps8.pom b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps8.pom new file mode 100644 index 00000000..ded5f85f --- /dev/null +++ b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps8.pom @@ -0,0 +1,26 @@ + + 4.0.0 + + groupG + groupG-deps8 + 1 + Multi License Library + groupG deps8 + + + + + MIT +0:120 Forged Key + http://www.opensource.org/licenses/mit-license.php + + + Apache License 2.0 + https://www.apache.org/licenses/LICENSE-2.0 + + + diff --git a/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps9.pom b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps9.pom new file mode 100644 index 00000000..436c3071 --- /dev/null +++ b/oss-licenses-plugin/src/test/resources/dependencies/groupG/deps9.pom @@ -0,0 +1,17 @@ + + 4.0.0 + + groupG + groupG-deps9 + 1 + + + groupG deps9 + + + + MIT License + http://www.opensource.org/licenses/mit-license.php + + +