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>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputedTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputedTest.java
index 4facd6e0079ac50917a784e07f8deb2ae9451924..ea5aaf5dd4769731adeb86f06d8815f99b0062c5 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputedTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputedTest.java
@@ -17,6 +17,7 @@ import static org.junit.Assert.assertTrue;
 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 class PackReverseIndexComputedTest extends RepositoryTestCase {
                }
        }
 
+       @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)

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1Test.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1Test.java
index b4849f99f458fbefb4b0dc69b8ffd2f47173fb9d..38b28b501bd453af37f1e2d5cb4e57b449ada161 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1Test.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1Test.java
@@ -15,11 +15,14 @@ import static org.eclipse.jgit.lib.Constants.OBJ_TREE;
 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 class PackReverseIndexV1Test {
                                                () -> 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 class PackReverseIndexV1Test {
                                () -> 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(

diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index bf8a1ef5dbcb89bdad91835e2324f03276b791e5..c73d85f078add2727be52f7add609d0b8623d0b6 100644 (file)
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/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}

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index a032d24c7172c8f509374d8009cc745304594557..91d53220a99b67aff9e840030537ff2148ffeeb8 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -252,6 +252,7 @@ public class JGitText extends TranslationBundle {
        /***/ public String corruptObjectTruncatedInName;
        /***/ public String corruptObjectTruncatedInObjectId;
        /***/ public String corruptObjectZeroId;
+       /***/ public String corruptReverseIndexChecksumIncorrect;
        /***/ public String corruptPack;
        /***/ public String corruptUseCnt;
        /***/ public String couldNotFindTabInLine;

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
index 782cbea05405e0cb4d18f74ea0aa7c1805b4d78d..8d9fe23ae567116c39d3bff157df09a503f38a83 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
@@ -50,12 +50,14 @@ import org.eclipse.jgit.errors.UnsupportedPackIndexVersionException;
 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 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
                                                        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 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
                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)));
                }
        }
 

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndex.java
index 74ea89025a983fe87f2d0d38d7b52c0583c16a4e..ef9753cd79227da4c62fe58417f40c008e8c3bdb 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndex.java
+++ b/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.

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputed.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputed.java
index d6eaa965d4bae21c0122eac0c273070ff1063319..0b487a28191bd42c40704df3f0f915bd03cfe6ed 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputed.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexComputed.java
@@ -12,6 +12,7 @@ package org.eclipse.jgit.internal.storage.file;
 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);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1.java
index 76f0793cc8172f27247b4dd82621ed4c99efb9b8..c77a8eb761c00e91214cdb354dc2bb1ab9ac5f6e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackReverseIndexV1.java
@@ -16,10 +16,14 @@ import java.io.IOException;
 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 @@ final class PackReverseIndexV1 implements PackReverseIndex {
        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