From 81f3119f6acded4bb9baebe60cebce78a2eabb14 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 30 Sep 2008 18:31:30 -0700 Subject: [PATCH] Honor receive.fsckobjects during any fetch connection If the configuration option receive.fsckobjects is true or if the application requests it on the Transport each object received over the wire is validated to pass "git fsck" style rules. This can be useful when fetching data from an untrusted source, to ensure that the incoming objects comply with parsing standards. The optional checking does require extra CPU on the client side. A test against git.git (69601 objects 49719 deltas) showed: receive.fsckobjects average time -------------------------------------- false (default) 0m17.588s true 0m18.465s So the additional checking costs about 5% more in client side time. Signed-off-by: Shawn O. Pearce Signed-off-by: Robin Rosenberg --- .../jgit/transport/BasePackFetchConnection.java | 1 + .../src/org/spearce/jgit/transport/IndexPack.java | 60 +++++++++++++++++++--- .../src/org/spearce/jgit/transport/Transport.java | 24 +++++++++ .../spearce/jgit/transport/TransportBundle.java | 10 +++- .../jgit/transport/WalkFetchConnection.java | 26 ++++++++-- 5 files changed, 108 insertions(+), 13 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java index a22b33d1..a542eb75 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java @@ -452,6 +452,7 @@ abstract class BasePackFetchConnection extends BasePackConnection implements ip = IndexPack.create(local, sideband ? pckIn.sideband(monitor) : in); ip.setFixThin(thinPack); + ip.setObjectChecking(transport.isCheckFetchedObjects()); ip.index(monitor); ip.renameAndOpenPack(); } diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java index 8a66ec97..2752bb0b 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java @@ -56,10 +56,12 @@ import java.util.zip.Inflater; import org.spearce.jgit.errors.CorruptObjectException; import org.spearce.jgit.errors.MissingObjectException; +import org.spearce.jgit.lib.AnyObjectId; import org.spearce.jgit.lib.BinaryDelta; import org.spearce.jgit.lib.Constants; import org.spearce.jgit.lib.InflaterCache; import org.spearce.jgit.lib.MutableObjectId; +import org.spearce.jgit.lib.ObjectChecker; import org.spearce.jgit.lib.ObjectId; import org.spearce.jgit.lib.ObjectIdMap; import org.spearce.jgit.lib.ObjectLoader; @@ -136,6 +138,8 @@ public class IndexPack { private int bAvail; + private ObjectChecker objCheck; + private boolean fixThin; private int outputVersion; @@ -231,6 +235,40 @@ public class IndexPack { } /** + * Configure the checker used to validate received objects. + *

+ * Usually object checking isn't necessary, as Git implementations only + * create valid objects in pack files. However, additional checking may be + * useful if processing data from an untrusted source. + * + * @param oc + * the checker instance; null to disable object checking. + */ + public void setObjectChecker(final ObjectChecker oc) { + objCheck = oc; + } + + /** + * Configure the checker used to validate received objects. + *

+ * Usually object checking isn't necessary, as Git implementations only + * create valid objects in pack files. However, additional checking may be + * useful if processing data from an untrusted source. + *

+ * This is shorthand for: + * + * 

+	 * setObjectChecker(on ? new ObjectChecker() : null);
+	 *
+ * + * @param on + * true to enable the default checker; false to disable it. + */ + public void setObjectChecking(final boolean on) { + setObjectChecker(on ? new ObjectChecker() : null); + } + + /** * Consume data from the input stream until the packfile is indexed. * * @param progress @@ -375,7 +413,7 @@ public class IndexPack { objectDigest.update(data); tempObjectId.fromRaw(objectDigest.digest(), 0); - verifyNoCollision(type, data); + verifySafeObject(tempObjectId, type, data); oe = new PackedObjectInfo(pos, crc32, tempObjectId); entries[entryCount++] = oe; } @@ -658,18 +696,28 @@ public class IndexPack { objectDigest.update(data); tempObjectId.fromRaw(objectDigest.digest(), 0); - verifyNoCollision(type, data); + verifySafeObject(tempObjectId, type, data); final int crc32 = (int) crc.getValue(); entries[entryCount++] = new PackedObjectInfo(pos, crc32, tempObjectId); } - private void verifyNoCollision(final int type, final byte[] data) - throws IOException { - final ObjectLoader ldr = repo.openObject(tempObjectId); + private void verifySafeObject(final AnyObjectId id, final int type, + final byte[] data) throws IOException { + if (objCheck != null) { + try { + objCheck.check(type, data); + } catch (CorruptObjectException e) { + throw new IOException("Invalid " + + Constants.encodedTypeString(type) + " " + id.name() + + ":" + e.getMessage()); + } + } + + final ObjectLoader ldr = repo.openObject(id); if (ldr != null) { final byte[] existingData = ldr.getCachedBytes(); if (ldr.getType() != type || !Arrays.equals(data, existingData)) { - throw new IOException("collision in " + tempObjectId.name()); + throw new IOException("Collision on " + id.name()); } } } diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java index 7284b283..28700b72 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java @@ -55,6 +55,7 @@ import org.spearce.jgit.lib.NullProgressMonitor; import org.spearce.jgit.lib.ProgressMonitor; import org.spearce.jgit.lib.Ref; import org.spearce.jgit.lib.Repository; +import org.spearce.jgit.lib.TransferConfig; /** * Connects two Git repositories together and copies objects between them. @@ -351,6 +352,9 @@ public abstract class Transport { /** Should push just check for operation result, not really push. */ private boolean dryRun; + /** Should an incoming (fetch) transfer validate objects? */ + private boolean checkFetchedObjects; + /** * Create a new transport instance. * @@ -363,8 +367,10 @@ public abstract class Transport { * URI passed to {@link #open(Repository, URIish)}. */ protected Transport(final Repository local, final URIish uri) { + final TransferConfig tc = local.getConfig().getTransfer(); this.local = local; this.uri = uri; + this.checkFetchedObjects = tc.isFsckObjects(); } /** @@ -444,6 +450,24 @@ public abstract class Transport { } /** + * @return true if fetch will verify received objects are formatted + * correctly. Validating objects requires more CPU time on the + * client side of the connection. + */ + public boolean isCheckFetchedObjects() { + return checkFetchedObjects; + } + + /** + * @param check + * true to enable checking received objects; false to assume all + * received objects are valid. + */ + public void setCheckFetchedObjects(final boolean check) { + checkFetchedObjects = check; + } + + /** * Default setting is: {@value RemoteConfig#DEFAULT_RECEIVE_PACK} * * @return remote executable providing receive-pack service for pack diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java index d43a18fd..5b321a0d 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java @@ -170,8 +170,7 @@ abstract class TransportBundle extends PackTransport { final Collection want) throws TransportException { verifyPrerequisites(); try { - final IndexPack ip = IndexPack.create(local, bin); - ip.setFixThin(true); + final IndexPack ip = newIndexPack(); ip.index(monitor); ip.renameAndOpenPack(); } catch (IOException err) { @@ -183,6 +182,13 @@ abstract class TransportBundle extends PackTransport { } } + private IndexPack newIndexPack() throws IOException { + final IndexPack ip = IndexPack.create(local, bin); + ip.setFixThin(true); + ip.setObjectChecking(TransportBundle.this.isCheckFetchedObjects()); + return ip; + } + private void verifyPrerequisites() throws TransportException { if (prereqs.isEmpty()) return; diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java index f309fe1c..56384546 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java @@ -59,6 +59,7 @@ import org.spearce.jgit.errors.TransportException; import org.spearce.jgit.lib.AnyObjectId; import org.spearce.jgit.lib.Constants; import org.spearce.jgit.lib.FileMode; +import org.spearce.jgit.lib.ObjectChecker; import org.spearce.jgit.lib.ObjectId; import org.spearce.jgit.lib.PackIndex; import org.spearce.jgit.lib.ProgressMonitor; @@ -99,6 +100,9 @@ class WalkFetchConnection extends BaseFetchConnection { /** The repository this transport fetches into, or pushes out of. */ private final Repository local; + /** If not null the validator for received objects. */ + private final ObjectChecker objCheck; + /** * List of all remote repositories we may need to get objects out of. *

@@ -157,9 +161,9 @@ class WalkFetchConnection extends BaseFetchConnection { */ private final HashMap> fetchErrors; - WalkFetchConnection(final WalkTransport walkTransport, - final WalkRemoteObjectDatabase w) { - local = walkTransport.local; + WalkFetchConnection(final WalkTransport wt, final WalkRemoteObjectDatabase w) { + local = wt.local; + objCheck = wt.isCheckFetchedObjects() ? new ObjectChecker() : null; remotes = new ArrayList(); remotes.add(w); @@ -560,10 +564,21 @@ class WalkFetchConnection extends BaseFetchConnection { throw e; } - if (!AnyObjectId.equals(id, uol.getId())) + if (!AnyObjectId.equals(id, uol.getId())) { throw new TransportException("Incorrect hash for " + id.name() + "; computed " + uol.getId().name() + " as a " - + uol.getType() + " from " + compressed.length + " bytes."); + + Constants.encodedTypeString(uol.getType()) + " from " + + compressed.length + " bytes."); + } + if (objCheck != null) { + try { + objCheck.check(uol.getType(), uol.getCachedBytes()); + } catch (CorruptObjectException e) { + throw new TransportException("Invalid " + + Constants.encodedTypeString(uol.getType()) + " " + + id.name() + ":" + e.getMessage()); + } + } } private void saveLooseObject(final AnyObjectId id, final byte[] compressed) @@ -811,6 +826,7 @@ class WalkFetchConnection extends BaseFetchConnection { s = connection.open("pack/" + packName); ip = IndexPack.create(local, s.in); ip.setFixThin(false); + ip.setObjectChecker(objCheck); ip.index(monitor); ip.renameAndOpenPack(); } -- 2.11.4.GIT