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 <ifrade@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
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 3411122..abd2840 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
@@ -425,6 +425,71 @@ public void testUsingUnknownBlobFails() throws Exception {
}
@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<Repository> 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<Repository> s = new TestRepository<>(src);
RevCommit N = s.commit().parent(B).add("q", s.blob("a")).create();
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 2083e1e..55e786c 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 @@
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 8586763..1bf55e3 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 static JGitText get() {
/***/ 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 4821c80..d4bba2d 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 static void assertValidSubmodulePath(String path)
}
}
+ /**
+ * @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 6f94dbb..14b683f 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.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 @@ protected Map<String, Ref> getAdvertisedOrDefaultRefs() {
*/
protected void receivePackAndCheckConnectivity() throws IOException {
receivePack();
- if (needCheckConnectivity())
+ if (needCheckConnectivity()) {
+ checkSubmodules();
checkConnectivity();
+ }
parser = null;
}
@@ -1400,6 +1408,21 @@ private boolean needCheckConnectivity() {
|| !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<ObjectId> baseObjects = null;
ObjectIdSubclassMap<ObjectId> providedObjects = null;
