This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch fix/7z-size-validation in repository https://gitbox.apache.org/repos/asf/commons-compress.git
commit 91e375d7ec238e7ed99bede569120413c7f523dc Author: Piotr P. Karwasz <[email protected]> AuthorDate: Sat Oct 18 00:01:34 2025 +0200 7z: unsigned number parsing and improved header validation The 7z file format specification defines only **unsigned numbers** (`UINT64`, `REAL_UINT64`, `UINT32`). However, the current implementation allows parsing methods like `readUint64`, `getLong`, and `getInt` to return negative values and then handles those inconsistently in downstream logic. This PR introduces a safer and more specification-compliant number parsing model. ### Key changes * **Strict unsigned number parsing** * Parsing methods now *never* return negative numbers. * `readUint64`, `readUint64ToIntExact`, `readRealUint64`, and `readUint32` follow the terminology from `7zFormat.txt`. * Eliminates scattered negative-value checks that previously compensated for parsing issues. * **Improved header integrity validation** * Before large allocations, the size is now validated against the **actual available data in the header** as well as the memory limit. * Prevents unnecessary or unsafe allocations when the archive is corrupted or truncated. * **Correct numeric type usage** * Some fields represent 7z numbers as 64-bit values but are constrained internally to Java `int` limits. * These are now declared as `int` to signal real constraints in our implementation. * **Consistent error handling** Parsing now throws only three well-defined exception types: | Condition | Exception | | ---------------------------------------------------------------------- | -------------------------------------------- | | Declared structure exceeds `maxMemoryLimitKiB` | `MemoryLimitException` | | Missing data inside header (truncated or corrupt) | `ArchiveException("Corrupted 7z archive")` | | Unsupported numeric values (too large for implementation) | `ArchiveException("Unsupported 7z archive")` | Note: `EOFException` is no longer used: a header with missing fields is not “EOF,” it is **corrupted**. This PR lays groundwork for safer parsing and easier future maintenance by aligning number handling with the actual 7z specification and making header parsing behavior *predictable and robust*. --- src/changes/changes.xml | 1 + .../compress/archivers/ArchiveException.java | 18 - .../compress/archivers/sevenz/SevenZFile.java | 592 ++++++++++++--------- .../compress/archivers/sevenz/StartHeader.java | 8 +- .../compress/archivers/sevenz/SubStreamsInfo.java | 22 +- .../compress/archivers/sevenz/SevenZFileTest.java | 104 ++++ 6 files changed, 467 insertions(+), 278 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index af7bf4aa4..665307373 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -57,6 +57,7 @@ The <action> type attribute can be add,update,fix,remove. <action type="fix" dev="ggregory" due-to="Gary Gregory">Don't loose precision while reading folders from a SevenZFile.</action> <action type="fix" dev="ggregory" due-to="Roel van Dijk, Gary Gregory">Improve some exception messages in TarUtils and TarArchiveEntry.</action> <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">SevenZFile now enforces the same folder and coder limits as the CPP implementation.</action> + <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">Refactor unsigned number parsing and header validation in SevenZFile.</action> <!-- FIX bzip2 --> <action type="fix" dev="ggregory" due-to="Tyler Nighswander, Gary Gregory">BZip2CompressorInputStream now throw CompressorException (a subclass of IOException) for invalid or corrupted data, providing more specific error reporting.</action> <action type="fix" dev="pkarwasz" due-to="Tyler Nighswander, Piotr P. Karwasz">BZip2 input streams treat Huffman codes longer than 20 bits as corrupted data, matching the behavior of the reference implementation.</action> diff --git a/src/main/java/org/apache/commons/compress/archivers/ArchiveException.java b/src/main/java/org/apache/commons/compress/archivers/ArchiveException.java index 505183077..a2c4c0a31 100644 --- a/src/main/java/org/apache/commons/compress/archivers/ArchiveException.java +++ b/src/main/java/org/apache/commons/compress/archivers/ArchiveException.java @@ -33,24 +33,6 @@ public class ArchiveException extends CompressException { /** Serial. */ private static final long serialVersionUID = 2772690708123267100L; - /** - * Delegates to {@link Math#addExact(int, int)} wrapping its {@link ArithmeticException} in our {@link ArchiveException}. - * - * @param x the first value. - * @param y the second value. - * @return the result. - * @throws ArchiveException if the result overflows an {@code int}. - * @see Math#addExact(int, int) - * @since 1.29.0 - */ - public static int addExact(final int x, final int y) throws ArchiveException { - try { - return Math.addExact(x, y); - } catch (final ArithmeticException e) { - throw new ArchiveException(e); - } - } - /** * Delegates to {@link Math#addExact(int, int)} wrapping its {@link ArithmeticException} in our {@link ArchiveException}. * diff --git a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java index 112e5fe5f..7a7b8b2a4 100644 --- a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java +++ b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java @@ -161,7 +161,7 @@ private long streamMapSize() { @Override public String toString() { return String.format("Archive with %,d entries in %,d folders, estimated size %,d KiB.", numberOfEntries, numberOfFolders, - kbToKiB(estimateSizeBytes())); + bytesToKiB(estimateSizeBytes())); } } @@ -226,7 +226,7 @@ public Builder setDefaultName(final String defaultName) { * @return {@code this} instance. */ public Builder setMaxMemoryLimitKb(final int maxMemoryLimitKb) { - this.maxMemoryLimitKiB = kbToKiB(maxMemoryLimitKb); + this.maxMemoryLimitKiB = maxMemoryLimitKb * 1000 / 1024; return this; } @@ -363,6 +363,51 @@ public Builder setUseDefaultNameForUnnamedEntries(final boolean useDefaultNameFo */ private static final int MAX_CODER_STREAMS_PER_FOLDER = 64; + /** Minimum number of bytes a 7z UINT64 can occupy. */ + private static final long MIN_UINT64_BYTES = 1L; + + /** Number of bytes a 7z UINT32 occupies. */ + private static final long UINT32_BYTES = 4L; + + /** Number of bytes a 7z REAL_UINT64 occupies. */ + private static final long REAL_UINT64_BYTES = 8L; + + /** + * Computes a partial count or sum of 7z objects, throwing ArchiveException if any limit is exceeded. + * + * @param sum current sum + * @param y second integer + * @param description description of the value being added, for error messages + * @return the new sum + * @throws ArchiveException if the sum overflows an int + */ + private static int accumulate(final int sum, final int y, final String description) throws ArchiveException { + try { + return Math.addExact(sum, y); + } catch (final ArithmeticException e) { + throw new ArchiveException("Unsupported 7-Zip archive: cannot handle more than %,d %s, but %,d present", Integer.MAX_VALUE, description, + Long.sum(sum, y)); + } + } + + /** + * Computes a partial count or sum of 7z objects, throwing ArchiveException if any limit is exceeded. + * + * @param sum current sum + * @param y second integer + * @param description description of the value being added, for error messages + * @return the new sum + * @throws ArchiveException if the sum overflows an int + */ + private static long accumulate(final long sum, final long y, final String description) throws ArchiveException { + try { + return Math.addExact(sum, y); + } catch (final ArithmeticException e) { + throw new ArchiveException("Unsupported 7-Zip archive: cannot handle more than %,d %s, but %,d present", Integer.MAX_VALUE, description, + Long.sum(sum, y)); + } + } + /** * Creates a new Builder. * @@ -377,46 +422,47 @@ static long bytesToKiB(final long bytes) { return bytes / 1024; } - private static ByteBuffer checkEndOfFile(final ByteBuffer buf, final int expectRemaining) throws EOFException { - final int remaining = buf.remaining(); - if (remaining < expectRemaining) { - throw new EOFException(String.format("remaining %,d < expectRemaining %,d", remaining, expectRemaining)); + /** + * Checks that there are at least {@code expectRemaining} bytes remaining in the header. + * + * @param header The buffer containing the 7z header. + * @param expectRemaining The number of bytes expected to be remaining. + * @return {@code header} for easy chaining. + * @throws ArchiveException if there are not enough bytes remaining, implying that the 7z header is incomplete or corrupted. + */ + private static ByteBuffer ensureRemaining(final ByteBuffer header, final long expectRemaining) throws ArchiveException { + if (expectRemaining > header.remaining()) { + throw new ArchiveException("Corrupted 7z archive: expecting %,d bytes, remaining header size %,d", expectRemaining, header.remaining()); } - return buf; + return header; } - private static void get(final ByteBuffer buf, final byte[] to) throws EOFException { - checkEndOfFile(buf, to.length).get(to); - } - - private static int getInt(final ByteBuffer buf) throws EOFException { - return checkEndOfFile(buf, Integer.BYTES).getInt(); - } - - private static long getLong(final ByteBuffer buf) throws EOFException { - return checkEndOfFile(buf, Long.BYTES).getLong(); + /** + * Wrapper of {@link ByteBuffer#get(byte[])} that checks remaining bytes first. + */ + private static void get(final ByteBuffer buf, final byte[] to) throws ArchiveException { + ensureRemaining(buf, to.length).get(to); } /** - * Gets the next unsigned byte as an int. - * - * @param buf the byte source. - * @return the next unsigned byte as an int. - * @throws EOFException Thrown if the given buffer doesn't have a remaining byte. + * Wrapper of {@link ByteBuffer#getInt()} that checks remaining bytes first. */ - private static int getUnsignedByte(final ByteBuffer buf) throws EOFException { - if (!buf.hasRemaining()) { - throw new EOFException(); - } - return buf.get() & 0xff; + private static int getInt(final ByteBuffer buf) throws ArchiveException { + return ensureRemaining(buf, Integer.BYTES).getInt(); } - private static int kbToKiB(final int kilobytes) { - return kilobytes * 1000 / 1024; + /** + * Wrapper of {@link ByteBuffer#getLong()} that checks remaining bytes first. + */ + private static long getLong(final ByteBuffer buf) throws ArchiveException { + return ensureRemaining(buf, Long.BYTES).getLong(); } - static long kbToKiB(final long kilobytes) { - return kilobytes * 1000 / 1024; + /** + * Checks remaining bytes and reads one unsigned byte. + */ + private static int getUnsignedByte(final ByteBuffer header) throws ArchiveException { + return Byte.toUnsignedInt(ensureRemaining(header, Byte.BYTES).get()); } /** @@ -431,50 +477,131 @@ public static boolean matches(final byte[] buffer, final int ignored) { return ArrayUtils.startsWith(buffer, SIGNATURE); } - private static long readUint64(final ByteBuffer in) throws IOException { + /** + * Reads the size of a header field and validates that it is not larger than the remaining bytes in the header buffer. + * + * @param header the buffer containing the 7z header. + * @return a non-negative int. + * @throws ArchiveException if the value is truncated, too large, or exceeds the remaining bytes in the header buffer. + */ + static int readFieldSize(final ByteBuffer header) throws ArchiveException { + final long propertySize = readUint64(header); + ensureRemaining(header, propertySize); + // propertySize is not larger than header.remaining() which is an int + return (int) propertySize; + } + + /** + * Reads a 7z REAL_UINT64 from the stream. + * + * @param inputStream the input stream containing the 7z header. + * @return a non-negative long. + * @throws ArchiveException if the value is truncated or too large. + */ + static long readRealUint64(final DataInputStream inputStream) throws IOException { + final long value = Long.reverseBytes(inputStream.readLong()); + if (value < 0) { + throw new ArchiveException("Unsupported 7-Zip archive: cannot handle integer larger then %d, but was %s", Integer.MAX_VALUE, + Long.toUnsignedString(value)); + } + return value; + } + + /** + * Reads a 7z UINT32 from the header. + * + * @param header the buffer containing the 7z header. + * @return a non-negative long. + * @throws ArchiveException if the value is truncated. + */ + static long readUint32(final ByteBuffer header) throws ArchiveException { + return Integer.toUnsignedLong(getInt(header)); + } + + + /** + * Reads a 7z UINT32 from the stream. + * + * @param inputStream the input stream containing the 7z header. + * @return a non-negative long. + * @throws ArchiveException if the value is truncated. + */ + static long readUint32(final DataInputStream inputStream) throws IOException { + return Integer.toUnsignedLong(Integer.reverseBytes(inputStream.readInt())); + } + + /** + * Reads a 7z UINT64 from the header. + * + * @param header the buffer containing the 7z header. + * @return a non-negative long. + * @throws ArchiveException if the value is truncated or too large. + */ + static long readUint64(final ByteBuffer header) throws ArchiveException { // long rather than int as it might get shifted beyond the range of an int - final long firstByte = getUnsignedByte(in); + final long firstByte = getUnsignedByte(header); int mask = 0x80; long value = 0; for (int i = 0; i < 8; i++) { if ((firstByte & mask) == 0) { - return value | (firstByte & mask - 1) << 8 * i; + value |= (firstByte & mask - 1) << 8 * i; + break; } - final long nextByte = getUnsignedByte(in); + final long nextByte = getUnsignedByte(header); value |= nextByte << 8 * i; mask >>>= 1; } + if (value < 0) { + throw new ArchiveException("Unsupported 7-Zip archive: can not handle integer values larger than %,d", Long.MAX_VALUE); + } return value; } - private static int readUint64ToIntExact(final ByteBuffer in) throws IOException { - return ArchiveException.toIntExact(readUint64(in)); + /** + * Reads a 7z UINT64 from the header. + * + * <p>If the value is used as the length of a header field, use {@link #readFieldSize} instead, which also validates it against the number of remaining + * bytes in the header.</p> + * + * @param header the buffer containing the 7z header. + * @return a non-negative int. + * @throws ArchiveException if the value is truncated or too large. + * @see #readFieldSize(ByteBuffer) + */ + private static int readUint64ToIntExact(final ByteBuffer header, final String description) throws ArchiveException { + final long value = readUint64(header); + // Values larger than Integer.MAX_VALUE are not formally forbidden, but we cannot handle them. + if (value > Integer.MAX_VALUE) { + throw new ArchiveException("Unsupported 7-Zip archive: cannot handle %s larger then %,d, but was %,d", description, Integer.MAX_VALUE, value); + } + return (int) value; } - private static long skipBytesFully(final ByteBuffer input, long bytesToSkip) { - if (bytesToSkip < 1) { - return 0; - } - final int current = input.position(); - final int maxSkip = input.remaining(); - if (maxSkip < bytesToSkip) { - bytesToSkip = maxSkip; - } - input.position(current + (int) bytesToSkip); - return bytesToSkip; + /** + * Skips the given number of bytes of an unsupported property. + * + * @param header the 7z header buffer. + * @param propertySize the number of bytes to skip. + * @throws ArchiveException if the property size exceeds the remaining bytes in the header buffer. + */ + private static void skipBytesFully(final ByteBuffer header, final long propertySize) throws ArchiveException { + // propertySize is not larger than header.remaining(), which is an int + ensureRemaining(header, propertySize).position(header.position() + (int) propertySize); } /** - * Throws IOException if the given value is not in {@code [0, Integer.MAX_VALUE]}. + * Throws ArchiveException if the given value is not in {@code [0, Integer.MAX_VALUE]}. * - * @param description A description for the IOException. - * @param value The value to check. + * @param description A description for the exception. + * @param value The value to check, interpreted as unsigned. * @return The given value as an int. - * @throws IOException Thrown if the given value is not in {@code [0, Integer.MAX_VALUE]}. + * @throws ArchiveException Thrown if the given value is not in {@code [0, Integer.MAX_VALUE]}. */ - private static int toNonNegativeInt(final String description, final long value) throws IOException { - if (value > Integer.MAX_VALUE || value < 0) { - throw new ArchiveException("Cannot handle %s %,d", description, value); + private static int toNonNegativeInt(final String description, final long value) throws ArchiveException { + assert value >= 0 : "value is supposed to be non-negative"; + if (value > Integer.MAX_VALUE) { + throw new ArchiveException("Unsupported 7-Zip archive: cannot handle %s larger then %d, but was %s", description, Integer.MAX_VALUE, + Long.toUnsignedString(value)); } return (int) value; } @@ -769,7 +896,7 @@ private InputStream buildDecoderStack(final Folder folder, final long folderOffs InputStream inputStreamStack = new FilterInputStream( new BufferedInputStream(new BoundedSeekableByteChannelInputStream(channel, archive.packSizes[firstPackStreamIndex]))) { private void count(final int c) throws ArchiveException { - compressedBytesReadFromCurrentEntry = ArchiveException.addExact(compressedBytesReadFromCurrentEntry, c); + compressedBytesReadFromCurrentEntry = accumulate(compressedBytesReadFromCurrentEntry, c, "compressed bytes read from current entry"); } @Override @@ -901,21 +1028,21 @@ private void buildDecodingStream(final int entryIndex, final boolean isRandomAcc private void calculateStreamMap(final Archive archive) throws IOException { int nextFolderPackStreamIndex = 0; - final int numFolders = ArrayUtils.getLength(archive.folders); - final int[] folderFirstPackStreamIndex = new int[checkIntArray(numFolders)]; + final int numFolders = archive.folders.length; + final int[] folderFirstPackStreamIndex = intArray(numFolders); for (int i = 0; i < numFolders; i++) { folderFirstPackStreamIndex[i] = nextFolderPackStreamIndex; - nextFolderPackStreamIndex = ArchiveException.addExact(nextFolderPackStreamIndex, archive.folders[i].packedStreams.length); + nextFolderPackStreamIndex = accumulate(nextFolderPackStreamIndex, archive.folders[i].packedStreams.length, "nextFolderPackStreamIndex"); } long nextPackStreamOffset = 0; final int numPackSizes = archive.packSizes.length; - final long[] packStreamOffsets = new long[checkLongArray(numPackSizes)]; + final long[] packStreamOffsets = longArray(numPackSizes); for (int i = 0; i < numPackSizes; i++) { packStreamOffsets[i] = nextPackStreamOffset; - nextPackStreamOffset = ArchiveException.addExact(nextPackStreamOffset, archive.packSizes[i]); + nextPackStreamOffset = accumulate(nextPackStreamOffset, archive.packSizes[i], "nextPackStreamOffset"); } - final int[] folderFirstFileIndex = new int[checkIntArray(numFolders)]; - final int[] fileFolderIndex = new int[checkIntArray(archive.files.length)]; + final int[] folderFirstFileIndex = intArray(numFolders); + final int[] fileFolderIndex = intArray(archive.files.length); int nextFolderIndex = 0; int nextFolderUnpackStreamIndex = 0; for (int i = 0; i < archive.files.length; i++) { @@ -947,26 +1074,6 @@ private void calculateStreamMap(final Archive archive) throws IOException { archive.streamMap = new StreamMap(folderFirstPackStreamIndex, packStreamOffsets, folderFirstFileIndex, fileFolderIndex); } - int checkByteArray(final int size) throws MemoryLimitException { - MemoryLimitException.checkKiB(bytesToKiB(size * Byte.BYTES), maxMemoryLimitKiB); - return size; - } - - int checkIntArray(final int size) throws MemoryLimitException { - MemoryLimitException.checkKiB(bytesToKiB(size * Integer.BYTES), maxMemoryLimitKiB); - return size; - } - - int checkLongArray(final int size) throws MemoryLimitException { - MemoryLimitException.checkKiB(bytesToKiB(size * Long.BYTES), maxMemoryLimitKiB); - return size; - } - - int checkObjectArray(final int size) throws MemoryLimitException { - MemoryLimitException.checkKiB(bytesToKiB(size * 4), maxMemoryLimitKiB); // assume compressed pointer - return size; - } - /** * Closes the archive. * @@ -1144,14 +1251,13 @@ private boolean hasCurrentEntryBeenRead() { } private Archive initializeArchive(final StartHeader startHeader, final byte[] password, final boolean verifyCrc) throws IOException { - final int nextHeaderSizeInt = toNonNegativeInt("startHeader.nextHeaderSize", startHeader.nextHeaderSize); - MemoryLimitException.checkKiB(bytesToKiB(nextHeaderSizeInt), Math.min(bytesToKiB(org.apache.commons.io.IOUtils.SOFT_MAX_ARRAY_LENGTH), + MemoryLimitException.checkKiB(bytesToKiB(startHeader.nextHeaderSize), Math.min(bytesToKiB(org.apache.commons.io.IOUtils.SOFT_MAX_ARRAY_LENGTH), maxMemoryLimitKiB)); channel.position(SIGNATURE_HEADER_SIZE + startHeader.nextHeaderOffset); if (verifyCrc) { final long position = channel.position(); final CheckedInputStream cis = new CheckedInputStream(Channels.newInputStream(channel), new CRC32()); - if (cis.skip(nextHeaderSizeInt) != nextHeaderSizeInt) { + if (cis.skip(startHeader.nextHeaderSize) != startHeader.nextHeaderSize) { throw new ArchiveException("Problem computing NextHeader CRC-32"); } if (startHeader.nextHeaderCrc != cis.getChecksum().getValue()) { @@ -1160,7 +1266,7 @@ private Archive initializeArchive(final StartHeader startHeader, final byte[] pa channel.position(position); } Archive archive = new Archive(); - ByteBuffer buf = ByteBuffer.allocate(nextHeaderSizeInt).order(ByteOrder.LITTLE_ENDIAN); + ByteBuffer buf = ByteBuffer.allocate(startHeader.nextHeaderSize).order(ByteOrder.LITTLE_ENDIAN); readFully(buf); int nid = getUnsignedByte(buf); if (nid == NID.kEncodedHeader) { @@ -1177,6 +1283,30 @@ private Archive initializeArchive(final StartHeader startHeader, final byte[] pa return archive; } + /** + * Creates an int array while checking memory limits. + * + * @param size the size of the array + * @return the int array + * @throws MemoryLimitException if memory limit is exceeded + */ + private int[] intArray(final int size) throws MemoryLimitException { + MemoryLimitException.checkKiB(bytesToKiB((long) size * Integer.BYTES), maxMemoryLimitKiB); + return new int[size]; + } + + /** + * Creates a long array while checking memory limits. + * + * @param size the size of the array + * @return the long array + * @throws MemoryLimitException if memory limit is exceeded + */ + private long[] longArray(final int size) throws MemoryLimitException { + MemoryLimitException.checkKiB(bytesToKiB((long) size * Long.BYTES), maxMemoryLimitKiB); + return new long[size]; + } + /** * Reads a byte of data. * @@ -1219,7 +1349,7 @@ public int read(final byte[] b, final int off, final int len) throws IOException @SuppressWarnings("resource") // does not allocate final int current = getCurrentStream().read(b, off, len); if (current > 0) { - uncompressedBytesReadFromCurrentEntry = ArchiveException.addExact(uncompressedBytesReadFromCurrentEntry, current); + uncompressedBytesReadFromCurrentEntry = accumulate(uncompressedBytesReadFromCurrentEntry, current, "uncompressedBytesReadFromCurrentEntry"); } return current; } @@ -1238,18 +1368,19 @@ private BitSet readAllOrBits(final ByteBuffer header, final int size) throws IOE return bits; } - private void readArchiveProperties(final ByteBuffer input) throws IOException { + private void readArchiveProperties(final ByteBuffer header) throws IOException { // FIXME: the reference implementation just throws them away? - long nid = readUint64(input); + long nid = readUint64(header); while (nid != NID.kEnd) { - final int propertySize = readUint64ToIntExact(input); - final byte[] property = new byte[checkByteArray(propertySize)]; - get(input, property); - nid = readUint64(input); + // We validate the size but ignore the value + final int propertySize = readFieldSize(header); + skipBytesFully(header, propertySize); + nid = readUint64(header); } } private BitSet readBits(final ByteBuffer header, final int size) throws IOException { + ensureRemaining(header, (size + 7) / 8); final BitSet bits = new BitSet(size); int mask = 0; int cache = 0; @@ -1310,7 +1441,7 @@ private ByteBuffer readEncodedHeader(final ByteBuffer header, final Archive arch } private void readFilesInfo(final ByteBuffer header, final Archive archive) throws IOException { - final int numFilesInt = readUint64ToIntExact(header); + final int numFilesInt = readUint64ToIntExact(header, "numFiles"); final Map<Integer, SevenZArchiveEntry> fileMap = new LinkedHashMap<>(); BitSet isEmptyStream = null; BitSet isEmptyFile = null; @@ -1318,14 +1449,11 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw final int originalLimit = header.limit(); while (true) { final int propertyType = getUnsignedByte(header); - if (propertyType == 0) { + if (propertyType == NID.kEnd) { break; } - final int size = readUint64ToIntExact(header); - if (size < 0 || size > header.remaining()) { - throw new ArchiveException("Corrupted 7z archive: property size %,d, but only %,d bytes available", size, header.remaining()); - } - // Limit the buffer to the size of the property + final int size = readFieldSize(header); + // Limit the buffer to the size of the property, so we don't read beyond it header.limit(header.position() + size); switch (propertyType) { case NID.kEmptyStream: { @@ -1426,6 +1554,10 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw break; } } + // We should have consumed all the bytes by now + if (header.remaining() > 0) { + throw new ArchiveException("Unsupported 7z archive: property 0x%02d has %d trailing bytes.", propertyType, header.remaining()); + } // Restore original limit header.limit(originalLimit); } @@ -1446,9 +1578,6 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw entryAtIndex.setHasCrc(archive.subStreamsInfo.hasCrc.get(nonEmptyFileCounter)); entryAtIndex.setCrcValue(archive.subStreamsInfo.crcs[nonEmptyFileCounter]); entryAtIndex.setSize(archive.subStreamsInfo.unpackSizes[nonEmptyFileCounter]); - if (entryAtIndex.getSize() < 0) { - throw new ArchiveException("Broken archive, entry with negative size"); - } ++nonEmptyFileCounter; } else { entryAtIndex.setDirectory(isEmptyFile == null || !isEmptyFile.get(emptyFileCounter)); @@ -1477,8 +1606,7 @@ Folder readFolder(final ByteBuffer header) throws IOException { final boolean isSimple = (bits & 0x10) == 0; final boolean hasAttributes = (bits & 0x20) != 0; final boolean moreAlternativeMethods = (bits & 0x80) != 0; - final byte[] decompressionMethodId = new byte[idSize]; - get(header, decompressionMethodId); + final byte[] decompressionMethodId = toByteArray(header, idSize); final long numInStreams; final long numOutStreams; if (isSimple) { @@ -1501,9 +1629,8 @@ Folder readFolder(final ByteBuffer header) throws IOException { totalOutStreams += (int) numOutStreams; byte[] properties = null; if (hasAttributes) { - final long propertiesSize = readUint64(header); - properties = new byte[checkByteArray(ArchiveException.toIntExact(propertiesSize))]; - get(header, properties); + final int propertiesSize = readFieldSize(header); + properties = toByteArray(header, propertiesSize); } // would need to keep looping as above: if (moreAlternativeMethods) { @@ -1596,7 +1723,7 @@ private Archive readHeaders(final byte[] password) throws IOException { throw new ArchiveException("Unsupported 7z version (%d,%d)", archiveVersionMajor, archiveVersionMinor); } boolean headerLooksValid = false; // See https://www.7-zip.org/recover.html - "There is no correct End Header at the end of archive" - final long startHeaderCrc = 0xffffFFFFL & buf.getInt(); + final long startHeaderCrc = readUint32(buf); if (startHeaderCrc == 0) { // This is an indication of a corrupt header - peek the next 20 bytes final long currentPosition = channel.position(); @@ -1626,10 +1753,11 @@ private Archive readHeaders(final byte[] password) throws IOException { private void readPackInfo(final ByteBuffer header, final Archive archive) throws IOException { archive.packPos = readUint64(header); - final int numPackStreamsInt = readUint64ToIntExact(header); + final int numPackStreamsInt = readUint64ToIntExact(header, "numPackStreams"); int nid = getUnsignedByte(header); if (nid == NID.kSize) { - archive.packSizes = new long[checkLongArray(numPackStreamsInt)]; + ensureRemaining(header, MIN_UINT64_BYTES * numPackStreamsInt); + archive.packSizes = longArray(numPackStreamsInt); for (int i = 0; i < archive.packSizes.length; i++) { archive.packSizes[i] = readUint64(header); } @@ -1637,10 +1765,11 @@ private void readPackInfo(final ByteBuffer header, final Archive archive) throws } if (nid == NID.kCRC) { archive.packCrcsDefined = readAllOrBits(header, numPackStreamsInt); - archive.packCrcs = new long[checkLongArray(numPackStreamsInt)]; + ensureRemaining(header, UINT32_BYTES * archive.packCrcsDefined.cardinality()); + archive.packCrcs = longArray(numPackStreamsInt); for (int i = 0; i < numPackStreamsInt; i++) { if (archive.packCrcsDefined.get(i)) { - archive.packCrcs[i] = 0xffffFFFFL & getInt(header); + archive.packCrcs[i] = readUint32(header); } } // read one more @@ -1658,16 +1787,15 @@ private StartHeader readStartHeader(final long startHeaderCrc) throws IOExceptio .setExpectedChecksumValue(startHeaderCrc) .get())) { // @formatter:on - final long nextHeaderOffset = Long.reverseBytes(dataInputStream.readLong()); - if (nextHeaderOffset < 0 || nextHeaderOffset + SIGNATURE_HEADER_SIZE > channel.size()) { + final long nextHeaderOffset = readRealUint64(dataInputStream); + if (nextHeaderOffset > channel.size() - SIGNATURE_HEADER_SIZE) { throw new ArchiveException("nextHeaderOffset is out of bounds"); } - final long nextHeaderSize = Long.reverseBytes(dataInputStream.readLong()); - final long nextHeaderEnd = nextHeaderOffset + nextHeaderSize; - if (nextHeaderEnd < nextHeaderOffset || nextHeaderEnd + SIGNATURE_HEADER_SIZE > channel.size()) { + final int nextHeaderSize = toNonNegativeInt("nextHeaderSize", readRealUint64(dataInputStream)); + if (nextHeaderSize > channel.size() - SIGNATURE_HEADER_SIZE - nextHeaderOffset) { throw new ArchiveException("nextHeaderSize is out of bounds"); } - final long nextHeaderCrc = 0xffffFFFFL & Integer.reverseBytes(dataInputStream.readInt()); + final long nextHeaderCrc = readUint32(dataInputStream); return new StartHeader(nextHeaderOffset, nextHeaderSize, nextHeaderCrc); } } @@ -1695,14 +1823,13 @@ private void readSubStreamsInfo(final ByteBuffer header, final Archive archive) for (final Folder folder : archive.folders) { folder.numUnpackSubStreams = 1; } - long unpackStreamsCount = archive.folders.length; + int unpackStreamsCount = archive.folders.length; int nid = getUnsignedByte(header); if (nid == NID.kNumUnpackStream) { unpackStreamsCount = 0; for (final Folder folder : archive.folders) { - final long numStreams = readUint64(header); - folder.numUnpackSubStreams = (int) numStreams; - unpackStreamsCount = ArchiveException.addExact(unpackStreamsCount, numStreams); + folder.numUnpackSubStreams = readUint64ToIntExact(header, "numUnpackSubStreams"); + unpackStreamsCount = accumulate(unpackStreamsCount, folder.numUnpackSubStreams, "numUnpackStreams"); } nid = getUnsignedByte(header); } @@ -1712,18 +1839,19 @@ private void readSubStreamsInfo(final ByteBuffer header, final Archive archive) if (folder.numUnpackSubStreams == 0) { continue; } - long sum = 0; + long totalUnpackSize = 0; if (nid == NID.kSize) { + ensureRemaining(header, MIN_UINT64_BYTES * (folder.numUnpackSubStreams - 1)); for (int i = 0; i < folder.numUnpackSubStreams - 1; i++) { final long size = readUint64(header); subStreamsInfo.unpackSizes[nextUnpackStream++] = size; - sum = ArchiveException.addExact(sum, size); + totalUnpackSize = accumulate(totalUnpackSize, size, "unpackSize"); } } - if (sum > folder.getUnpackSize()) { + if (totalUnpackSize > folder.getUnpackSize()) { throw new ArchiveException("Sum of unpack sizes of folder exceeds total unpack size"); } - subStreamsInfo.unpackSizes[nextUnpackStream++] = folder.getUnpackSize() - sum; + subStreamsInfo.unpackSizes[nextUnpackStream++] = folder.getUnpackSize() - totalUnpackSize; } if (nid == NID.kSize) { nid = getUnsignedByte(header); @@ -1731,15 +1859,16 @@ private void readSubStreamsInfo(final ByteBuffer header, final Archive archive) int numDigests = 0; for (final Folder folder : archive.folders) { if (folder.numUnpackSubStreams != 1 || !folder.hasCrc) { - numDigests = ArchiveException.addExact(numDigests, folder.numUnpackSubStreams); + numDigests = accumulate(numDigests, folder.numUnpackSubStreams, "numDigests"); } } if (nid == NID.kCRC) { final BitSet hasMissingCrc = readAllOrBits(header, numDigests); - final long[] missingCrcs = new long[checkLongArray(numDigests)]; + ensureRemaining(header, UINT32_BYTES * hasMissingCrc.cardinality()); + final long[] missingCrcs = longArray(numDigests); for (int i = 0; i < numDigests; i++) { if (hasMissingCrc.get(i)) { - missingCrcs[i] = 0xffffFFFFL & getInt(header); + missingCrcs[i] = readUint32(header); } } int nextCrc = 0; @@ -1765,16 +1894,21 @@ private void readSubStreamsInfo(final ByteBuffer header, final Archive archive) private void readUnpackInfo(final ByteBuffer header, final Archive archive) throws IOException { int nid = getUnsignedByte(header); - final int numFoldersInt = readUint64ToIntExact(header); - final Folder[] folders = new Folder[checkObjectArray(numFoldersInt)]; - archive.folders = folders; + final int numFoldersInt = readUint64ToIntExact(header, "numFolders"); /* final int external = */ getUnsignedByte(header); + // Verify available header bytes and memory limit before allocating array + ensureRemaining(header, 3L * numFoldersInt); + // Assumes compressed pointer + MemoryLimitException.checkKiB(bytesToKiB(numFoldersInt * 4L), maxMemoryLimitKiB); + final Folder[] folders = new Folder[numFoldersInt]; + archive.folders = folders; for (int i = 0; i < numFoldersInt; i++) { folders[i] = readFolder(header); } nid = getUnsignedByte(header); for (final Folder folder : folders) { - folder.unpackSizes = new long[checkLongArray(toNonNegativeInt("totalOutputStreams", folder.totalOutputStreams))]; + ensureRemaining(header, folder.totalOutputStreams); + folder.unpackSizes = longArray(folder.totalOutputStreams); for (int i = 0; i < folder.totalOutputStreams; i++) { folder.unpackSizes[i] = readUint64(header); } @@ -1782,10 +1916,11 @@ private void readUnpackInfo(final ByteBuffer header, final Archive archive) thro nid = getUnsignedByte(header); if (nid == NID.kCRC) { final BitSet crcsDefined = readAllOrBits(header, numFoldersInt); + ensureRemaining(header, UINT32_BYTES * crcsDefined.cardinality()); for (int i = 0; i < numFoldersInt; i++) { if (crcsDefined.get(i)) { folders[i].hasCrc = true; - folders[i].crc = 0xffffFFFFL & getInt(header); + folders[i].crc = readUint32(header); } else { folders[i].hasCrc = false; } @@ -1841,27 +1976,23 @@ private ArchiveStatistics sanityCheckAndCollectStatistics(final ByteBuffer heade private void sanityCheckArchiveProperties(final ByteBuffer header) throws IOException { long nid = readUint64(header); while (nid != NID.kEnd) { - final int propertySize = toNonNegativeInt("propertySize", readUint64(header)); - if (skipBytesFully(header, propertySize) < propertySize) { - throw new ArchiveException("Invalid property size"); - } + // We validate the size but ignore the value + final int propertySize = readFieldSize(header); + skipBytesFully(header, propertySize); nid = readUint64(header); } } private void sanityCheckFilesInfo(final ByteBuffer header, final ArchiveStatistics stats) throws IOException { - stats.numberOfEntries = toNonNegativeInt("numFiles", readUint64(header)); + stats.numberOfEntries = readUint64ToIntExact(header, "numFiles"); int emptyStreams = -1; final int originalLimit = header.limit(); while (true) { final int propertyType = getUnsignedByte(header); - if (propertyType == 0) { + if (propertyType == NID.kEnd) { break; } - final int size = readUint64ToIntExact(header); - if (size < 0 || size > header.remaining()) { - throw new ArchiveException("Corrupted 7z archive: property size %,d, but only %,d bytes available", size, header.remaining()); - } + final int size = readFieldSize(header); // Limit the buffer to the size of the property header.limit(header.position() + size); switch (propertyType) { @@ -1873,14 +2004,14 @@ private void sanityCheckFilesInfo(final ByteBuffer header, final ArchiveStatisti if (emptyStreams == -1) { throw new ArchiveException("Header format error: kEmptyStream must appear before kEmptyFile"); } - readBits(header, emptyStreams); + skipBytesFully(header, size); break; } case NID.kAnti: { if (emptyStreams == -1) { throw new ArchiveException("Header format error: kEmptyStream must appear before kAnti"); } - readBits(header, emptyStreams); + skipBytesFully(header, size); break; } case NID.kName: { @@ -1904,48 +2035,16 @@ private void sanityCheckFilesInfo(final ByteBuffer header, final ArchiveStatisti } break; } - case NID.kCTime: { - final int timesDefined = readAllOrBits(header, stats.numberOfEntries).cardinality(); - final int external = getUnsignedByte(header); - if (external != 0) { - throw new ArchiveException("Not implemented"); - } - if (skipBytesFully(header, 8 * timesDefined) < 8 * timesDefined) { - throw new ArchiveException("Invalid creation dates size"); - } - break; - } - case NID.kATime: { - final int timesDefined = readAllOrBits(header, stats.numberOfEntries).cardinality(); - final int external = getUnsignedByte(header); - if (external != 0) { - throw new ArchiveException("Not implemented"); - } - if (skipBytesFully(header, 8 * timesDefined) < 8 * timesDefined) { - throw new ArchiveException("Invalid access dates size"); - } - break; - } - case NID.kMTime: { - final int timesDefined = readAllOrBits(header, stats.numberOfEntries).cardinality(); - final int external = getUnsignedByte(header); - if (external != 0) { - throw new ArchiveException("Not implemented"); - } - if (skipBytesFully(header, 8 * timesDefined) < 8 * timesDefined) { - throw new ArchiveException("Invalid modification dates size"); - } - break; - } + case NID.kCTime: + case NID.kATime: + case NID.kMTime: case NID.kWinAttributes: { - final int attributesDefined = readAllOrBits(header, stats.numberOfEntries).cardinality(); + final int definedCount = readAllOrBits(header, stats.numberOfEntries).cardinality(); final int external = getUnsignedByte(header); if (external != 0) { throw new ArchiveException("Not implemented"); } - if (skipBytesFully(header, 4 * attributesDefined) < 4 * attributesDefined) { - throw new ArchiveException("Invalid windows attributes size"); - } + skipBytesFully(header, (propertyType == NID.kWinAttributes ? UINT32_BYTES : REAL_UINT64_BYTES) * definedCount); break; } case NID.kStartPos: { @@ -1954,19 +2053,19 @@ private void sanityCheckFilesInfo(final ByteBuffer header, final ArchiveStatisti case NID.kDummy: { // 7z 9.20 asserts the content is all zeros and ignores the property // Compress up to 1.8.1 would throw an exception, now we ignore it (see COMPRESS-287 - if (skipBytesFully(header, size) < size) { - throw new ArchiveException("Incomplete kDummy property"); - } + skipBytesFully(header, size); break; } default: { // Compress up to 1.8.1 would throw an exception, now we ignore it (see COMPRESS-287 - if (skipBytesFully(header, size) < size) { - throw new ArchiveException("Incomplete property of type " + propertyType); - } + skipBytesFully(header, size); break; } } + // We should have consumed all the bytes by now + if (header.remaining() > 0) { + throw new ArchiveException("Unsupported 7z archive: property 0x%02d has %d trailing bytes.", propertyType, header.remaining()); + } // Restore original limit header.limit(originalLimit); } @@ -1974,17 +2073,16 @@ private void sanityCheckFilesInfo(final ByteBuffer header, final ArchiveStatisti } private long sanityCheckFolder(final ByteBuffer header, final ArchiveStatistics stats) throws IOException { - final int numCoders = toNonNegativeInt("numCoders", readUint64(header)); - if (numCoders == 0) { - throw new ArchiveException("Folder without coders"); + final long numCoders = readUint64(header); + if (numCoders == 0 || numCoders > MAX_CODERS_PER_FOLDER) { + throw new ArchiveException("Unsupported 7z archive: %,d coders in folder.", numCoders); } - stats.numberOfCoders = ArchiveException.addExact(stats.numberOfCoders, numCoders); - long totalOutStreams = 0; - long totalInStreams = 0; + stats.numberOfCoders = accumulate(stats.numberOfCoders, numCoders, "numCoders"); + int totalInStreams = 0; for (int i = 0; i < numCoders; i++) { final int bits = getUnsignedByte(header); final int idSize = bits & 0xf; - get(header, new byte[idSize]); + skipBytesFully(header, idSize); final boolean isSimple = (bits & 0x10) == 0; final boolean hasAttributes = (bits & 0x20) != 0; final boolean moreAlternativeMethods = (bits & 0x80) != 0; @@ -1993,37 +2091,36 @@ private long sanityCheckFolder(final ByteBuffer header, final ArchiveStatistics } if (isSimple) { totalInStreams++; - totalOutStreams++; } else { - totalInStreams = ArchiveException.addExact(totalInStreams, toNonNegativeInt("numInStreams", readUint64(header))); - totalOutStreams = ArchiveException.addExact(totalOutStreams, toNonNegativeInt("numOutStreams", readUint64(header))); + final long numInStreams = readUint64(header); + if (numInStreams > MAX_CODER_STREAMS_PER_FOLDER) { + throw new ArchiveException("Unsupported 7z archive: %,d coder input streams in folder.", numInStreams); + } + if (readUint64(header) != 1) { + throw new ArchiveException("Unsupported 7z archive: %,d coder output streams in folder.", readUint64(header)); + } + totalInStreams += (int) numInStreams; } if (hasAttributes) { - final int propertiesSize = toNonNegativeInt("propertiesSize", readUint64(header)); - if (skipBytesFully(header, propertiesSize) < propertiesSize) { - throw new ArchiveException("Invalid propertiesSize in folder"); - } + final int propertiesSize = readFieldSize(header); + skipBytesFully(header, propertiesSize); } } - toNonNegativeInt("totalInStreams", totalInStreams); - toNonNegativeInt("totalOutStreams", totalOutStreams); - stats.numberOfOutStreams = ArchiveException.addExact(stats.numberOfOutStreams, totalOutStreams); - stats.numberOfInStreams = ArchiveException.addExact(stats.numberOfInStreams, totalInStreams); - if (totalOutStreams == 0) { - throw new ArchiveException("Total output streams can't be 0"); - } - final int numBindPairs = toNonNegativeInt("numBindPairs", totalOutStreams - 1); + final int totalOutStreams = (int) numCoders; + stats.numberOfOutStreams = accumulate(stats.numberOfOutStreams, numCoders, "numOutStreams"); + stats.numberOfInStreams = accumulate(stats.numberOfInStreams, totalInStreams, "numInStreams"); + final int numBindPairs = totalOutStreams - 1; if (totalInStreams < numBindPairs) { throw new ArchiveException("Total input streams can't be less than the number of bind pairs"); } - final BitSet inStreamsBound = new BitSet((int) totalInStreams); + final BitSet inStreamsBound = new BitSet(totalInStreams); for (int i = 0; i < numBindPairs; i++) { - final int inIndex = toNonNegativeInt("inIndex", readUint64(header)); + final int inIndex = readUint64ToIntExact(header, "inIndex"); if (totalInStreams <= inIndex) { throw new ArchiveException("inIndex is bigger than number of inStreams"); } inStreamsBound.set(inIndex); - final int outIndex = toNonNegativeInt("outIndex", readUint64(header)); + final int outIndex = readUint64ToIntExact(header, "outIndex"); if (totalOutStreams <= outIndex) { throw new ArchiveException("outIndex is bigger than number of outStreams"); } @@ -2035,7 +2132,7 @@ private long sanityCheckFolder(final ByteBuffer header, final ArchiveStatistics } } else { for (int i = 0; i < numPackedStreams; i++) { - final int packedStreamIndex = toNonNegativeInt("packedStreamIndex", readUint64(header)); + final int packedStreamIndex = readUint64ToIntExact(header, "packedStreamIndex"); if (packedStreamIndex >= totalInStreams) { throw new ArchiveException("packedStreamIndex is bigger than number of totalInStreams"); } @@ -2046,19 +2143,19 @@ private long sanityCheckFolder(final ByteBuffer header, final ArchiveStatistics private void sanityCheckPackInfo(final ByteBuffer header, final ArchiveStatistics stats) throws IOException { final long packPos = readUint64(header); - if (packPos < 0 || SIGNATURE_HEADER_SIZE + packPos > channel.size() || SIGNATURE_HEADER_SIZE + packPos < 0) { + if (packPos > channel.size() - SIGNATURE_HEADER_SIZE) { throw new ArchiveException("packPos (%,d) is out of range", packPos); } - final long numPackStreams = readUint64(header); - stats.numberOfPackedStreams = toNonNegativeInt("numPackStreams", numPackStreams); + stats.numberOfPackedStreams = readUint64ToIntExact(header, "numPackStreams"); int nid = getUnsignedByte(header); if (nid == NID.kSize) { long totalPackSizes = 0; + ensureRemaining(header, MIN_UINT64_BYTES * stats.numberOfPackedStreams); for (int i = 0; i < stats.numberOfPackedStreams; i++) { final long packSize = readUint64(header); - totalPackSizes = ArchiveException.addExact(totalPackSizes, packSize); - final long endOfPackStreams = SIGNATURE_HEADER_SIZE + packPos + totalPackSizes; - if (packSize < 0 || endOfPackStreams > channel.size() || endOfPackStreams < packPos) { + totalPackSizes = accumulate(totalPackSizes, packSize, "packSize"); + // We check the total pack size against the file size. + if (totalPackSizes > channel.size() - SIGNATURE_HEADER_SIZE - packPos) { throw new ArchiveException("packSize (%,d) is out of range", packSize); } } @@ -2066,9 +2163,7 @@ private void sanityCheckPackInfo(final ByteBuffer header, final ArchiveStatistic } if (nid == NID.kCRC) { final int crcsDefined = readAllOrBits(header, stats.numberOfPackedStreams).cardinality(); - if (skipBytesFully(header, 4 * crcsDefined) < 4 * crcsDefined) { - throw new ArchiveException("Invalid number of CRCs in PackInfo"); - } + skipBytesFully(header, 4L * crcsDefined); nid = getUnsignedByte(header); } if (nid != NID.kEnd) { @@ -2100,7 +2195,7 @@ private void sanityCheckSubStreamsInfo(final ByteBuffer header, final ArchiveSta final List<Integer> numUnpackSubStreamsPerFolder = new LinkedList<>(); if (nid == NID.kNumUnpackStream) { for (int i = 0; i < stats.numberOfFolders; i++) { - numUnpackSubStreamsPerFolder.add(toNonNegativeInt("numStreams", readUint64(header))); + numUnpackSubStreamsPerFolder.add(readUint64ToIntExact(header, "numStreams")); } stats.numberOfUnpackSubStreams = numUnpackSubStreamsPerFolder.stream().mapToLong(Integer::longValue).sum(); nid = getUnsignedByte(header); @@ -2114,10 +2209,7 @@ private void sanityCheckSubStreamsInfo(final ByteBuffer header, final ArchiveSta continue; } for (int i = 0; i < numUnpackSubStreams - 1; i++) { - final long size = readUint64(header); - if (size < 0) { - throw new ArchiveException("Negative unpackSize"); - } + readUint64(header); } } nid = getUnsignedByte(header); @@ -2129,16 +2221,13 @@ private void sanityCheckSubStreamsInfo(final ByteBuffer header, final ArchiveSta int folderIdx = 0; for (final int numUnpackSubStreams : numUnpackSubStreamsPerFolder) { if (numUnpackSubStreams != 1 || stats.folderHasCrc == null || !stats.folderHasCrc.get(folderIdx++)) { - numDigests = ArchiveException.addExact(numDigests, numUnpackSubStreams); + numDigests = accumulate(numDigests, numUnpackSubStreams, "numDigests"); } } } if (nid == NID.kCRC) { - toNonNegativeInt("numDigests", numDigests); final int missingCrcs = readAllOrBits(header, numDigests).cardinality(); - if (skipBytesFully(header, 4 * missingCrcs) < 4 * missingCrcs) { - throw new ArchiveException("Invalid number of missing CRCs in SubStreamInfo"); - } + skipBytesFully(header, UINT32_BYTES * missingCrcs); nid = getUnsignedByte(header); } if (nid != NID.kEnd) { @@ -2151,8 +2240,7 @@ private void sanityCheckUnpackInfo(final ByteBuffer header, final ArchiveStatist if (nid != NID.kFolder) { throw new ArchiveException("Expected NID.kFolder, got %s", nid); } - final long numFolders = readUint64(header); - stats.numberOfFolders = toNonNegativeInt("numFolders", numFolders); + stats.numberOfFolders = readUint64ToIntExact(header, "numFolders"); final int external = getUnsignedByte(header); if (external != 0) { throw new ArchiveException("External unsupported"); @@ -2172,19 +2260,14 @@ private void sanityCheckUnpackInfo(final ByteBuffer header, final ArchiveStatist } for (final long numberOfOutputStreams : numberOfOutputStreamsPerFolder) { for (long i = 0; i < numberOfOutputStreams; i++) { - final long unpackSize = readUint64(header); - if (unpackSize < 0) { - throw new IllegalArgumentException("Negative unpackSize"); - } + readUint64(header); } } nid = getUnsignedByte(header); if (nid == NID.kCRC) { stats.folderHasCrc = readAllOrBits(header, stats.numberOfFolders); final int crcsDefined = stats.folderHasCrc.cardinality(); - if (skipBytesFully(header, 4 * crcsDefined) < 4 * crcsDefined) { - throw new ArchiveException("Invalid number of CRCs in UnpackInfo"); - } + skipBytesFully(header, UINT32_BYTES * crcsDefined); nid = getUnsignedByte(header); } if (nid != NID.kEnd) { @@ -2273,6 +2356,22 @@ public IOStream<? extends SevenZArchiveEntry> stream() { return IOStream.of(archive.files); } + /** + * Converts the given ByteBuffer to a byte array of the given size. + * + * @param header The buffer containing the 7z header data. + * @param size The size of the byte array to create. + * @return A byte array containing the data from the buffer. + * @throws IOException if there are insufficient resources to allocate the array or insufficient data in the buffer. + */ + private byte[] toByteArray(final ByteBuffer header, final int size) throws IOException { + // Check if we have enough resources to allocate the array + MemoryLimitException.checkKiB(bytesToKiB(size * Byte.BYTES), maxMemoryLimitKiB); + final byte[] result = new byte[size]; + get(header, result); + return result; + } + @Override public String toString() { return archive.toString(); @@ -2305,8 +2404,9 @@ private Archive tryToLocateEndHeader(final byte[] password) throws IOException { try { // Try to initialize Archive structure from here final long nextHeaderOffset = pos - previousDataSize; + // Smaller than 1 MiB, so fits in an int final long nextHeaderSize = channel.size() - pos; - final StartHeader startHeader = new StartHeader(nextHeaderOffset, nextHeaderSize, 0); + final StartHeader startHeader = new StartHeader(nextHeaderOffset, (int) nextHeaderSize, 0); final Archive result = initializeArchive(startHeader, password, false); // Sanity check: There must be some data... if (result.packSizes.length > 0 && result.files.length > 0) { diff --git a/src/main/java/org/apache/commons/compress/archivers/sevenz/StartHeader.java b/src/main/java/org/apache/commons/compress/archivers/sevenz/StartHeader.java index a1821dd83..bf7212fe1 100644 --- a/src/main/java/org/apache/commons/compress/archivers/sevenz/StartHeader.java +++ b/src/main/java/org/apache/commons/compress/archivers/sevenz/StartHeader.java @@ -21,10 +21,14 @@ final class StartHeader { final long nextHeaderOffset; - final long nextHeaderSize; + final int nextHeaderSize; final long nextHeaderCrc; - StartHeader(final long nextHeaderOffset, final long nextHeaderSize, final long nextHeaderCrc) { + StartHeader(final long nextHeaderOffset, final int nextHeaderSize, final long nextHeaderCrc) { + // The interval [SIGNATURE_HEADER_SIZE + nextHeaderOffset, SIGNATURE_HEADER_SIZE + nextHeaderOffset + nextHeaderSize) + // must be a valid range of the file, in particular must be within [0, Long.MAX_VALUE). + assert nextHeaderOffset >= 0 && nextHeaderOffset <= Long.MAX_VALUE - SevenZFile.SIGNATURE_HEADER_SIZE; + assert nextHeaderSize >= 0 && nextHeaderSize <= Long.MAX_VALUE - SevenZFile.SIGNATURE_HEADER_SIZE - nextHeaderOffset; this.nextHeaderOffset = nextHeaderOffset; this.nextHeaderSize = nextHeaderSize; this.nextHeaderCrc = nextHeaderCrc; diff --git a/src/main/java/org/apache/commons/compress/archivers/sevenz/SubStreamsInfo.java b/src/main/java/org/apache/commons/compress/archivers/sevenz/SubStreamsInfo.java index 3b5ece3e7..4272bf76f 100644 --- a/src/main/java/org/apache/commons/compress/archivers/sevenz/SubStreamsInfo.java +++ b/src/main/java/org/apache/commons/compress/archivers/sevenz/SubStreamsInfo.java @@ -22,6 +22,7 @@ import java.util.BitSet; import org.apache.commons.compress.CompressException; +import org.apache.commons.compress.MemoryLimitException; /** * Properties for non-empty files. @@ -41,24 +42,21 @@ final class SubStreamsInfo { */ final long[] crcs; - SubStreamsInfo(final long totalUnpackStreams, final int maxMemoryLimitKiB) throws CompressException { - final int intExactCount = Math.toIntExact(totalUnpackStreams); - int alloc; + SubStreamsInfo(final int totalUnpackStreams, final int maxMemoryLimitKiB) throws CompressException { + long alloc; try { // 2 long arrays, just count the longs - alloc = Math.multiplyExact(intExactCount, Long.BYTES * 2); + alloc = Math.multiplyExact(totalUnpackStreams, Long.BYTES * 2); // one BitSet [boolean, long[], int]. just count the long array - final int sizeOfBitSet = Math.multiplyExact(Long.BYTES, (intExactCount - 1 >> 6) + 1); - alloc = Math.addExact(alloc, Math.multiplyExact(intExactCount, sizeOfBitSet)); + final int sizeOfBitSet = Math.multiplyExact(Long.BYTES, (totalUnpackStreams - 1 >> 6) + 1); + alloc = Math.addExact(alloc, Math.multiplyExact(totalUnpackStreams, sizeOfBitSet)); } catch (final ArithmeticException e) { throw new CompressException("Cannot create allocation request for a SubStreamsInfo of totalUnpackStreams %,d, maxMemoryLimitKiB %,d: %s", totalUnpackStreams, maxMemoryLimitKiB, e); } - // Avoid false positives. - // Not a reliable check in old VMs or in low memory VMs. - // MemoryLimitException.checkKiB(SevenZFile.bytesToKiB(alloc), maxMemoryLimitKiB); - this.hasCrc = new BitSet(intExactCount); - this.crcs = new long[intExactCount]; - this.unpackSizes = new long[intExactCount]; + MemoryLimitException.checkKiB(SevenZFile.bytesToKiB(alloc), maxMemoryLimitKiB); + this.hasCrc = new BitSet(totalUnpackStreams); + this.crcs = new long[totalUnpackStreams]; + this.unpackSizes = new long[totalUnpackStreams]; } } diff --git a/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java b/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java index 772a2101f..0c338c86e 100644 --- a/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java @@ -29,7 +29,9 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.io.DataInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -66,6 +68,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; class SevenZFileTest extends AbstractArchiveFileTest<SevenZArchiveEntry> { @@ -124,6 +127,63 @@ static Stream<Consumer<ByteBuffer>> testReadFolder_Unsupported() { ); } + static Stream<byte[]> testReadRealUint64_Invalid() { + final byte m = (byte) 0xff; + return Stream.of( + new byte[] { (byte) 0b11111111, 0, 0, 0, 0, 0, 0, (byte) 0x80 }, + new byte[] { (byte) 0b11111111, m, m, m, m, m, m, m } + ); + } + + static Stream<Arguments> testReadRealUint64_Valid() { + final byte m = (byte) 0xff; + return Stream.of( + Arguments.of(new byte[] { 0, 1, 2, 3, 4, 5, 6, 7 }, 0x0706_0504_0302_0100L), + Arguments.of(new byte[] { m, m, m, m, m, m, m, Byte.MAX_VALUE }, 0x7FFF_FFFF_FFFF_FFFFL) + ); + } + + static Stream<Arguments> testReadUint32_Valid() { + final byte m = (byte) 0xff; + return Stream.of( + Arguments.of(new byte[] { 0, 1, 2, 3 }, 0x0302_0100L), + Arguments.of(new byte[] { m, m, m, Byte.MAX_VALUE }, 0x7FFF_FFFFL), + Arguments.of(new byte[] { m, m, m, m }, 0xFFFF_FFFFL) + ); + } + + static Stream<byte[]> testReadUint64_Overflow() { + final byte m = (byte) 0xff; + return Stream.of( + new byte[] { (byte) 0b11111111, 0, 0, 0, 0, 0, 0, 0, (byte) 0x80 }, + new byte[] { (byte) 0b11111111, m, m, m, m, m, m, m, m } + ); + } + + static Stream<Arguments> testReadUint64_Valid() { + final byte m = (byte) 0xff; + return Stream.of( + Arguments.of(new byte[] { 0 }, 0L), + Arguments.of(new byte[] { Byte.MAX_VALUE }, 0x7FL), + Arguments.of(new byte[] { (byte) 0b10_000001, 2 }, 0x0102L), + Arguments.of(new byte[] { (byte) 0b10_111111, m }, 0x3FFFL), + Arguments.of(new byte[] { (byte) 0b110_00001, 3, 2 }, 0x01_0203L), + Arguments.of(new byte[] { (byte) 0b110_11111, m, m }, 0x1F_FFFFL), + Arguments.of(new byte[] { (byte) 0b1110_0001, 4, 3, 2 }, 0x0102_0304L), + Arguments.of(new byte[] { (byte) 0b1110_1111, m, m, m }, 0x0FFF_FFFFL), + Arguments.of(new byte[] { (byte) 0b11110_001, 5, 4, 3, 2 }, 0x01_0203_0405L), + Arguments.of(new byte[] { (byte) 0b11110_111, m, m, m, m }, 0x07_FFFF_FFFFL), + Arguments.of(new byte[] { (byte) 0b111110_01, 6, 5, 4, 3, 2 }, 0x0102_0304_0506L), + Arguments.of(new byte[] { (byte) 0b111110_11, m, m, m, m, m }, 0x03FF_FFFF_FFFFL), + Arguments.of(new byte[] { (byte) 0b1111110_1, 7, 6, 5, 4, 3, 2 }, 0x01_0203_0405_0607L), + Arguments.of(new byte[] { (byte) 0b1111110_1, m, m, m, m, m, m }, 0x01_FFFF_FFFF_FFFFL), + Arguments.of(new byte[] { (byte) 0b11111110, 7, 6, 5, 4, 3, 2, 1 }, 0x01_0203_0405_0607L), + Arguments.of(new byte[] { (byte) 0b11111110, m, m, m, m, m, m, m }, 0xFF_FFFF_FFFF_FFFFL), + Arguments.of(new byte[] { (byte) 0b11111111, 8, 7, 6, 5, 4, 3, 2, 1 }, 0x0102_0304_0506_0708L), + Arguments.of(new byte[] { (byte) 0b11111111, m, m, m, m, m, m, m, Byte.MAX_VALUE }, 0x7FFF_FFFF_FFFF_FFFFL) + ); + } + private static void writeBindPair(final ByteBuffer buffer, final long inIndex, final long outIndex) { writeUint64(buffer, inIndex); writeUint64(buffer, outIndex); @@ -1028,6 +1088,23 @@ void testReadingBackLZMA2DictSize() throws Exception { } } + @ParameterizedTest + @MethodSource + void testReadRealUint64_Invalid(final byte[] input) throws IOException { + try (DataInputStream dis = new DataInputStream(new ByteArrayInputStream(input))) { + assertThrows(IOException.class, () -> SevenZFile.readRealUint64(dis)); + } + } + + @ParameterizedTest + @MethodSource + void testReadRealUint64_Valid(final byte[] input, final long expected) throws IOException { + try (DataInputStream dis = new DataInputStream(new ByteArrayInputStream(input))) { + final long actual = SevenZFile.readRealUint64(dis); + assertEquals(expected, actual); + } + } + @Test void testReadTimesFromFile() throws IOException { try (SevenZFile sevenZFile = getSevenZFile("times.7z")) { @@ -1054,6 +1131,33 @@ void testReadTimesFromFile() throws IOException { } } + @ParameterizedTest + @MethodSource + void testReadUint32_Valid(final byte[] input, final long expected) throws IOException { + try (DataInputStream dis = new DataInputStream(new ByteArrayInputStream(input))) { + final long actual = SevenZFile.readUint32(dis); + assertEquals(expected, actual); + } + final ByteBuffer buf = ByteBuffer.wrap(input).order(ByteOrder.LITTLE_ENDIAN); + final long actual = SevenZFile.readUint32(buf); + assertEquals(expected, actual); + } + + @ParameterizedTest + @MethodSource + void testReadUint64_Overflow(final byte[] bytes) { + final ByteBuffer buf = ByteBuffer.wrap(bytes); + final ArchiveException ex = assertThrows(ArchiveException.class, () -> SevenZFile.readUint64(buf)); + assertTrue(ex.getMessage().contains("Unsupported 7-Zip archive")); + } + + @ParameterizedTest + @MethodSource + void testReadUint64_Valid(final byte[] bytes, final long expected) throws IOException { + final ByteBuffer buf = ByteBuffer.wrap(bytes); + assertEquals(expected, SevenZFile.readUint64(buf)); + } + @Test void testRemainingBytesUnchangedAfterRead() throws Exception { try (SevenZFile sevenZFile = getSevenZFile("COMPRESS-256.7z")) {
