Reject unknown file type in CPIO entry mode - #790
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adjusts CPIO header parsing to reject unsupported file-type bits in c_mode without leaking a raw IllegalArgumentException, and adds regression tests for the three supported header formats.
Changes:
- Introduces a
setMode(...)helper to validate and applyc_modeacross new ASCII, old ASCII, and old binary readers. - Attempts to convert invalid mode/file-type failures into an
ArchiveException. - Adds unit tests covering invalid file type bits in
c_modefor all three magic formats.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStream.java | Centralizes mode parsing/validation and changes exception behavior for invalid file types. |
| src/test/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStreamTest.java | Adds regression tests asserting the new error behavior for invalid c_mode file types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (CpioUtil.fileType(mode) != 0) { // mode is initialized to 0 | ||
| newEntry.setMode(mode); | ||
| } | ||
| setMode(newEntry, mode); |
| if (CpioUtil.fileType(mode) != 0) { | ||
| ret.setMode(mode); | ||
| } | ||
| setMode(ret, mode); |
| if (CpioUtil.fileType(mode) != 0) { | ||
| oldEntry.setMode(mode); | ||
| } | ||
| setMode(oldEntry, mode); |
| private void setMode(final CpioArchiveEntry entry, final long mode) throws ArchiveException { | ||
| if (CpioUtil.fileType(mode) == 0) { | ||
| return; | ||
| } | ||
| try { | ||
| entry.setMode(mode); | ||
| } catch (final IllegalArgumentException e) { | ||
| throw new ArchiveException("Corrupted CPIO archive: Invalid file mode 0%s at byte: %,d", Long.toOctalString(mode), getBytesRead()); | ||
| } | ||
| } |
| try (CpioArchiveInputStream cpio = CpioArchiveInputStream.builder() | ||
| .setByteArray(header.getBytes(StandardCharsets.US_ASCII)) | ||
| .get()) { | ||
| assertThrows(ArchiveException.class, cpio::getNextEntry); | ||
| } |
|
Hello @kali834x |
|
Went through CpioArchiveEntry to see what is actually reachable from the three header readers. From archive input it's setMode (any of the seven undefined S_IFMT values, in all three formats) and setSize (the two ASCII formats can parse a negative size via a So agreed, a one-off wasn't the right shape. I dropped the helper and moved the conversion into getNextCPIOEntry instead: a single catch that rethrows IllegalArgumentException from header parsing as ArchiveException with the cause kept, same as ArArchiveInputStream.getNextEntry already does. That covers every setter the readers call, now and in the future, and added a regression test for the negative size case. I do think it's worth addressing since getNextEntry only declares IOException, and byte-mutating the bundled cpio test files reaches the setMode IAE readily. |
Invalid CPIO header fields escape as IllegalArgumentException
CpioArchiveEntry.setModeaccepts only the eight file types CPIO defines, so a craftedc_modenaming one of the seven other non-zeroS_IFMTvalues (0170000, 030000, ...) throws a rawIllegalArgumentExceptionout ofgetNextEntry, which declaresIOException.setSizehas the same problem in the two ASCII formats: a-in the size field parses to a negative value and the setter throws before thegetSize() < 0checks are reached.Rather than guarding individual call sites,
getNextCPIOEntrynow rethrowsIllegalArgumentExceptionfrom header parsing asArchiveException, keeping the original exception as the cause, the same wayArArchiveInputStream.getNextEntryalready does. This covers every entry setter the three header readers call. TheUnsupportedOperationExceptionsites inCpioArchiveEntryare format guards and each reader only calls the setters matching the format it constructs, so those are not reachable from input.mvn; that'smvnon the command line by itself.