From a9347cf22087abe5d550d34b6739b87297c27ab3 Mon Sep 17 00:00:00 2001 From: Anatolii Kmetiuk Date: Tue, 24 Mar 2026 09:12:58 +0900 Subject: [PATCH] Merge commit from fork * 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 * Allowlist-based approach to VCS string sanitization --- main/src/main/scala/sbt/Resolvers.scala | 25 +++--- .../scala/sbt/internal/VcsUriFragment.scala | 32 +++++++ .../internal/ResolversVcsSecurityTest.scala | 85 +++++++++++++++++++ .../sbt/internal/VcsUriFragmentTest.scala | 74 ++++++++++++++++ 4 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 main/src/main/scala/sbt/internal/VcsUriFragment.scala create mode 100644 main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala create mode 100644 main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala diff --git a/main/src/main/scala/sbt/Resolvers.scala b/main/src/main/scala/sbt/Resolvers.scala index d4f5987d4..3ef5b4cdd 100644 --- a/main/src/main/scala/sbt/Resolvers.scala +++ b/main/src/main/scala/sbt/Resolvers.scala @@ -21,10 +21,9 @@ import java.util.Locale import scala.sys.process.{ BasicIO, Process } import scala.util.control.NonFatal -import sbt.internal.util.Util import sbt.internal.{ BuildDependencies, LoadedBuild, LoadedBuildUnit } import sbt.util.Logger -import sbt.internal.RetrieveUnit +import sbt.internal.{ RetrieveUnit, VcsUriFragment } object Resolvers { @@ -67,6 +66,7 @@ object Resolvers { if (uri.hasFragment) { val revision = uri.getFragment + VcsUriFragment.validate(revision) Some { () => creates(localCopy) { run(cwd = None, log = None, "svn", "checkout", "-q", "-r", revision, from, to) @@ -87,6 +87,7 @@ object Resolvers { if (uri.hasFragment) { val branch = uri.getFragment + VcsUriFragment.validate(branch) Some { () => creates(localCopy) { run(cwd = None, log = None, "hg", "clone", "-q", from, localCopy.getAbsolutePath) @@ -108,6 +109,7 @@ object Resolvers { if (uri.hasFragment) { val branch = uri.getFragment + VcsUriFragment.validate(branch) Some { () => creates(localCopy) { run(cwd = None, log = None, "git", "clone", from, localCopy.getAbsolutePath) @@ -132,11 +134,7 @@ object Resolvers { } private def run(cwd: Option[File], log: Option[Logger], command: String*): Unit = { - val process = Process( - if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command - else command, - cwd - ) + val process = Process(command, cwd) val result = (log match { case Some(log) => val io = BasicIO(false, log).withInput(_.close()) @@ -295,7 +293,11 @@ object Resolvers { false } else { val headBefore = captureOutput(Some(localCopy), "git", "rev-parse", "HEAD") - val ref = if (fromURI(uri).hasFragment) uri.getFragment else "HEAD" + val ref = if (fromURI(uri).hasFragment) { + val f = uri.getFragment + VcsUriFragment.validate(f) + f + } else "HEAD" run(Some(localCopy), Some(log), "git", "fetch", "origin", ref) run(Some(localCopy), Some(log), "git", "reset", "--hard", "FETCH_HEAD") markUpdated(localCopy) @@ -310,16 +312,14 @@ object Resolvers { } private def captureOutput(cwd: Option[File], command: String*): String = - Process( - if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command else command, - cwd - ).!!.trim + Process(command, cwd).!!.trim private def updateMercurial(localCopy: File, uri: URI, log: Logger): Boolean = try { val idBefore = captureOutput(Some(localCopy), "hg", "id", "-i") if (fromURI(uri).hasFragment) { val branch = uri.getFragment + VcsUriFragment.validate(branch) run(Some(localCopy), Some(log), "hg", "pull") run(Some(localCopy), Some(log), "hg", "update", branch) } else { @@ -340,6 +340,7 @@ object Resolvers { val revBefore = captureOutput(Some(localCopy), "svn", "info", "--show-item", "revision") if (fromURI(uri).hasFragment) { val revision = uri.getFragment + VcsUriFragment.validate(revision) run(Some(localCopy), Some(log), "svn", "update", "-q", "-r", revision) } else { run(Some(localCopy), Some(log), "svn", "update", "-q") 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..4443dc0d3 --- /dev/null +++ b/main/src/main/scala/sbt/internal/VcsUriFragment.scala @@ -0,0 +1,32 @@ +/* + * 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") + if (fragment.isEmpty) + throw new IllegalArgumentException("VCS URI fragment must not be empty") + fragment.foreach { c => + if (!isAllowed(c)) + throw new IllegalArgumentException( + "Invalid character in VCS URI fragment (only ASCII letters, digits, and - _ . / + are allowed)" + ) + } + } + + private def isAllowed(c: Char): Boolean = + (c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || + c == '-' || c == '_' || c == '.' || c == '/' || c == '+' +} 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..d9337c98b --- /dev/null +++ b/main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala @@ -0,0 +1,85 @@ +/* + * 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..a406b996d --- /dev/null +++ b/main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala @@ -0,0 +1,74 @@ +/* + * 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("accepts hex commit id fragment", testAcceptsHexSha), + example("rejects empty fragment", testRejectsEmpty), + example("rejects ampersand", testRejectsAmpersand), + example("rejects pipe", testRejectsPipe), + example("rejects semicolon", testRejectsSemicolon), + example("rejects space", testRejectsSpace), + example("rejects percent", testRejectsPercent), + example("rejects greater-than", testRejectsGreaterThan), + example("rejects newline", testRejectsNewline), + example("rejects DEL", testRejectsDel), + ) + + def testAcceptsSafe: Result = + VcsUriFragment.validate("develop") + VcsUriFragment.validate("v1.2.3") + VcsUriFragment.validate("feature/foo-bar") + VcsUriFragment.validate("release/1.0.0+build") + Result.success + + def testAcceptsHexSha: Result = + VcsUriFragment.validate("abc123def4567890abcdef1234567890abcdef12") + Result.success + + def testRejectsEmpty: Result = + interceptIllegal("") + + def testRejectsAmpersand: Result = + interceptIllegal("a&b") + + def testRejectsPipe: Result = + interceptIllegal("a|b") + + def testRejectsSemicolon: Result = + interceptIllegal("a;b") + + def testRejectsSpace: Result = + interceptIllegal("a b") + + def testRejectsPercent: Result = + interceptIllegal("a%20b") + + def testRejectsGreaterThan: 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