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 3411122888ed2bd6b6bf85433c256a0ceb0bfbe5..abd284087348612ab1e5be99b323ff42d6f00d7c 100644 (file)
--- 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<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);

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 2083e1eef9842c780e768ce9264665a456c9640c..55e786cdf9adfa53790e09f30ab987473ffbd6e1 100644 (file)
--- 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 8586763970cc682dd42751e41cc09534e1362fb8..1bf55e3e7a1ee28472d338f415a762c705a471d6 100644 (file)
--- 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 4821c80f00f9ee3a04f81b82f2f590a377ff39c5..d4bba2d0d1070a391d80710e91f6f702b409d11b 100644 (file)
--- 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 6f94dbbfecb3a71eb3fdf115b02a56cc4300d81e..14b683f017bd02a79fe99533ff5e6a63907c4f9a 100644 (file)
--- 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<ObjectId> baseObjects = null;
                ObjectIdSubclassMap<ObjectId> providedObjects = null;