From c051fc1d2f6a3bad17c38505b788c08bf012509c Mon Sep 17 00:00:00 2001 From: Tim Froehlich Date: Wed, 9 Sep 2026 13:40:04 -0500 Subject: [PATCH] fix(oss-licenses-plugin): sanitize newlines in license metadata records A dependency author could inject line breaks into a library display name, via either the Maven POM element or a key in an AAR's third_party_licenses.json, and forge extra records in the newline-delimited res/raw/third_party_license_metadata file. Each injected break yielded a fully attacker-controlled attribution entry in the consuming app's license menu. Sanitize both key and name in the Dependency constructor, the only point at which an instance can be created, collapsing every Unicode line break to a single space. Make the fields final so Groovy's generated setters and map constructor cannot write past the constructor. Reject a blank key, and log and skip such records in processLicenseEntry rather than breaking a consumer's build over one malformed third-party entry. Also harden getBytesFromInputStream: reject a negative offset or length, and close the stream on every path by way of withCloseable. BUG=557266592 CONV=b20a8ae9-55d6-4655-87f8-edba5da5028b TAG=agy --- .../oss/licenses/plugin/LicensesTask.groovy | 145 +++++++++++--- .../oss/licenses/plugin/LicensesTaskTest.java | 185 ++++++++++++++++++ .../resources/dependencies/groupG/deps7.pom | 23 +++ .../resources/dependencies/groupG/deps8.pom | 26 +++ .../resources/dependencies/groupG/deps9.pom | 17 ++ 5 files changed, 367 insertions(+), 29 deletions(-) create mode 100644 oss-licenses-plugin/src/test/resources/dependencies/groupG/deps7.pom create mode 100644 oss-licenses-plugin/src/test/resources/dependencies/groupG/deps8.pom create mode 100644 oss-licenses-plugin/src/test/resources/dependencies/groupG/deps9.pom 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 + + +