PackReverseIndex: verify checksums

The new version 1 file-based reverse index has a footer with the
checksum of the corresponding pack file and a checksum of its own
contents. The initial implementation doesn't enforce that the pack
checksum matches the checksum found in the forward index nor that the
self checksum matches the contents of the file just read in.

Offer a method for reverse index users to verify the checksums in a way
appropriate to the version being used. For the pre-existing computed
version, always succeed since it is not based on a file so there is no
possibility of corruption.

Check for corruption of the file itself during parsing the checksum
footer, by comparing the self checksum with the digest of the file
contents read.

Change-Id: I87ff3933cf1afa76663350400b616695e4966cb6
Signed-off-by: Anna Papitto <annapapitto@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputedTest.java

@@ -17,6 +17,7 @@
 import static org.junit.Assert.fail;
 
 import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.errors.PackMismatchException;
 import org.eclipse.jgit.internal.storage.file.PackIndex.MutableEntry;
 import org.eclipse.jgit.junit.JGitTestUtil;
 import org.eclipse.jgit.junit.RepositoryTestCase;
@@ -92,6 +93,12 @@ public void testFindNextOffsetWrongOffset() {
 		}
 	}
 
+	@Test
+	public void testVerifyChecksum() throws PackMismatchException {
+		// ComputedReverseIndex doesn't have a file containing a checksum.
+		reverseIdx.verifyPackChecksum(null);
+	}
+
 	private long findFirstOffset() {
 		long min = Long.MAX_VALUE;
 		for (MutableEntry me : idx)

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1Test.java

@@ -15,11 +15,14 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 
 import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.errors.PackMismatchException;
 import org.eclipse.jgit.junit.JGitTestUtil;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.transport.PackedObjectInfo;
@@ -146,6 +149,26 @@ public void read_objectCountTooLarge() {
 						() -> null));
 	}
 
+	@Test
+	public void read_incorrectChecksum() {
+		byte[] badChecksum = new byte[] { 'R', 'I', 'D', 'X', // magic
+				0x00, 0x00, 0x00, 0x01, // file version
+				0x00, 0x00, 0x00, 0x01, // oid version
+				// pack checksum to copy into at byte 12
+				0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+				// checksum
+				(byte) 0xf2, 0x1a, 0x1a, (byte) 0xaa, 0x32, 0x2d, (byte) 0xb9,
+				(byte) 0xfd, 0x0f, (byte) 0xa5, 0x4c, (byte) 0xea, (byte) 0xcf,
+				(byte) 0xbb, (byte) 0x99, (byte) 0xde, (byte) 0xd3, 0x4e,
+				(byte) 0xb1, (byte) 0xee, // would be 0x74 if correct
+		};
+		System.arraycopy(FAKE_PACK_CHECKSUM, 0, badChecksum, 12,
+				FAKE_PACK_CHECKSUM.length);
+		ByteArrayInputStream in = new ByteArrayInputStream(badChecksum);
+		assertThrows(CorruptObjectException.class,
+				() -> PackReverseIndexFactory.readFromFile(in, 0, () -> null));
+	}
+
 	@Test
 	public void findObject_noObjects() {
 		assertNull(emptyReverseIndex.findObject(0));
@@ -235,6 +258,26 @@ public void findObjectByPosition_badOffset() {
 				() -> smallReverseIndex.findObjectByPosition(10));
 	}
 
+	@Test
+	public void verifyChecksum_match() throws IOException {
+		smallReverseIndex.verifyPackChecksum("smallPackFilePath");
+	}
+
+	@Test
+	public void verifyChecksum_mismatch() throws IOException {
+		ByteArrayInputStream in = new ByteArrayInputStream(NO_OBJECTS);
+		PackIndex mockForwardIndex = mock(PackIndex.class);
+		when(mockForwardIndex.getChecksum()).thenReturn(
+				new byte[] { 'D', 'I', 'F', 'F', 'P', 'A', 'C', 'K', 'C', 'H',
+						'E', 'C', 'K', 'S', 'U', 'M', '7', '8', '9', '0', });
+		PackReverseIndex reverseIndex = PackReverseIndexFactory.readFromFile(in,
+				0,
+				() -> mockForwardIndex);
+
+		assertThrows(PackMismatchException.class,
+				() -> reverseIndex.verifyPackChecksum("packFilePath"));
+	}
+
 	private static PackedObjectInfo objectInfo(String objectId, int type,
 			long offset) {
 		PackedObjectInfo objectInfo = new PackedObjectInfo(

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -223,6 +223,7 @@ corruptObjectTruncatedInMode=truncated in mode
 corruptObjectTruncatedInName=truncated in name
 corruptObjectTruncatedInObjectId=truncated in object id
 corruptObjectZeroId=entry points to null SHA-1
+corruptReverseIndexChecksumIncorrect=Reverse index checksum incorrect: written as {0} but digest was {1}
 corruptUseCnt=close() called when useCnt is already zero for {0}
 couldNotGetAdvertisedRef=Remote {0} did not advertise Ref for branch {1}. This Ref may not exist in the remote or may be hidden by permission settings.
 couldNotGetRepoStatistics=Could not get repository statistics
@@ -566,7 +567,7 @@ openingConnection=Opening connection
 operationCanceled=Operation {0} was canceled
 outputHasAlreadyBeenStarted=Output has already been started.
 overflowedReftableBlock=Overflowed reftable block
-packChecksumMismatch=Pack checksum mismatch detected for pack file {0}: .pack has {1} whilst .idx has {2}
+packChecksumMismatch=Pack checksum mismatch detected for pack file {0}: {1} has {2} whilst {3} has {4}
 packCorruptedWhileWritingToFilesystem=Pack corrupted while writing to filesystem
 packedRefsHandleIsStale=packed-refs handle is stale, {0}. retry
 packetSizeMustBeAtLeast=packet size {0} must be >= {1}

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -252,6 +252,7 @@ public static JGitText get() {
 	/***/ public String corruptObjectTruncatedInName;
 	/***/ public String corruptObjectTruncatedInObjectId;
 	/***/ public String corruptObjectZeroId;
+	/***/ public String corruptReverseIndexChecksumIncorrect;
 	/***/ public String corruptPack;
 	/***/ public String corruptUseCnt;
 	/***/ public String couldNotFindTabInLine;

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java

@@ -50,12 +50,14 @@
 import org.eclipse.jgit.errors.UnsupportedPackVersionException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.pack.BinaryDelta;
+import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.internal.storage.pack.PackOutputStream;
 import org.eclipse.jgit.lib.AbbreviatedObjectId;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.util.Hex;
 import org.eclipse.jgit.util.LongList;
 import org.eclipse.jgit.util.NB;
 import org.eclipse.jgit.util.RawParseUtils;
@@ -177,10 +179,10 @@ private PackIndex idx() throws IOException {
 							throw new PackMismatchException(MessageFormat
 									.format(JGitText.get().packChecksumMismatch,
 											packFile.getPath(),
-											ObjectId.fromRaw(packChecksum)
-													.name(),
-											ObjectId.fromRaw(idx.packChecksum)
-													.name()));
+											PackExt.PACK.getExtension(),
+											Hex.toHexString(packChecksum),
+											PackExt.INDEX.getExtension(),
+											Hex.toHexString(idx.packChecksum)));
 						}
 						loadedIdx = idx;
 					} catch (InterruptedIOException e) {
@@ -765,11 +767,11 @@ private void onOpenPack() throws IOException {
 		fd.seek(length - 20);
 		fd.readFully(buf, 0, 20);
 		if (!Arrays.equals(buf, packChecksum)) {
-			throw new PackMismatchException(MessageFormat.format(
-					JGitText.get().packChecksumMismatch,
-					getPackFile(),
-					ObjectId.fromRaw(buf).name(),
-					ObjectId.fromRaw(idx.packChecksum).name()));
+			throw new PackMismatchException(
+					MessageFormat.format(JGitText.get().packChecksumMismatch,
+							getPackFile(), PackExt.PACK.getExtension(),
+							Hex.toHexString(buf), PackExt.INDEX.getExtension(),
+							Hex.toHexString(idx.packChecksum)));
 		}
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndex.java

@@ -11,6 +11,7 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.errors.PackMismatchException;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -34,6 +35,17 @@ public interface PackReverseIndex {
 	 */
 	int VERSION_1 = 1;
 
+	/**
+	 * Verify that the pack checksum found in the reverse index matches that
+	 * from the pack file.
+	 *
+	 * @param packFilePath
+	 *            the path to display in event of a mismatch
+	 * @throws PackMismatchException
+	 *             if the checksums do not match
+	 */
+	void verifyPackChecksum(String packFilePath) throws PackMismatchException;
+
 	/**
 	 * Search for object id with the specified start offset in this pack
 	 * (reverse) index.

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputed.java

@@ -12,6 +12,7 @@
 import java.text.MessageFormat;
 
 import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.errors.PackMismatchException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.file.PackIndex.MutableEntry;
 import org.eclipse.jgit.lib.ObjectId;
@@ -143,6 +144,12 @@ final class PackReverseIndexComputed implements PackReverseIndex {
 		}
 	}
 
+	@Override
+	public void verifyPackChecksum(String packFilePath)
+			throws PackMismatchException {
+		// There is no file with a checksum.
+	}
+
 	@Override
 	public ObjectId findObject(long offset) {
 		final int ith = binarySearch(offset);

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1.java

@@ -16,10 +16,14 @@
 import java.io.UncheckedIOException;
 import java.security.DigestInputStream;
 import java.text.MessageFormat;
+import java.util.Arrays;
 
 import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.errors.PackMismatchException;
 import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.internal.storage.pack.PackExt;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.util.Hex;
 import org.eclipse.jgit.util.IO;
 
 /**
@@ -89,7 +93,20 @@ final class PackReverseIndexV1 implements PackReverseIndex {
 		parseChecksums();
 	}
 
-	private void parseBody() throws IOException {
+	@Override
+	public void verifyPackChecksum(String packFilePath)
+			throws PackMismatchException {
+		if (!Arrays.equals(packChecksum, getPackIndex().getChecksum())) {
+			throw new PackMismatchException(
+					MessageFormat.format(JGitText.get().packChecksumMismatch,
+							packFilePath, PackExt.INDEX.getExtension(),
+							Hex.toHexString(getPackIndex().getChecksum()),
+							PackExt.REVERSE_INDEX.getExtension(),
+							Hex.toHexString(packChecksum)));
+		}
+	}
+
+		private void parseBody() throws IOException {
 		for (int i = 0; i < objectCount; i++) {
 			indexPositionsSortedByOffset[i] = dataIn.readInt();
 		}
@@ -98,10 +115,19 @@ private void parseBody() throws IOException {
 	private void parseChecksums() throws IOException {
 		packChecksum = new byte[SHA1_BYTES];
 		IO.readFully(inputStream, packChecksum);
-		// TODO: verify checksum
+
+		// Take digest before reading the self checksum changes it.
+		byte[] observedSelfChecksum = inputStream.getMessageDigest().digest();
 
 		byte[] readSelfChecksum = new byte[SHA1_BYTES];
 		IO.readFully(inputStream, readSelfChecksum);
+
+		if (!Arrays.equals(readSelfChecksum, observedSelfChecksum)) {
+			throw new CorruptObjectException(MessageFormat.format(
+					JGitText.get().corruptReverseIndexChecksumIncorrect,
+					Hex.toHexString(readSelfChecksum),
+					Hex.toHexString(observedSelfChecksum)));
+		}
 	}
 
 	@Override