diff options
author | Klaudio Sinani <klaudio.sinani@sonarsource.com> | 2022-05-30 16:44:56 +0200 |
---|---|---|
committer | sonartech <sonartech@sonarsource.com> | 2022-05-30 20:02:56 +0000 |
commit | f1c51d1bc5fe20975ac9e95c9717ab3c4cbbd875 (patch) | |
tree | 6deb6a2daa20614339b9808cf53c62e6ad65b83d /sonar-scanner-engine | |
parent | eb6741754b2b35172012bc5b30f5b0d53a61f7be (diff) | |
download | sonarqube-f1c51d1bc5fe20975ac9e95c9717ab3c4cbbd875.tar.gz sonarqube-f1c51d1bc5fe20975ac9e95c9717ab3c4cbbd875.zip |
SONAR-16415 Prevent argument injection on `git blame` native command
Diffstat (limited to 'sonar-scanner-engine')
-rw-r--r-- | sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java | 32 | ||||
-rw-r--r-- | sonar-scanner-engine/src/test/java/org/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<BlameLine> 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; @@ -153,6 +154,41 @@ public class GitBlameCommandTest { } @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(); GitBlameCommand blameCommand = new GitBlameCommand("randomcmdthatwillneverbefound", System2.INSTANCE, processWrapperFactory); @@ -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<String>) invocation.getArgument(1); + argument.accept(commandOutput); + return mockProcess; + }); + + return mockFactory; + } } |