Harden Windows VCS URI fragments against command injection

- Use Process(argv) for git/hg/svn without cmd /c on Windows
- Add VcsUriFragment.validate for fragments in clone/checkout/update
- Add tests

main/src/main/scala/sbt/Resolvers.scala

@@ -19,9 +19,10 @@ import BuildLoader.ResolveInfo
 import RichURI.fromURI
 import java.util.Locale
 
-import scala.sys.process.Process
+import scala.sys.process.{ BasicIO, Process }
 import scala.util.control.NonFatal
-import sbt.internal.util.Util
+import sbt.util.Logger
+import sbt.internal.VcsUriFragment
 
 object Resolvers {
   type Resolver = BuildLoader.Resolver
@@ -56,6 +57,7 @@ object Resolvers {
 
     if (uri.hasFragment) {
       val revision = uri.getFragment
+      VcsUriFragment.validate(revision)
       Some { () =>
         creates(localCopy) {
           run("svn", "checkout", "-q", "-r", revision, from, to)
@@ -88,6 +90,7 @@ object Resolvers {
 
     if (uri.hasFragment) {
       val branch = uri.getFragment
+      VcsUriFragment.validate(branch)
       Some { () =>
         creates(localCopy) {
           run("git", "clone", from, localCopy.getAbsolutePath)
@@ -116,6 +119,7 @@ object Resolvers {
 
       if (uri.hasFragment) {
         val branch = uri.getFragment
+        VcsUriFragment.validate(branch)
         Some { () =>
           creates(localCopy) {
             clone(from, to = localCopy)
@@ -134,14 +138,20 @@ object Resolvers {
   def run(command: String*): Unit =
     run(None, command*)
 
-  def run(cwd: Option[File], command: String*): Unit = {
+  def run(cwd: Option[File], command: String*): Unit =
-    val result = Process(
+    run(None, None, command*)
-      if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command
+
-      else command,
+  private def run(cwd: Option[File], log: Option[Logger], command: String*): Unit = {
-      cwd
+    val process = Process(command, cwd)
-    ).!
+    val result = (log match {
-    if (result != 0)
+      case Some(log) =>
-      sys.error("Nonzero exit code (" + result + "): " + command.mkString(" "))
+        val io = BasicIO(false, log).withInput(_.close())
+        process.run(io).exitValue()
+      case None =>
+        process.run().exitValue()
+    })
+
+    if result != 0 then sys.error("Nonzero exit code (" + result + "): " + command.mkString(" "))
   }
 
   def creates(file: File)(f: => Unit) = {

main/src/main/scala/sbt/internal/VcsUriFragment.scala

@@ -0,0 +1,28 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+private[sbt] object VcsUriFragment {
+
+  def validate(fragment: String): Unit = {
+    if (fragment == null)
+      throw new IllegalArgumentException("VCS URI fragment must not be null")
+    fragment.foreach { c =>
+      if (c == '&' || c == '|' || c == ';')
+        throw new IllegalArgumentException(
+          "Invalid character in VCS URI fragment (shell metacharacters are not allowed)"
+        )
+      if (Character.isISOControl(c))
+        throw new IllegalArgumentException(
+          "Invalid character in VCS URI fragment (control characters are not allowed)"
+        )
+    }
+  }
+}

main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala

@@ -0,0 +1,86 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+import hedgehog.*
+import hedgehog.runner.*
+
+import java.net.URI
+import scala.jdk.CollectionConverters.*
+
+import _root_.sbt.Resolvers
+import _root_.sbt.io.IO
+import _root_.sbt.internal.BuildLoader.ResolveInfo
+
+object ResolversVcsSecurityTest extends Properties:
+
+  override def tests: List[Test] = List(
+    example(
+      "git resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main&evil"))
+    ),
+    example(
+      "git resolver rejects fragment containing |",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main|evil"))
+    ),
+    example(
+      "git resolver rejects fragment containing ;",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main;evil"))
+    ),
+    example(
+      "mercurial resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.mercurial, vcsUri("hg", "/repo", "main&evil"))
+    ),
+    example(
+      "subversion resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.subversion, vcsUri("svn", "/repo", "main&evil"))
+    ),
+    example(
+      "git resolver accepts safe branch fragment and returns Some",
+      testResolverAccepts(Resolvers.git, vcsUri("git", "/repo.git", "develop"))
+    ),
+    example(
+      "ProcessBuilder passes VCS ref as a single argv element (no shell parsing)",
+      testProcessBuilderKeepsMetacharactersInSingleArgument
+    ),
+  )
+
+  private def vcsUri(scheme: String, path: String, fragment: String): URI =
+    new URI(scheme, null, "example.com", -1, path, null, fragment)
+
+  private def testResolverRejects(resolver: Resolvers.Resolver, uri: URI): Result =
+    val staging = IO.createTemporaryDirectory
+    try
+      val info = new ResolveInfo(uri, staging, null, null)
+      try
+        resolver(info)
+        Result.failure.log(s"expected IllegalArgumentException for $uri")
+      catch case _: IllegalArgumentException => Result.success
+    finally IO.delete(staging)
+
+  private def testResolverAccepts(resolver: Resolvers.Resolver, uri: URI): Result =
+    val staging = IO.createTemporaryDirectory
+    try
+      val info = new ResolveInfo(uri, staging, null, null)
+      try
+        resolver(info) match
+          case Some(_) => Result.success
+          case None    => Result.failure.log(s"expected Some for $uri")
+      catch
+        case e: IllegalArgumentException =>
+          Result.failure.log(s"unexpected IllegalArgumentException for $uri: ${e.getMessage}")
+    finally IO.delete(staging)
+
+  private def testProcessBuilderKeepsMetacharactersInSingleArgument: Result =
+    val argv =
+      new ProcessBuilder("git", "fetch", "origin", "topic&injection").command().asScala.toList
+    Result.assert(argv == List("git", "fetch", "origin", "topic&injection"))
+
+end ResolversVcsSecurityTest

main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala

@@ -0,0 +1,52 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+import hedgehog.*
+import hedgehog.runner.*
+
+object VcsUriFragmentTest extends Properties:
+  override def tests: List[Test] = List(
+    example("accepts typical branch and tag names", testAcceptsSafe),
+    example("rejects ampersand", testRejectsAmpersand),
+    example("rejects pipe", testRejectsPipe),
+    example("rejects semicolon", testRejectsSemicolon),
+    example("rejects newline", testRejectsNewline),
+    example("rejects DEL", testRejectsDel),
+  )
+
+  def testAcceptsSafe: Result =
+    VcsUriFragment.validate("develop")
+    VcsUriFragment.validate("v1.2.3")
+    VcsUriFragment.validate("feature/foo-bar")
+    Result.success
+
+  def testRejectsAmpersand: Result =
+    interceptIllegal("a&b")
+
+  def testRejectsPipe: Result =
+    interceptIllegal("a|b")
+
+  def testRejectsSemicolon: Result =
+    interceptIllegal("a;b")
+
+  def testRejectsNewline: Result =
+    interceptIllegal("a\nb")
+
+  def testRejectsDel: Result =
+    interceptIllegal("a\u007fb")
+
+  private def interceptIllegal(s: String): Result =
+    try
+      VcsUriFragment.validate(s)
+      Result.failure.log(s"expected failure for ${s.map(_.toInt).mkString(",")}")
+    catch case _: IllegalArgumentException => Result.success
+
+end VcsUriFragmentTest