fix(oss-licenses-plugin): sanitize newlines in Dependency.name to prevent metadata injection - #461
timothyfroehlich wants to merge 1 commit into
Conversation
2172a66 to
fe25d93
Compare
fe25d93 to
88b5b04
Compare
| String expected = | ||
| "0:8 Dependency 1 0:120 Forged Entry" | ||
| + LINE_BREAK | ||
| + "9:8 Dependency 2 Spoofed" |
There was a problem hiding this comment.
Consider calculate the second offset dynamically to avoid issues on different OS.
There was a problem hiding this comment.
Done. Offsets are derived from the license byte lengths plus the separator width, so the test holds under both LF and CRLF.
| this.key = Objects.requireNonNull(key, "key cannot be null") | ||
| this.name = Objects.requireNonNull(name, "name cannot be null") | ||
| .replaceAll(/\R+/, ' ') | ||
| .strip() |
There was a problem hiding this comment.
Consider defaulting to key if name becomes empty after sanitization.
There was a problem hiding this comment.
Done: this.name = sanitizedName.isEmpty() ? this.key : sanitizedName.
Worth flagging that this fallback is defensive only. All four new Dependency(...) call sites already guarantee a non-blank name: two pass the key as the name, and the POM path falls back to licenseKey at addLicensesFromPom:315-317. testAddLicensesFromPom_blankNameIsAttributedByCoordinate pins the reachable path.
| Dependency(String key, String name) { | ||
| this.key = key | ||
| this.name = name | ||
| this.key = Objects.requireNonNull(key, "key cannot be null") |
There was a problem hiding this comment.
Key should be sanitized as well?
There was a problem hiding this comment.
Done. Both fields now run through a shared sanitize helper in the constructor.
The key mattered for a second reason. processLicenseEntry deduplicates on it, and the raw key was feeding embeddedLicenses while the sanitized key fed licensesMap. That made "foo\nbar" and "foo bar" two dedup entries but one map entry. Both now use dependency.key.
37bc025 to
35b6f0c
Compare
| InputStream stream, | ||
| long offset, | ||
| int length) { | ||
| if (offset < 0 || length < 0) { |
There was a problem hiding this comment.
This is before stream.withCloseable, throwing IllegalArgumentException leaves stream open
There was a problem hiding this comment.
I wasn't ready for re-review yet, was fixing up some things first.
There was a problem hiding this comment.
Good catch, fixed. The guard now sits inside withCloseable but outside the inner try. That is the only arrangement that closes the stream and still lets the IllegalArgumentException propagate unwrapped, since inside the try the existing catch-all would rewrap it as RuntimeException(FAIL_READING_LICENSES_ERROR, e).
testGetBytesFromInputStream_invalidBoundsClosesStream asserts verify(inputStream).close() on the throwing path.
| new LicensesTask.Dependency("test:baz\nkey", "\r\n \n\r"), licenseC); | ||
| licensesTask.writeMetadata(); | ||
|
|
||
| int secondOffset = licenseA.length + LINE_BREAK.length(); |
There was a problem hiding this comment.
Use LINE_BREAK.getBytes(UTF_8).length keeps all offset arithmetic strictly in bytes.
There was a problem hiding this comment.
Agreed, done. LINE_BREAK.length() is a char count being summed with licenseA.length, a byte count. They agree only because separators are ASCII. Now hoisted to int lineBreakBytes = LINE_BREAK.getBytes(UTF_8).length.
A dependency author could inject line breaks into a library display name, via either the Maven POM <name> 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
35b6f0c to
c051fc1
Compare
|
Pushed Changes since your review:
|
2 participants
Strip CR/LF characters and trim whitespace in
Dependencyconstructor, and makekey/namefinalto prevent Groovy setter bypass.