From e4c28665b60140f43e2caaa7926fa51e093682d5 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Mon, 1 Oct 2018 13:44:00 -0700 Subject: [PATCH] BaseReceivePack: Validate incoming .gitmodules files The main concern are submodule urls starting with '-' that could pass as options to an unguarded tool. Pass through the parser the ids of blobs identified as .gitmodules files in the ObjectChecker. Load the blobs and parse/validate them in SubmoduleValidator. Change-Id: Ia0cc32ce020d288f995bf7bc68041fda36be1963 Signed-off-by: Ivan Frade Signed-off-by: Matthias Sohn --- .../ReceivePackAdvertiseRefsHookTest.java | 65 +++++++++++++++++++ .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../submodule/SubmoduleValidator.java | 39 +++++++++++ .../jgit/transport/BaseReceivePack.java | 25 ++++++- 5 files changed, 130 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java index 341112288..abd284087 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java @@ -424,6 +424,71 @@ public class ReceivePackAdvertiseRefsHookTest extends LocalDiskRepositoryTestCas assertSame(PacketLineIn.END, r.readString()); } + @Test + public void testIncludesInvalidGitmodules() throws Exception { + final TemporaryBuffer.Heap inBuf = setupSourceRepoInvalidGitmodules(); + final TemporaryBuffer.Heap outBuf = new TemporaryBuffer.Heap(1024); + final ReceivePack rp = new ReceivePack(dst); + rp.setCheckReceivedObjects(true); + rp.setCheckReferencedObjectsAreReachable(true); + rp.setAdvertiseRefsHook(new HidePrivateHook()); + try { + receive(rp, inBuf, outBuf); + fail("Expected UnpackException"); + } catch (UnpackException failed) { + Throwable err = failed.getCause(); + assertTrue(err instanceof IOException); + } + + final PacketLineIn r = asPacketLineIn(outBuf); + String master = r.readString(); + int nul = master.indexOf('\0'); + assertTrue("has capability list", nul > 0); + assertEquals(B.name() + ' ' + R_MASTER, master.substring(0, nul)); + assertSame(PacketLineIn.END, r.readString()); + + String errorLine = r.readString(); + System.out.println(errorLine); + assertTrue(errorLine.startsWith( + "unpack error Invalid submodule URL '-")); + assertEquals("ng refs/heads/s n/a (unpacker error)", r.readString()); + assertSame(PacketLineIn.END, r.readString()); + } + + private TemporaryBuffer.Heap setupSourceRepoInvalidGitmodules() + throws IOException, Exception, MissingObjectException { + String fakeGitmodules = new StringBuilder() + .append("[submodule \"test\"]\n") + .append(" path = xlib\n") + .append(" url = https://example.com/repo/xlib.git\n\n") + .append("[submodule \"test2\"]\n") + .append(" path = zlib\n") + .append(" url = -upayload.sh\n") + .toString(); + + TestRepository s = new TestRepository<>(src); + RevBlob blob = s.blob(fakeGitmodules); + RevCommit N = s.commit().parent(B) + .add(".gitmodules", blob).create(); + RevTree t = s.parseBody(N).getTree(); + + final TemporaryBuffer.Heap pack = new TemporaryBuffer.Heap(1024); + packHeader(pack, 3); + copy(pack, src.open(N)); + copy(pack, src.open(t)); + copy(pack, src.open(blob)); + digest(pack); + + final TemporaryBuffer.Heap inBuf = new TemporaryBuffer.Heap(1024); + final PacketLineOut inPckLine = new PacketLineOut(inBuf); + inPckLine.writeString(ObjectId.zeroId().name() + ' ' + N.name() + ' ' + + "refs/heads/s" + '\0' + + BasePackPushConnection.CAPABILITY_REPORT_STATUS); + inPckLine.end(); + pack.writeTo(inBuf, PM); + return inBuf; + } + @Test public void testUsingUnknownTreeFails() throws Exception { TestRepository s = new TestRepository<>(src); 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 2083e1eef..55e786cdf 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -347,6 +347,7 @@ invalidDepth=Invalid depth: {0} invalidEncryption=Invalid encryption invalidExpandWildcard=ExpandFromSource on a refspec that can have mismatched wildcards does not make sense. invalidGitdirRef = Invalid .git reference in file ''{0}'' +invalidGitModules=Invalid .gitmodules file invalidGitType=invalid git type: {0} invalidId=Invalid id: {0} invalidId0=Invalid id 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 858676397..1bf55e3e7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -406,6 +406,7 @@ public class JGitText extends TranslationBundle { /***/ public String invalidEncryption; /***/ public String invalidExpandWildcard; /***/ public String invalidGitdirRef; + /***/ public String invalidGitModules; /***/ public String invalidGitType; /***/ public String invalidId; /***/ public String invalidId0; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/submodule/SubmoduleValidator.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/submodule/SubmoduleValidator.java index 4821c80f0..d4bba2d0d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/submodule/SubmoduleValidator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/submodule/SubmoduleValidator.java @@ -42,9 +42,13 @@ */ package org.eclipse.jgit.internal.submodule; +import java.io.IOException; import java.text.MessageFormat; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ConfigConstants; /** * Validations for the git submodule fields (name, path, uri). @@ -138,4 +142,39 @@ public class SubmoduleValidator { } } + /** + * @param gitModulesContents + * Contents of a .gitmodule file. They will be parsed internally. + * @throws IOException + * If the contents + */ + public static void assertValidGitModulesFile(String gitModulesContents) + throws IOException { + // Validate .gitmodules file + Config c = new Config(); + try { + c.fromText(gitModulesContents); + for (String subsection : c.getSubsections( + ConfigConstants.CONFIG_SUBMODULE_SECTION)) { + String url = c.getString( + ConfigConstants.CONFIG_SUBMODULE_SECTION, + subsection, ConfigConstants.CONFIG_KEY_URL); + assertValidSubmoduleUri(url); + + assertValidSubmoduleName(subsection); + + String path = c.getString( + ConfigConstants.CONFIG_SUBMODULE_SECTION, subsection, + ConfigConstants.CONFIG_KEY_PATH); + assertValidSubmodulePath(path); + } + } catch (ConfigInvalidException e) { + throw new IOException( + MessageFormat.format( + JGitText.get().invalidGitModules, + e)); + } catch (SubmoduleValidationException e) { + throw new IOException(e.getMessage(), e); + } + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java index 6f94dbbfe..14b683f01 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java @@ -43,6 +43,7 @@ package org.eclipse.jgit.transport; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_ATOMIC; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_DELETE_REFS; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_OFS_DELTA; @@ -76,15 +77,20 @@ import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.errors.TooLargePackException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.file.PackLock; +import org.eclipse.jgit.internal.submodule.SubmoduleValidator; +import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config.SectionParser; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.GitmoduleEntry; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectChecker; +import org.eclipse.jgit.lib.ObjectDatabase; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdSubclassMap; import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; @@ -1081,8 +1087,10 @@ public abstract class BaseReceivePack { */ protected void receivePackAndCheckConnectivity() throws IOException { receivePack(); - if (needCheckConnectivity()) + if (needCheckConnectivity()) { + checkSubmodules(); checkConnectivity(); + } parser = null; } @@ -1400,6 +1408,21 @@ public abstract class BaseReceivePack { || !getClientShallowCommits().isEmpty(); } + private void checkSubmodules() + throws IOException { + ObjectDatabase odb = db.getObjectDatabase(); + if (objectChecker == null) { + return; + } + for (GitmoduleEntry entry : objectChecker.getGitsubmodules()) { + AnyObjectId blobId = entry.getBlobId(); + ObjectLoader blob = odb.open(blobId, Constants.OBJ_BLOB); + + SubmoduleValidator.assertValidGitModulesFile( + new String(blob.getBytes(), UTF_8)); + } + } + private void checkConnectivity() throws IOException { ObjectIdSubclassMap baseObjects = null; ObjectIdSubclassMap providedObjects = null;