From ec7f88eec8f52a2254b82bcb73aa489028ea3b39 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 15 Dec 2017 08:50:28 +0900 Subject: [PATCH] Config: Remove the include functionality The Config class must be safe to run against untrusted input files. Reading arbitrary local system paths using include.path is risky for servers, including Gerrit Code Review. This was fixed on master [1] by making "readIncludedConfig" a noop by default. This allows only FileBasedConfig, which originated from local disk, to read local system paths. However, the "readIncludedConfig" method was only introduced in [2] which was needed by [3], both of which are only on the master branch. On the stable branch only Config supports includes. Therefore this commit simply disables the include functionality. [1] https://git.eclipse.org/r/#/c/113371/ [2] https://git.eclipse.org/r/#/c/111847/ [3] https://git.eclipse.org/r/#/c/111848/ Bug: 528781 Change-Id: I9a3be3f1d07c4b6772bff535a2556e699a61381c Signed-off-by: David Pursehouse --- .../tst/org/eclipse/jgit/lib/ConfigTest.java | 24 ++++-------- .../src/org/eclipse/jgit/lib/Config.java | 39 ------------------- 2 files changed, 8 insertions(+), 55 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java index e9505f67d0..a12831a149 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java @@ -80,6 +80,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.SystemReader; import org.junit.After; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -766,6 +767,7 @@ public class ConfigTest { } @Test + @Ignore public void testIncludeInvalidName() throws ConfigInvalidException { expectedEx.expect(ConfigInvalidException.class); expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile); @@ -773,6 +775,7 @@ public class ConfigTest { } @Test + @Ignore public void testIncludeNoValue() throws ConfigInvalidException { expectedEx.expect(ConfigInvalidException.class); expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile); @@ -780,6 +783,7 @@ public class ConfigTest { } @Test + @Ignore public void testIncludeEmptyValue() throws ConfigInvalidException { expectedEx.expect(ConfigInvalidException.class); expectedEx.expectMessage(JGitText.get().invalidLineInConfigFile); @@ -816,6 +820,7 @@ public class ConfigTest { } @Test + @Ignore public void testIncludeTooManyRecursions() throws IOException { File config = tmp.newFile("config"); String include = "[include]\npath=" + config.toPath() + "\n"; @@ -832,27 +837,14 @@ public class ConfigTest { } @Test - public void testInclude() throws IOException, ConfigInvalidException { + public void testIncludeIsNoop() throws IOException, ConfigInvalidException { File config = tmp.newFile("config"); - File more = tmp.newFile("config.more"); - File other = tmp.newFile("config.other"); String fooBar = "[foo]\nbar=true\n"; - String includeMore = "[include]\npath=" + more.toPath() + "\n"; - String includeOther = "path=" + other.toPath() + "\n"; - String fooPlus = fooBar + includeMore + includeOther; - Files.write(config.toPath(), fooPlus.getBytes()); - - String fooMore = "[foo]\nmore=bar\n"; - Files.write(more.toPath(), fooMore.getBytes()); - - String otherMore = "[other]\nmore=bar\n"; - Files.write(other.toPath(), otherMore.getBytes()); + Files.write(config.toPath(), fooBar.getBytes()); Config parsed = parse("[include]\npath=" + config.toPath() + "\n"); - assertTrue(parsed.getBoolean("foo", "bar", false)); - assertEquals("bar", parsed.getString("foo", null, "more")); - assertEquals("bar", parsed.getString("other", null, "more")); + assertFalse(parsed.getBoolean("foo", "bar", false)); } private static void assertReadLong(long exp) throws ConfigInvalidException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java index 6281bcfb3d..b0f5c2cf4e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java @@ -51,9 +51,6 @@ package org.eclipse.jgit.lib; -import java.io.File; -import java.io.FileNotFoundException; -import java.io.IOException; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collections; @@ -70,8 +67,6 @@ import org.eclipse.jgit.events.ListenerHandle; import org.eclipse.jgit.events.ListenerList; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.transport.RefSpec; -import org.eclipse.jgit.util.IO; -import org.eclipse.jgit.util.RawParseUtils; /** * Git style {@code .config}, {@code .gitconfig}, {@code .gitmodules} file. @@ -1073,10 +1068,6 @@ public class Config { e.value = MAGIC_EMPTY_VALUE; } else e.value = readValue(in, false, -1); - - if (e.section.equals("include")) { //$NON-NLS-1$ - addIncludedConfig(newEntries, e, depth); - } } else throw new ConfigInvalidException(JGitText.get().invalidLineInConfigFile); } @@ -1084,36 +1075,6 @@ public class Config { return newEntries; } - private void addIncludedConfig(final List newEntries, - ConfigLine line, int depth) throws ConfigInvalidException { - if (!line.name.equals("path") || //$NON-NLS-1$ - line.value == null || line.value.equals(MAGIC_EMPTY_VALUE)) { - throw new ConfigInvalidException( - JGitText.get().invalidLineInConfigFile); - } - File path = new File(line.value); - try { - byte[] bytes = IO.readFully(path); - String decoded; - if (isUtf8(bytes)) { - decoded = RawParseUtils.decode(RawParseUtils.UTF8_CHARSET, - bytes, 3, bytes.length); - } else { - decoded = RawParseUtils.decode(bytes); - } - newEntries.addAll(fromTextRecurse(decoded, depth + 1)); - } catch (FileNotFoundException fnfe) { - if (path.exists()) { - throw new ConfigInvalidException(MessageFormat - .format(JGitText.get().cannotReadFile, path), fnfe); - } - } catch (IOException ioe) { - throw new ConfigInvalidException( - MessageFormat.format(JGitText.get().cannotReadFile, path), - ioe); - } - } - private ConfigSnapshot newState() { return new ConfigSnapshot(Collections. emptyList(), getBaseState()); -- 2.39.5