From f1c51d1bc5fe20975ac9e95c9717ab3c4cbbd875 Mon Sep 17 00:00:00 2001 From: Klaudio Sinani Date: Mon, 30 May 2022 16:44:56 +0200 Subject: [PATCH] SONAR-16415 Prevent argument injection on `git blame` native command --- .../org/sonar/scm/git/GitBlameCommand.java | 32 +++++++++++- .../sonar/scm/git/GitBlameCommandTest.java | 49 +++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java index 10733386574..4439e71b4aa 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java @@ -27,8 +27,11 @@ import java.util.LinkedList; import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.commons.lang.math.NumberUtils; import org.sonar.api.batch.scm.BlameLine; import org.sonar.api.utils.System2; +import org.sonar.api.utils.Version; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import org.springframework.beans.factory.annotation.Autowired; @@ -42,11 +45,16 @@ public class GitBlameCommand { private static final String COMMITTER_TIME = "committer-time "; private static final String COMMITTER_MAIL = "committer-mail "; + private static final String MINIMUM_REQUIRED_GIT_VERSION = "2.24.0"; private static final String DEFAULT_GIT_COMMAND = "git"; private static final String BLAME_COMMAND = "blame"; private static final String BLAME_LINE_PORCELAIN_FLAG = "--line-porcelain"; + private static final String END_OF_OPTIONS_FLAG = "--end-of-options"; private static final String IGNORE_WHITESPACES = "-w"; + private static final Pattern whitespaceRegex = Pattern.compile("\\s+"); + private static final Pattern semanticVersionDelimiter = Pattern.compile("\\."); + private final System2 system; private final ProcessWrapperFactory processWrapperFactory; private String gitCommand; @@ -73,7 +81,7 @@ public class GitBlameCommand { this.gitCommand = locateDefaultGit(); MutableString stdOut = new MutableString(); this.processWrapperFactory.create(null, l -> stdOut.string = l, gitCommand, "--version").execute(); - return stdOut.string != null && stdOut.string.startsWith("git version"); + return stdOut.string != null && stdOut.string.startsWith("git version") && isCompatibleGitVersion(stdOut.string); } catch (Exception e) { LOG.debug("Failed to find git native client", e); return false; @@ -110,7 +118,7 @@ public class GitBlameCommand { public List blame(Path baseDir, String fileName) throws Exception { BlameOutputProcessor outputProcessor = new BlameOutputProcessor(); try { - this.processWrapperFactory.create(baseDir, outputProcessor::process, gitCommand, BLAME_COMMAND, BLAME_LINE_PORCELAIN_FLAG, IGNORE_WHITESPACES, fileName) + this.processWrapperFactory.create(baseDir, outputProcessor::process, gitCommand, BLAME_COMMAND, BLAME_LINE_PORCELAIN_FLAG, IGNORE_WHITESPACES, END_OF_OPTIONS_FLAG, fileName) .execute(); } catch (UncommittedLineException e) { LOG.debug("Unable to blame file '{}' - it has uncommitted changes", fileName); @@ -166,6 +174,26 @@ public class GitBlameCommand { } } + private static boolean isCompatibleGitVersion(String gitVersionCommandOutput) { + // Due to the danger of argument injection on git blame the use of `--end-of-options` flag is necessary + // The flag is available only on git versions >= 2.24.0 + String gitVersion = whitespaceRegex + .splitAsStream(gitVersionCommandOutput) + .skip(2) + .findFirst() + .orElse(""); + + String formattedGitVersion = formatGitSemanticVersion(gitVersion); + return Version.parse(formattedGitVersion).isGreaterThanOrEqual(Version.parse(MINIMUM_REQUIRED_GIT_VERSION)); + } + + private static String formatGitSemanticVersion(String version) { + return semanticVersionDelimiter + .splitAsStream(version) + .takeWhile(NumberUtils::isNumber) + .collect(Collectors.joining(".")); + } + private static class MutableString { String string; } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java index 328383c235c..b3e428c1715 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java @@ -28,6 +28,7 @@ import java.util.Date; import java.util.LinkedList; import java.util.List; import java.util.function.Consumer; +import java.util.stream.Stream; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; import org.junit.Before; @@ -152,6 +153,41 @@ public class GitBlameCommandTest { assertThat(blameCommand.checkIfEnabled()).isFalse(); } + @Test + public void git_should_not_be_enabled_if_version_command_is_not_found() { + ProcessWrapperFactory mockedCmd = mockGitVersionCommand("error: unknown option `version'"); + GitBlameCommand blameCommand = new GitBlameCommand(System2.INSTANCE, mockedCmd); + assertThat(blameCommand.checkIfEnabled()).isFalse(); + } + + @Test + public void git_should_not_be_enabled_if_version_command_does_not_return_string_output() { + ProcessWrapperFactory mockedCmd = mockGitVersionCommand(null); + GitBlameCommand blameCommand = new GitBlameCommand(System2.INSTANCE, mockedCmd); + assertThat(blameCommand.checkIfEnabled()).isFalse(); + } + + @Test + public void git_should_be_enabled_if_version_is_equal_or_greater_than_required_minimum() { + Stream.of( + "git version 2.24.0", + "git version 2.25.2.1", + "git version 2.24.1.1.windows.2", + "git version 2.25.1.msysgit.2" + ).forEach(output -> { + ProcessWrapperFactory mockedCmd = mockGitVersionCommand(output); + GitBlameCommand blameCommand = new GitBlameCommand(System2.INSTANCE, mockedCmd); + assertThat(blameCommand.checkIfEnabled()).isTrue(); + }); + } + + @Test + public void git_should_not_be_enabled_if_version_is_less_than_required_minimum() { + ProcessWrapperFactory mockFactory = mockGitVersionCommand("git version 1.9.0"); + GitBlameCommand blameCommand = new GitBlameCommand(System2.INSTANCE, mockFactory); + assertThat(blameCommand.checkIfEnabled()).isFalse(); + } + @Test public void throw_exception_if_command_fails() throws Exception { Path baseDir = temp.newFolder().toPath(); @@ -247,4 +283,17 @@ public class GitBlameCommandTest { // This is needed for Windows, otherwise the created File point to invalid (shortened by Windows) temp folder path return temp.newFolder().toPath().toRealPath(LinkOption.NOFOLLOW_LINKS).toFile(); } + + private ProcessWrapperFactory mockGitVersionCommand(String commandOutput) { + ProcessWrapperFactory mockFactory = mock(ProcessWrapperFactory.class); + ProcessWrapper mockProcess = mock(ProcessWrapper.class); + + when(mockFactory.create(isNull(), any(), eq("git"), eq("--version"))).then(invocation -> { + var argument = (Consumer) invocation.getArgument(1); + argument.accept(commandOutput); + return mockProcess; + }); + + return mockFactory; + } } -- 2.39.5