diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleAddTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleAddTest.java index 5c46659c0..93f47090a 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleAddTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleAddTest.java @@ -182,6 +182,37 @@ public void apply(DirCacheEntry ent) { } } + @Test + public void addSubmoduleWithInvalidPath() throws Exception { + SubmoduleAddCommand command = new SubmoduleAddCommand(db); + command.setPath("-invalid-path"); + // TODO(ms) set name to a valid value in 5.1.0 and adapt expected + // message below + command.setURI("http://example.com/repo/x.git"); + try { + command.call().close(); + fail("Exception not thrown"); + } catch (IllegalArgumentException e) { + // TODO(ms) should check for submodule path, but can't set name + // before 5.1.0 + assertEquals("Invalid submodule name '-invalid-path'", + e.getMessage()); + } + } + + @Test + public void addSubmoduleWithInvalidUri() throws Exception { + SubmoduleAddCommand command = new SubmoduleAddCommand(db); + command.setPath("valid-path"); + command.setURI("-upstream"); + try { + command.call().close(); + fail("Exception not thrown"); + } catch (IllegalArgumentException e) { + assertEquals("Invalid submodule URL '-upstream'", e.getMessage()); + } + } + @Test public void addSubmoduleWithRelativeUri() throws Exception { try (Git git = new Git(db)) { @@ -269,4 +300,4 @@ public void addSubmoduleWithExistingSubmoduleDefined() throws Exception { ConfigConstants.CONFIG_KEY_URL)); } } -} \ No newline at end of file +} diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 4badd2c1c..ed43015a3 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -3,7 +3,7 @@ - + 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 34457c936..2083e1eef 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -358,6 +358,7 @@ invalidKey=Invalid key: {0} invalidLineInConfigFile=Invalid line in config file invalidModeFor=Invalid mode {0} for {1} {2} in {3}. invalidModeForPath=Invalid mode {0} for path {1} +invalidNameContainsDotDot=Invalid name (contains ".."): {0} invalidObject=Invalid {0} {1}: {2} invalidOldIdSent=invalid old id sent invalidPacketLineHeader=Invalid packet line header: {0} @@ -605,7 +606,10 @@ storePushCertMultipleRefs=Store push certificate for {0} refs storePushCertOneRef=Store push certificate for {0} storePushCertReflog=Store push certificate submoduleExists=Submodule ''{0}'' already exists in the index +submoduleNameInvalid=Invalid submodule name ''{0}'' submoduleParentRemoteUrlInvalid=Cannot remove segment from remote url ''{0}'' +submodulePathInvalid=Invalid submodule path ''{0}'' +submoduleUrlInvalid=Invalid submodule URL ''{0}'' submodulesNotSupported=Submodules are not supported supportOnlyPackIndexVersion2=Only support index version 2 symlinkCannotBeWrittenAsTheLinkTarget=Symlink "{0}" cannot be written as the link target cannot be read from within Java. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/SubmoduleAddCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/SubmoduleAddCommand.java index 0519d454e..e3ba8945d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/SubmoduleAddCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/SubmoduleAddCommand.java @@ -51,6 +51,7 @@ import org.eclipse.jgit.api.errors.NoFilepatternException; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.internal.submodule.SubmoduleValidator; import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; @@ -157,6 +158,14 @@ public Repository call() throws GitAPIException { if (uri == null || uri.length() == 0) throw new IllegalArgumentException(JGitText.get().uriNotConfigured); + try { + SubmoduleValidator.assertValidSubmoduleName(path); + SubmoduleValidator.assertValidSubmodulePath(path); + SubmoduleValidator.assertValidSubmoduleUri(uri); + } catch (SubmoduleValidator.SubmoduleValidationException e) { + throw new IllegalArgumentException(e.getMessage()); + } + try { if (submoduleExists()) throw new JGitInternalException(MessageFormat.format( 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 41f3c2a48..858676397 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -417,6 +417,7 @@ public static JGitText get() { /***/ public String invalidLineInConfigFile; /***/ public String invalidModeFor; /***/ public String invalidModeForPath; + /***/ public String invalidNameContainsDotDot; /***/ public String invalidObject; /***/ public String invalidOldIdSent; /***/ public String invalidPacketLineHeader; @@ -664,8 +665,11 @@ public static JGitText get() { /***/ public String storePushCertOneRef; /***/ public String storePushCertReflog; /***/ public String submoduleExists; - /***/ public String submodulesNotSupported; + /***/ public String submoduleNameInvalid; /***/ public String submoduleParentRemoteUrlInvalid; + /***/ public String submodulePathInvalid; + /***/ public String submodulesNotSupported; + /***/ public String submoduleUrlInvalid; /***/ public String supportOnlyPackIndexVersion2; /***/ public String symlinkCannotBeWrittenAsTheLinkTarget; /***/ public String systemConfigFileInvalid; 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 new file mode 100644 index 000000000..4821c80f0 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/submodule/SubmoduleValidator.java @@ -0,0 +1,141 @@ +/* + * Copyright (C) 2018, Google LLC. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.eclipse.jgit.internal.submodule; + +import java.text.MessageFormat; + +import org.eclipse.jgit.internal.JGitText; + +/** + * Validations for the git submodule fields (name, path, uri). + * + * Invalid values in these fields can cause security problems as reported in + * CVE-2018-11235 and and CVE-2018-17456 + */ +public class SubmoduleValidator { + + /** + * Error validating a git submodule declaration + */ + public static class SubmoduleValidationException extends Exception { + + /** + * @param message + * Description of the problem + */ + public SubmoduleValidationException(String message) { + super(message); + } + + private static final long serialVersionUID = 1L; + } + + /** + * Validate name for a submodule + * + * @param name + * name of a submodule + * @throws SubmoduleValidationException + * name doesn't seem valid (detail in message) + */ + public static void assertValidSubmoduleName(String name) + throws SubmoduleValidationException { + if (name.contains("/../") || name.contains("\\..\\") //$NON-NLS-1$ //$NON-NLS-2$ + || name.startsWith("../") || name.startsWith("..\\") //$NON-NLS-1$ //$NON-NLS-2$ + || name.endsWith("/..") || name.endsWith("\\..")) { //$NON-NLS-1$ //$NON-NLS-2$ + // Submodule names are used to store the submodule repositories + // under $GIT_DIR/modules. Having ".." in submodule names makes a + // vulnerability (CVE-2018-11235 + // https://bugs.eclipse.org/bugs/show_bug.cgi?id=535027#c0) + // Reject names containing ".." path segments. We don't + // automatically replace these characters or canonicalize by + // regarding the name as a file path. + // Since Path class is platform dependent, we manually check '/' and + // '\\' patterns here. + throw new SubmoduleValidationException(MessageFormat + .format(JGitText.get().invalidNameContainsDotDot, name)); + } + + if (name.startsWith("-")) { //$NON-NLS-1$ + throw new SubmoduleValidationException( + MessageFormat.format( + JGitText.get().submoduleNameInvalid, name)); + } + } + + /** + * Validate URI for a submodule + * + * @param uri + * uri of a submodule + * @throws SubmoduleValidationException + * uri doesn't seem valid + */ + public static void assertValidSubmoduleUri(String uri) + throws SubmoduleValidationException { + if (uri.startsWith("-")) { //$NON-NLS-1$ + throw new SubmoduleValidationException( + MessageFormat.format( + JGitText.get().submoduleUrlInvalid, uri)); + } + } + + /** + * Validate path for a submodule + * + * @param path + * path of a submodule + * @throws SubmoduleValidationException + * path doesn't look right + */ + public static void assertValidSubmodulePath(String path) + throws SubmoduleValidationException { + + if (path.startsWith("-")) { //$NON-NLS-1$ + throw new SubmoduleValidationException( + MessageFormat.format( + JGitText.get().submodulePathInvalid, path)); + } + } + +}