CVE-2018-12418: Loop with Unreachable Exit Condition ('Infinite Loop') (CWE-835)
value: Archive.java in Junrar before 1.0.1, as used in Apache Tika and other products, is affected by a denial of service vulnerability due to an infinite loop when handling corrupt RAR files.
CWE description: The product contains an iteration or loop with an exit condition that cannot be reached, i.e., an infinite loop.
CVSS v3 base score: 5.5
Published: 2018-06-14T16:29Z
Repository: https://github.com/junrar/junrar
Language: Java
Files changed: src/main/java/com/github/junrar/Archive.java, src/test/java/com/github/junrar/testUtil/SimpleTest.java
Commit date: 2018-05-30 23:22:23 -0300
Commit message: make Archive slightly more robust against corrupt files.  Add (#8)

unit test file from Apache Tika.
Diff: diff --git a/src/main/java/com/github/junrar/Archive.java b/src/main/java/com/github/junrar/Archive.java
index 94df25f..ec09f88 100644
--- a/src/main/java/com/github/junrar/Archive.java
+++ b/src/main/java/com/github/junrar/Archive.java
@@ -24,11 +24,13 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.PipedInputStream;
 import java.io.PipedOutputStream;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import com.github.junrar.exception.RarException;
 import com.github.junrar.exception.RarException.RarExceptionType;
@@ -60,10 +62,12 @@ import com.github.junrar.unpack.Unpack;
  * @version $LastChangedRevision$
  */
 public class Archive implements Closeable {
 	private static Logger logger = Logger.getLogger(Archive.class.getName());
 
+	private static int MAX_HEADER_SIZE = 20971520;//20MB
+
 	private IReadOnlyAccess rof;
 
 	private final UnrarCallback unrarCallback;
 
 	private final ComprDataIO dataIO;
@@ -227,15 +231,17 @@ public class Archive implements Closeable {
 		markHead = null;
 		newMhd = null;
 		headers.clear();
 		currentHeaderIndex = 0;
 		int toRead = 0;
-
+		//keep track of positions already processed for
+		//more robustness against corrupt files
+		Set<Long> processedPositions = new HashSet<Long>();
 		while (true) {
 			int size = 0;
 			long newpos = 0;
-			byte[] baseBlockBuffer = new byte[BaseBlock.BaseBlockSize];
+			byte[] baseBlockBuffer = safelyAllocate(BaseBlock.BaseBlockSize, MAX_HEADER_SIZE);
 
 			long position = rof.getPosition();
 
 			// Weird, but is trying to read beyond the end of the file
 			if (position >= fileLength) {
@@ -264,11 +270,11 @@ public class Archive implements Closeable {
 				break;
 
 			case MainHeader:
 				toRead = block.hasEncryptVersion() ? MainHeader.mainHeaderSizeWithEnc
 						: MainHeader.mainHeaderSize;
-				byte[] mainbuff = new byte[toRead];
+				byte[] mainbuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
 				rof.readFully(mainbuff, toRead);
 				MainHeader mainhead = new MainHeader(block, mainbuff);
 				headers.add(mainhead);
 				this.newMhd = mainhead;
 				if (newMhd.isEncrypted()) {
@@ -278,37 +284,41 @@ public class Archive implements Closeable {
 				// mainhead.print();
 				break;
 
 			case SignHeader:
 				toRead = SignHeader.signHeaderSize;
-				byte[] signBuff = new byte[toRead];
+				byte[] signBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
 				rof.readFully(signBuff, toRead);
 				SignHeader signHead = new SignHeader(block, signBuff);
 				headers.add(signHead);
 				// logger.info("HeaderType: SignHeader");
 
 				break;
 
 			case AvHeader:
 				toRead = AVHeader.avHeaderSize;
-				byte[] avBuff = new byte[toRead];
+				byte[] avBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
 				rof.readFully(avBuff, toRead);
 				AVHeader avHead = new AVHeader(block, avBuff);
 				headers.add(avHead);
 				// logger.info("headertype: AVHeader");
 				break;
 
 			case CommHeader:
 				toRead = CommentHeader.commentHeaderSize;
-				byte[] commBuff = new byte[toRead];
+				byte[] commBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
 				rof.readFully(commBuff, toRead);
 				CommentHeader commHead = new CommentHeader(block, commBuff);
 				headers.add(commHead);
 				// logger.info("method: "+commHead.getUnpMethod()+"; 0x"+
 				// Integer.toHexString(commHead.getUnpMethod()));
 				newpos = commHead.getPositionInFile()
 						+ commHead.getHeaderSize();
+				if (processedPositions.contains(newpos)) {
+					throw new RarException(RarExceptionType.badRarArchive);
+				}
+				processedPositions.add(newpos);
 				rof.setPosition(newpos);
 
 				break;
 			case EndArcHeader:
 
@@ -319,11 +329,11 @@ public class Archive implements Closeable {
 				if (block.hasVolumeNumber()) {
 					toRead += EndArcHeader.endArcVolumeNumberSize;
 				}
 				EndArcHeader endArcHead;
 				if (toRead > 0) {
-					byte[] endArchBuff = new byte[toRead];
+					byte[] endArchBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
 					rof.readFully(endArchBuff, toRead);
 					endArcHead = new EndArcHeader(block, endArchBuff);
 					// logger.info("HeaderType: endarch\ndatacrc:"+
 					// endArcHead.getArchiveDataCRC());
 				} else {
@@ -333,55 +343,62 @@ public class Archive implements Closeable {
 				headers.add(endArcHead);
 				// logger.info("\n--------end header--------");
 				return;
 
 			default:
-				byte[] blockHeaderBuffer = new byte[BlockHeader.blockHeaderSize];
+				byte[] blockHeaderBuffer = safelyAllocate(BlockHeader.blockHeaderSize, MAX_HEADER_SIZE);
 				rof.readFully(blockHeaderBuffer, BlockHeader.blockHeaderSize);
 				BlockHeader blockHead = new BlockHeader(block,
 						blockHeaderBuffer);
 
 				switch (blockHead.getHeaderType()) {
 				case NewSubHeader:
 				case FileHeader:
 					toRead = blockHead.getHeaderSize()
 							- BlockHeader.BaseBlockSize
 							- BlockHeader.blockHeaderSize;
-					byte[] fileHeaderBuffer = new byte[toRead];
+					byte[] fileHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
 					rof.readFully(fileHeaderBuffer, toRead);
 
 					FileHeader fh = new FileHeader(blockHead, fileHeaderBuffer);
 					headers.add(fh);
 					newpos = fh.getPositionInFile() + fh.getHeaderSize()
 							+ fh.getFullPackSize();
+					if (processedPositions.contains(newpos)) {
+						throw new RarException(RarExceptionType.badRarArchive);
+					}
+					processedPositions.add(newpos);
 					rof.setPosition(newpos);
 					break;
 
 				case ProtectHeader:
 					toRead = blockHead.getHeaderSize()
 							- BlockHeader.BaseBlockSize
 							- BlockHeader.blockHeaderSize;
-					byte[] protectHeaderBuffer = new byte[toRead];
+					byte[] protectHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
 					rof.readFully(protectHeaderBuffer, toRead);
 					ProtectHeader ph = new ProtectHeader(blockHead,
 							protectHeaderBuffer);
-
 					newpos = ph.getPositionInFile() + ph.getHeaderSize()
 							+ ph.getDataSize();
+					if (processedPositions.contains(newpos)) {
+						throw new RarException(RarExceptionType.badRarArchive);
+					}
+					processedPositions.add(newpos);
 					rof.setPosition(newpos);
 					break;
 
 				case SubHeader: {
-					byte[] subHeadbuffer = new byte[SubBlockHeader.SubBlockHeaderSize];
+					byte[] subHeadbuffer = safelyAllocate(SubBlockHeader.SubBlockHeaderSize, MAX_HEADER_SIZE);
 					rof.readFully(subHeadbuffer,
 							SubBlockHeader.SubBlockHeaderSize);
 					SubBlockHeader subHead = new SubBlockHeader(blockHead,
 							subHeadbuffer);
 					subHead.print();
 					switch (subHead.getSubType()) {
 					case MAC_HEAD: {
-						byte[] macHeaderbuffer = new byte[MacInfoHeader.MacInfoHeaderSize];
+						byte[] macHeaderbuffer = safelyAllocate(MacInfoHeader.MacInfoHeaderSize, MAX_HEADER_SIZE);
 						rof.readFully(macHeaderbuffer,
 								MacInfoHeader.MacInfoHeaderSize);
 						MacInfoHeader macHeader = new MacInfoHeader(subHead,
 								macHeaderbuffer);
 						macHeader.print();
@@ -391,11 +408,11 @@ public class Archive implements Closeable {
 					}
 					// TODO implement other subheaders
 					case BEEA_HEAD:
 						break;
 					case EA_HEAD: {
-						byte[] eaHeaderBuffer = new byte[EAHeader.EAHeaderSize];
+						byte[] eaHeaderBuffer = safelyAllocate(EAHeader.EAHeaderSize, MAX_HEADER_SIZE);
 						rof.readFully(eaHeaderBuffer, EAHeader.EAHeaderSize);
 						EAHeader eaHeader = new EAHeader(subHead,
 								eaHeaderBuffer);
 						eaHeader.print();
 						headers.add(eaHeader);
@@ -409,11 +426,11 @@ public class Archive implements Closeable {
 					case UO_HEAD:
 						toRead = subHead.getHeaderSize();
 						toRead -= BaseBlock.BaseBlockSize;
 						toRead -= BlockHeader.blockHeaderSize;
 						toRead -= SubBlockHeader.SubBlockHeaderSize;
-						byte[] uoHeaderBuffer = new byte[toRead];
+						byte[] uoHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
 						rof.readFully(uoHeaderBuffer, toRead);
 						UnixOwnersHeader uoHeader = new UnixOwnersHeader(
 								subHead, uoHeaderBuffer);
 						uoHeader.print();
 						headers.add(uoHeader);
@@ -432,10 +449,20 @@ public class Archive implements Closeable {
 			}
 			// logger.info("\n--------end header--------");
 		}
 	}
 
+	private static byte[] safelyAllocate(long len, int maxSize) throws RarException {
+		if (maxSize < 0) {
+			throw new IllegalArgumentException("maxsize must be >= 0");
+		}
+		if (len < 0 || len > (long)maxSize) {
+			throw new RarException(RarExceptionType.badRarArchive);
+		}
+		return new byte[(int)len];
+	}
+
 	/**
 	 * Extract the file specified by the given header and write it to the
 	 * supplied output stream
 	 * 
 	 * @param hd
diff --git a/src/test/java/com/github/junrar/testUtil/SimpleTest.java b/src/test/java/com/github/junrar/testUtil/SimpleTest.java
new file mode 100644
index 0000000..24b2cf9
--- /dev/null
+++ b/src/test/java/com/github/junrar/testUtil/SimpleTest.java
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.github.junrar.testUtil;
+
+import com.github.junrar.Archive;
+import com.github.junrar.rarfile.FileHeader;
+import org.junit.Test;
+
+import java.io.File;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class SimpleTest {
+
+    @Test
+    public void testTikaDocs() throws Exception {
+        String[] expected = {"testEXCEL.xls", "13824",
+                "testHTML.html", "167",
+                "testOpenOffice2.odt", "26448",
+                "testPDF.pdf", "34824",
+                "testPPT.ppt", "16384",
+                "testRTF.rtf", "3410",
+                "testTXT.txt", "49",
+                "testWORD.doc", "19456",
+                "testXML.xml", "766"};
+
+
+        File f = new File(getClass().getResource("test-documents.rar").toURI());
+        Archive archive = null;
+        try {
+            archive = new Archive(f);
+
+            FileHeader fileHeader = archive.nextFileHeader();
+            int i = 0;
+            while (fileHeader != null) {
+                assertTrue(fileHeader.getFileNameString().contains(expected[i++]));
+                assertEquals(Long.parseLong(expected[i++]), fileHeader.getUnpSize());
+                fileHeader = archive.nextFileHeader();
+            }
+        } finally {
+            archive.close();
+        }
+
+    }
+}
diff --git a/src/test/resources/com/github/junrar/testUtil/test-documents.rar b/src/test/resources/com/github/junrar/testUtil/test-documents.rar
new file mode 100644
index 0000000..63a4749
Binary files /dev/null and b/src/test/resources/com/github/junrar/testUtil/test-documents.rar differ

Vulnerable code: 
			byte[] baseBlockBuffer = new byte[BaseBlock.BaseBlockSize];
				byte[] mainbuff = new byte[toRead];
				byte[] signBuff = new byte[toRead];
				byte[] avBuff = new byte[toRead];
				byte[] commBuff = new byte[toRead];
					byte[] endArchBuff = new byte[toRead];
				byte[] blockHeaderBuffer = new byte[BlockHeader.blockHeaderSize];
					byte[] fileHeaderBuffer = new byte[toRead];
					byte[] protectHeaderBuffer = new byte[toRead];

					byte[] subHeadbuffer = new byte[SubBlockHeader.SubBlockHeaderSize];
						byte[] macHeaderbuffer = new byte[MacInfoHeader.MacInfoHeaderSize];
						byte[] eaHeaderBuffer = new byte[EAHeader.EAHeaderSize];
						byte[] uoHeaderBuffer = new byte[toRead];
Fixed code: import java.util.HashSet;
import java.util.Set;
	private static int MAX_HEADER_SIZE = 20971520;//20MB

		//keep track of positions already processed for
		//more robustness against corrupt files
		Set<Long> processedPositions = new HashSet<Long>();
			byte[] baseBlockBuffer = safelyAllocate(BaseBlock.BaseBlockSize, MAX_HEADER_SIZE);
				byte[] mainbuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
				byte[] signBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
				byte[] avBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
				byte[] commBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
				if (processedPositions.contains(newpos)) {
					throw new RarException(RarExceptionType.badRarArchive);
				}
				processedPositions.add(newpos);
					byte[] endArchBuff = safelyAllocate(toRead, MAX_HEADER_SIZE);
				byte[] blockHeaderBuffer = safelyAllocate(BlockHeader.blockHeaderSize, MAX_HEADER_SIZE);
					byte[] fileHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
					if (processedPositions.contains(newpos)) {
						throw new RarException(RarExceptionType.badRarArchive);
					}
					processedPositions.add(newpos);
					byte[] protectHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
					if (processedPositions.contains(newpos)) {
						throw new RarException(RarExceptionType.badRarArchive);
					}
					processedPositions.add(newpos);
					byte[] subHeadbuffer = safelyAllocate(SubBlockHeader.SubBlockHeaderSize, MAX_HEADER_SIZE);
						byte[] macHeaderbuffer = safelyAllocate(MacInfoHeader.MacInfoHeaderSize, MAX_HEADER_SIZE);
						byte[] eaHeaderBuffer = safelyAllocate(EAHeader.EAHeaderSize, MAX_HEADER_SIZE);
						byte[] uoHeaderBuffer = safelyAllocate(toRead, MAX_HEADER_SIZE);
	private static byte[] safelyAllocate(long len, int maxSize) throws RarException {
		if (maxSize < 0) {
			throw new IllegalArgumentException("maxsize must be >= 0");
		}
		if (len < 0 || len > (long)maxSize) {
			throw new RarException(RarExceptionType.badRarArchive);
		}
		return new byte[(int)len];
	}

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.
 * The ASF licenses this file to You under the Apache License, Version 2.0
 * (the "License"); you may not use this file except in compliance with
 * the License.  You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package com.github.junrar.testUtil;

import com.github.junrar.Archive;
import com.github.junrar.rarfile.FileHeader;
import org.junit.Test;

import java.io.File;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class SimpleTest {

    @Test
    public void testTikaDocs() throws Exception {
        String[] expected = {"testEXCEL.xls", "13824",
                "testHTML.html", "167",
                "testOpenOffice2.odt", "26448",
                "testPDF.pdf", "34824",
                "testPPT.ppt", "16384",
                "testRTF.rtf", "3410",
                "testTXT.txt", "49",
                "testWORD.doc", "19456",
                "testXML.xml", "766"};


        File f = new File(getClass().getResource("test-documents.rar").toURI());
        Archive archive = null;
        try {
            archive = new Archive(f);

            FileHeader fileHeader = archive.nextFileHeader();
            int i = 0;
            while (fileHeader != null) {
                assertTrue(fileHeader.getFileNameString().contains(expected[i++]));
                assertEquals(Long.parseLong(expected[i++]), fileHeader.getUnpSize());
                fileHeader = archive.nextFileHeader();
            }
        } finally {
            archive.close();
        }

    }
}