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>tags/v4.7.5.201810051826-r
@@ -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); |
@@ -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 |
@@ -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; |
@@ -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); | |||
} | |||
} | |||
} |
@@ -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; |