Skip to content

Commit 1ce945b

Browse files
anatoliykmetyukeed3si9n
anatoliykmetyuk
authored and
eed3si9n
committed
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
1 parent 2378719 commit 1ce945b

4 files changed

Lines changed: 185 additions & 9 deletions

File tree

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

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,10 @@ import BuildLoader.ResolveInfo
1919
import RichURI.fromURI
2020
import java.util.Locale
2121

22-
import scala.sys.process.Process
22+
import scala.sys.process.{ BasicIO, Process }
2323
import scala.util.control.NonFatal
24-
import sbt.internal.util.Util
24+
import sbt.util.Logger
25+
import sbt.internal.VcsUriFragment
2526

2627
object Resolvers {
2728
type Resolver = BuildLoader.Resolver
@@ -55,6 +56,7 @@ object Resolvers {
5556

5657
if (uri.hasFragment) {
5758
val revision = uri.getFragment
59+
VcsUriFragment.validate(revision)
5860
Some { () =>
5961
creates(localCopy) {
6062
run("svn", "checkout", "-q", "-r", revision, from, to)
@@ -87,6 +89,7 @@ object Resolvers {
8789

8890
if (uri.hasFragment) {
8991
val branch = uri.getFragment
92+
VcsUriFragment.validate(branch)
9093
Some { () =>
9194
creates(localCopy) {
9295
run("git", "clone", from, localCopy.getAbsolutePath)
@@ -115,6 +118,7 @@ object Resolvers {
115118

116119
if (uri.hasFragment) {
117120
val branch = uri.getFragment
121+
VcsUriFragment.validate(branch)
118122
Some { () =>
119123
creates(localCopy) {
120124
clone(from, to = localCopy)
@@ -133,12 +137,19 @@ object Resolvers {
133137
def run(command: String*): Unit =
134138
run(None, command: _*)
135139

136-
def run(cwd: Option[File], command: String*): Unit = {
137-
val result = Process(
138-
if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command
139-
else command,
140-
cwd
141-
) !;
140+
def run(cwd: Option[File], command: String*): Unit =
141+
run(None, None, command: _*)
142+
143+
private def run(cwd: Option[File], log: Option[Logger], command: String*): Unit = {
144+
val process = Process(command, cwd)
145+
val result = (log match {
146+
case Some(log) =>
147+
val io = BasicIO(false, log).withInput(_.close())
148+
process.run(io).exitValue()
149+
case None =>
150+
process.run().exitValue()
151+
})
152+
142153
if (result != 0)
143154
sys.error("Nonzero exit code (" + result + "): " + command.mkString(" "))
144155
}
@@ -172,6 +183,6 @@ object Resolvers {
172183
private[this] def normalizeDirectoryName(name: String): String =
173184
dropExtensions(name).toLowerCase(Locale.ENGLISH).replaceAll("""\W+""", "-")
174185

175-
private[this] def dropExtensions(name: String): String = name.takeWhile(_ != '.')
186+
private def dropExtensions(name: String): String = name.takeWhile(_ != '.')
176187

177188
}

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/*
2+
* sbt
3+
* Copyright 2023, Scala center
4+
* Copyright 2011 - 2022, Lightbend, Inc.
5+
* Copyright 2008 - 2010, Mark Harrah
6+
* Licensed under Apache License 2.0 (see LICENSE)
7+
*/
8+
9+
package sbt
10+
package internal
11+
12+
private[sbt] object VcsUriFragment {
13+
14+
def validate(fragment: String): Unit = {
15+
if (fragment == null)
16+
throw new IllegalArgumentException("VCS URI fragment must not be null")
17+
fragment.foreach { c =>
18+
if (c == '&' || c == '|' || c == ';')
19+
throw new IllegalArgumentException(
20+
"Invalid character in VCS URI fragment (shell metacharacters are not allowed)"
21+
)
22+
if (Character.isISOControl(c))
23+
throw new IllegalArgumentException(
24+
"Invalid character in VCS URI fragment (control characters are not allowed)"
25+
)
26+
}
27+
}
28+
}

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*
2+
* sbt
3+
* Copyright 2023, Scala center
4+
* Copyright 2011 - 2022, Lightbend, Inc.
5+
* Copyright 2008 - 2010, Mark Harrah
6+
* Licensed under Apache License 2.0 (see LICENSE)
7+
*/
8+
9+
package sbt
10+
package internal
11+
12+
import hedgehog.*
13+
import hedgehog.runner.*
14+
15+
import java.net.URI
16+
import scala.jdk.CollectionConverters.*
17+
18+
import _root_.sbt.Resolvers
19+
import _root_.sbt.io.IO
20+
import _root_.sbt.internal.BuildLoader.ResolveInfo
21+
22+
object ResolversVcsSecurityTest extends Properties:
23+
24+
override def tests: List[Test] = List(
25+
example(
26+
"git resolver rejects fragment containing & before running VCS",
27+
testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main&evil"))
28+
),
29+
example(
30+
"git resolver rejects fragment containing |",
31+
testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main|evil"))
32+
),
33+
example(
34+
"git resolver rejects fragment containing ;",
35+
testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main;evil"))
36+
),
37+
example(
38+
"mercurial resolver rejects fragment containing & before running VCS",
39+
testResolverRejects(Resolvers.mercurial, vcsUri("hg", "/repo", "main&evil"))
40+
),
41+
example(
42+
"subversion resolver rejects fragment containing & before running VCS",
43+
testResolverRejects(Resolvers.subversion, vcsUri("svn", "/repo", "main&evil"))
44+
),
45+
example(
46+
"git resolver accepts safe branch fragment and returns Some",
47+
testResolverAccepts(Resolvers.git, vcsUri("git", "/repo.git", "develop"))
48+
),
49+
example(
50+
"ProcessBuilder passes VCS ref as a single argv element (no shell parsing)",
51+
testProcessBuilderKeepsMetacharactersInSingleArgument
52+
),
53+
)
54+
55+
private def vcsUri(scheme: String, path: String, fragment: String): URI =
56+
new URI(scheme, null, "example.com", -1, path, null, fragment)
57+
58+
private def testResolverRejects(resolver: Resolvers.Resolver, uri: URI): Result =
59+
val staging = IO.createTemporaryDirectory
60+
try
61+
val info = new ResolveInfo(uri, staging, null, null)
62+
try
63+
resolver(info)
64+
Result.failure.log(s"expected IllegalArgumentException for $uri")
65+
catch case _: IllegalArgumentException => Result.success
66+
finally IO.delete(staging)
67+
68+
private def testResolverAccepts(resolver: Resolvers.Resolver, uri: URI): Result =
69+
val staging = IO.createTemporaryDirectory
70+
try
71+
val info = new ResolveInfo(uri, staging, null, null)
72+
try
73+
resolver(info) match
74+
case Some(_) => Result.success
75+
case None => Result.failure.log(s"expected Some for $uri")
76+
catch
77+
case e: IllegalArgumentException =>
78+
Result.failure.log(s"unexpected IllegalArgumentException for $uri: ${e.getMessage}")
79+
finally IO.delete(staging)
80+
81+
private def testProcessBuilderKeepsMetacharactersInSingleArgument: Result =
82+
val argv = new ProcessBuilder("git", "fetch", "origin", "topic&injection").command().asScala.toList
83+
Result.assert(argv == List("git", "fetch", "origin", "topic&injection"))
84+
85+
end ResolversVcsSecurityTest

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
/*
2+
* sbt
3+
* Copyright 2023, Scala center
4+
* Copyright 2011 - 2022, Lightbend, Inc.
5+
* Copyright 2008 - 2010, Mark Harrah
6+
* Licensed under Apache License 2.0 (see LICENSE)
7+
*/
8+
9+
package sbt
10+
package internal
11+
12+
import hedgehog.*
13+
import hedgehog.runner.*
14+
15+
object VcsUriFragmentTest extends Properties:
16+
override def tests: List[Test] = List(
17+
example("accepts typical branch and tag names", testAcceptsSafe),
18+
example("rejects ampersand", testRejectsAmpersand),
19+
example("rejects pipe", testRejectsPipe),
20+
example("rejects semicolon", testRejectsSemicolon),
21+
example("rejects newline", testRejectsNewline),
22+
example("rejects DEL", testRejectsDel),
23+
)
24+
25+
def testAcceptsSafe: Result =
26+
VcsUriFragment.validate("develop")
27+
VcsUriFragment.validate("v1.2.3")
28+
VcsUriFragment.validate("feature/foo-bar")
29+
Result.success
30+
31+
def testRejectsAmpersand: Result =
32+
interceptIllegal("a&b")
33+
34+
def testRejectsPipe: Result =
35+
interceptIllegal("a|b")
36+
37+
def testRejectsSemicolon: Result =
38+
interceptIllegal("a;b")
39+
40+
def testRejectsNewline: Result =
41+
interceptIllegal("a\nb")
42+
43+
def testRejectsDel: Result =
44+
interceptIllegal("a\u007fb")
45+
46+
private def interceptIllegal(s: String): Result =
47+
try
48+
VcsUriFragment.validate(s)
49+
Result.failure.log(s"expected failure for ${s.map(_.toInt).mkString(",")}")
50+
catch case _: IllegalArgumentException => Result.success
51+
52+
end VcsUriFragmentTest

0 commit comments

Comments
 (0)