diff --git a/main/src/main/scala/sbt/Resolvers.scala b/main/src/main/scala/sbt/Resolvers.scala index 14312c732..2cd9ab14b 100644 --- a/main/src/main/scala/sbt/Resolvers.scala +++ b/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 = { - val result = Process( - if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command - else command, - cwd - ).! - if (result != 0) - sys.error("Nonzero exit code (" + result + "): " + command.mkString(" ")) + def run(cwd: Option[File], command: String*): Unit = + run(None, None, command*) + + private def run(cwd: Option[File], log: Option[Logger], command: String*): Unit = { + val process = Process(command, cwd) + val result = (log match { + case Some(log) => + 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) = { diff --git a/main/src/main/scala/sbt/internal/VcsUriFragment.scala b/main/src/main/scala/sbt/internal/VcsUriFragment.scala new file mode 100644 index 000000000..0d112c493 --- /dev/null +++ b/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)" + ) + } + } +} diff --git a/main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala b/main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala new file mode 100644 index 000000000..14516f1ae --- /dev/null +++ b/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 diff --git a/main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala b/main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala new file mode 100644 index 000000000..2d20b84b7 --- /dev/null +++ b/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