Skip to content

fix(oss-licenses-plugin): sanitize newlines in Dependency.name to prevent metadata injection #461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
timothyfroehlich wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(oss-licenses-plugin): sanitize newlines in Dependency.name to prevent metadata injection #461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 116 additions & 29 deletions ...ses-plugin/src/main/groovy/com/google/android/gms/oss/licenses/plugin/LicensesTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
}
}

Expand Down Expand Up @@ -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 <name>} 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"
}
Expand Down
185 changes: 185 additions & 0 deletions ...ses-plugin/src/test/java/com/google/android/gms/oss/licenses/plugin/LicensesTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -177,6 +178,77 @@ public void testAddLicensesFromPom_withDuplicate() throws IOException {
assertEquals(expected, content);
}

/**
* A POM {@code <name>} 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 <license>}
* 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 <name>} 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());
}
Expand Down Expand Up @@ -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");
Expand Down
Loading
Loading