{"thread":{"id":"15715","subject":"[JGIT PATCH] index-pack: Detect SHA-1 hash collisions to avoid replacing objects","startedAt":"2008-09-30T03:54:47Z","lastAt":"2008-09-30T03:54:47Z","messageCount":1,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"91891","messageId":"1222746887-7711-1-git-send-email-spearce@spearce.org","threadId":"15715","inReplyTo":null,"subject":"[JGIT PATCH] index-pack: Detect SHA-1 hash collisions to avoid replacing objects","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-30T03:54:47Z","receivedAt":"2008-09-30T03:54:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"When indexing a pack file coming in from the network there may\nbe an object contained in the pack that we already have.  Its\nonly safe to retain that object in our newly stored pack if it\nexactly matches the content we already have on disk.  Storing\na different content for the same object name runs the risk of\nallowing a malicious attacker to replace an object in our store.\n\nData validation is performed the first time we have the object\nin memory, which occurs when we first compute its SHA-1.  This\nis the earliest opportunity we have to validate that there is\nno collision, and it reduces the number of times we need to do\na read for an object.  However our memory usage when we process\na whole object is now increased as the collision test is done\nby loading the entire object into memory.  Future improvements\nmay permit a streaming compare of the objects.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n This is only the first in what will be a series to add basic fsck\n support to jgit.  My day-job application is accepting untrusted\n input through IndexPack so I need to fully validate the objects\n are sane before I can allow them to enter the local repository.\n\n In the end we'll honor receive.fsckObjects.  But right now this\n is enough to detect evil SHA-1 collisions, and is the same level\n of protection that git.git's index-pack has.\n\n I'm offering this up for comment because its done and ready for\n inclusion.  The rest of the object fsck rules are going to take\n a bit more time, but I hope to have most of them done tomorrow or\n the day after.\n\n .../src/org/spearce/jgit/transport/IndexPack.java  |   63 +++++++++++++++++---\n 1 files changed, 54 insertions(+), 9 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\nindex bc52896..8a66ec9 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n@@ -375,6 +375,7 @@ private void resolveDeltas(final long pos, final int oldCRC, int type,\n \t\t\tobjectDigest.update(data);\n \t\t\ttempObjectId.fromRaw(objectDigest.digest(), 0);\n \n+\t\t\tverifyNoCollision(type, data);\n \t\t\toe = new PackedObjectInfo(pos, crc32, tempObjectId);\n \t\t\tentries[entryCount++] = oe;\n \t\t}\n@@ -622,7 +623,7 @@ private void indexOneObject() throws IOException {\n \t\t\t\tr = new ArrayList<UnresolvedDelta>(8);\n \t\t\t\tbaseByPos.put(base, r);\n \t\t\t}\n-\t\t\tinflateFromInput(false);\n+\t\t\tskipInflateFromInput(sz);\n \t\t\tr.add(new UnresolvedDelta(pos, (int) crc.getValue()));\n \t\t\tdeltaCount++;\n \t\t\tbreak;\n@@ -637,7 +638,7 @@ private void indexOneObject() throws IOException {\n \t\t\t\tr = new ArrayList<UnresolvedDelta>(8);\n \t\t\t\tbaseById.put(base, r);\n \t\t\t}\n-\t\t\tinflateFromInput(false);\n+\t\t\tskipInflateFromInput(sz);\n \t\t\tr.add(new UnresolvedDelta(pos, (int) crc.getValue()));\n \t\t\tdeltaCount++;\n \t\t\tbreak;\n@@ -649,17 +650,30 @@ private void indexOneObject() throws IOException {\n \n \tprivate void whole(final int type, final long pos, final long sz)\n \t\t\tthrows IOException {\n+\t\tfinal byte[] data = inflateFromInput(sz);\n \t\tobjectDigest.update(Constants.encodedTypeString(type));\n \t\tobjectDigest.update((byte) ' ');\n \t\tobjectDigest.update(Constants.encodeASCII(sz));\n \t\tobjectDigest.update((byte) 0);\n-\t\tinflateFromInput(true);\n+\t\tobjectDigest.update(data);\n \t\ttempObjectId.fromRaw(objectDigest.digest(), 0);\n \n+\t\tverifyNoCollision(type, data);\n \t\tfinal int crc32 = (int) crc.getValue();\n \t\tentries[entryCount++] = new PackedObjectInfo(pos, crc32, tempObjectId);\n \t}\n \n+\tprivate void verifyNoCollision(final int type, final byte[] data)\n+\t\t\tthrows IOException {\n+\t\tfinal ObjectLoader ldr = repo.openObject(tempObjectId);\n+\t\tif (ldr != null) {\n+\t\t\tfinal byte[] existingData = ldr.getCachedBytes();\n+\t\t\tif (ldr.getType() != type || !Arrays.equals(data, existingData)) {\n+\t\t\t\tthrow new IOException(\"collision in \" + tempObjectId.name());\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \t// Current position of {@link #bOffset} within the entire file.\n \tprivate long position() {\n \t\treturn bBase + bOffset;\n@@ -747,7 +761,7 @@ private void sync() throws IOException {\n \t\tbOffset = 0;\n \t}\n \n-\tprivate void inflateFromInput(final boolean digest) throws IOException {\n+\tprivate void skipInflateFromInput(long sz) throws IOException {\n \t\tfinal Inflater inf = inflater;\n \t\ttry {\n \t\t\tfinal byte[] dst = objectData;\n@@ -765,21 +779,52 @@ private void inflateFromInput(final boolean digest) throws IOException {\n \n \t\t\t\tint free = dst.length - n;\n \t\t\t\tif (free < 8) {\n-\t\t\t\t\tif (digest)\n-\t\t\t\t\t\tobjectDigest.update(dst, 0, n);\n+\t\t\t\t\tsz -= n;\n \t\t\t\t\tn = 0;\n \t\t\t\t\tfree = dst.length;\n \t\t\t\t}\n-\n \t\t\t\tn += inf.inflate(dst, n, free);\n \t\t\t}\n-\t\t\tif (digest)\n-\t\t\t\tobjectDigest.update(dst, 0, n);\n+\t\t\tif (n != sz)\n+\t\t\t\tthrow new DataFormatException(\"wrong decompressed length\");\n+\t\t\tn = bAvail - inf.getRemaining();\n+\t\t\tif (n > 0) {\n+\t\t\t\tcrc.update(buf, p, n);\n+\t\t\t\tuse(n);\n+\t\t\t}\n+\t\t} catch (DataFormatException dfe) {\n+\t\t\tthrow corrupt(dfe);\n+\t\t} finally {\n+\t\t\tinf.reset();\n+\t\t}\n+\t}\n+\n+\tprivate byte[] inflateFromInput(final long sz) throws IOException {\n+\t\tfinal byte[] dst = new byte[(int) sz];\n+\t\tfinal Inflater inf = inflater;\n+\t\ttry {\n+\t\t\tint n = 0;\n+\t\t\tint p = -1;\n+\t\t\twhile (!inf.finished()) {\n+\t\t\t\tif (inf.needsInput()) {\n+\t\t\t\t\tif (p >= 0) {\n+\t\t\t\t\t\tcrc.update(buf, p, bAvail);\n+\t\t\t\t\t\tuse(bAvail);\n+\t\t\t\t\t}\n+\t\t\t\t\tp = fillFromInput(1);\n+\t\t\t\t\tinf.setInput(buf, p, bAvail);\n+\t\t\t\t}\n+\n+\t\t\t\tn += inf.inflate(dst, n, dst.length - n);\n+\t\t\t}\n+\t\t\tif (n != sz)\n+\t\t\t\tthrow new DataFormatException(\"wrong decompressed length\");\n \t\t\tn = bAvail - inf.getRemaining();\n \t\t\tif (n > 0) {\n \t\t\t\tcrc.update(buf, p, n);\n \t\t\t\tuse(n);\n \t\t\t}\n+\t\t\treturn dst;\n \t\t} catch (DataFormatException dfe) {\n \t\t\tthrow corrupt(dfe);\n \t\t} finally {\n-- \n1.6.0.2.513.g6dbd\n"}]}