diff --git a/build.sbt b/build.sbt index c59936eae..d8f4038a2 100644 --- a/build.sbt +++ b/build.sbt @@ -630,6 +630,7 @@ lazy val commandProj = (project in file("main-command")) contrabandSettings, mimaSettings, mimaBinaryIssueFilters ++= Vector( + exclude[ReversedMissingMethodProblem]("sbt.internal.server.ServerCallback.*"), exclude[MissingClassProblem]("sbt.internal.util.JoinThread"), exclude[MissingClassProblem]("sbt.internal.util.JoinThread$"), exclude[MissingClassProblem]("sbt.internal.util.ReadJsonFromInputStream"), diff --git a/main-command/src/main/scala/sbt/internal/server/ServerHandler.scala b/main-command/src/main/scala/sbt/internal/server/ServerHandler.scala index 5c3cc69ec..be88d5237 100644 --- a/main-command/src/main/scala/sbt/internal/server/ServerHandler.scala +++ b/main-command/src/main/scala/sbt/internal/server/ServerHandler.scala @@ -81,6 +81,7 @@ trait ServerCallback { private[sbt] def authOptions: Set[ServerAuthentication] private[sbt] def authenticate(token: String): Boolean + private[sbt] def isAuthenticated: Boolean private[sbt] def setInitialized(value: Boolean): Unit private[sbt] def setInitializeOption(opts: InitializeOption): Unit private[sbt] def onSettingQuery(execId: Option[String], req: Q): Unit diff --git a/main/src/main/scala/sbt/internal/LintUnused.scala b/main/src/main/scala/sbt/internal/LintUnused.scala index 32ff02568..d4a85a28d 100644 --- a/main/src/main/scala/sbt/internal/LintUnused.scala +++ b/main/src/main/scala/sbt/internal/LintUnused.scala @@ -47,6 +47,7 @@ object LintUnused { sbt.nio.Keys.outputFileStamper, sbt.nio.Keys.watchTriggers, serverConnectionType, + serverPort, serverIdleTimeout, shellPrompt, sLog, diff --git a/main/src/main/scala/sbt/internal/server/LanguageServerProtocol.scala b/main/src/main/scala/sbt/internal/server/LanguageServerProtocol.scala index 4ea284708..77f79d1d1 100644 --- a/main/src/main/scala/sbt/internal/server/LanguageServerProtocol.scala +++ b/main/src/main/scala/sbt/internal/server/LanguageServerProtocol.scala @@ -41,12 +41,22 @@ private[sbt] object LanguageServerProtocol { ServerCapabilities( textDocumentSync = TextDocumentSyncOptions(true, 0, false, false, SaveOptions(false)), hoverProvider = false, - definitionProvider = true + definitionProvider = false ) } def handler(converter: FileConverter): ServerHandler = ServerHandler { callback => import callback.* + + def checkAuthenticated(r: JsonRpcRequestMessage)(f: => Unit): Unit = + if !isAuthenticated then + jsonRpcRespondError( + Some(r.id), + ErrorCodes.InvalidRequest, + s"'${r.method}' is not allowed before authentication." + ) + else f + ServerIntent( onRequest = { case r: JsonRpcRequestMessage if r.method == "initialize" => @@ -68,32 +78,37 @@ private[sbt] object LanguageServerProtocol { if (!opt.skipAnalysis.getOrElse(false)) appendExec("collectAnalyses", None) jsonRpcRespond(InitializeResult(serverCapabilities), Some(r.id)) - case r: JsonRpcRequestMessage if r.method == "textDocument/definition" => - val _ = Definition.lspDefinition(json(r), r.id, CommandSource(name), converter, log)(using - StandardMain.executionContext - ) - case r: JsonRpcRequestMessage if r.method == "sbt/exec" => - val param = Converter.fromJson[SbtExecParams](json(r)).get - val _ = appendExec(param.commandLine, Some(r.id)) + checkAuthenticated(r) { + val param = Converter.fromJson[SbtExecParams](json(r)).get + val _ = appendExec(param.commandLine, Some(r.id)) + } case r: JsonRpcRequestMessage if r.method == "sbt/setting" => - val param = Converter.fromJson[Q](json(r)).get - onSettingQuery(Option(r.id), param) + checkAuthenticated(r) { + val param = Converter.fromJson[Q](json(r)).get + onSettingQuery(Option(r.id), param) + } case r: JsonRpcRequestMessage if r.method == "sbt/cancelRequest" => - val param = Converter.fromJson[CancelRequestParams](json(r)).get - onCancellationRequest(Option(r.id), param) + checkAuthenticated(r) { + val param = Converter.fromJson[CancelRequestParams](json(r)).get + onCancellationRequest(Option(r.id), param) + } case r: JsonRpcRequestMessage if r.method == "sbt/completion" => - val param = Converter.fromJson[CP](json(r)).get - onCompletionRequest(Option(r.id), param) + checkAuthenticated(r) { + val param = Converter.fromJson[CP](json(r)).get + onCompletionRequest(Option(r.id), param) + } }, onResponse = PartialFunction.empty, onNotification = { case n: JsonRpcNotificationMessage if n.method == "textDocument/didSave" => - val _ = appendExec(";Test/compile; collectAnalyses", None) + if (isAuthenticated) { + val _ = appendExec(";Test/compile; collectAnalyses", None) + } else log.warn(s"ignoring '${n.method}' before authentication") } ) } diff --git a/main/src/main/scala/sbt/internal/server/NetworkChannel.scala b/main/src/main/scala/sbt/internal/server/NetworkChannel.scala index 5e57878a5..626fa3356 100644 --- a/main/src/main/scala/sbt/internal/server/NetworkChannel.scala +++ b/main/src/main/scala/sbt/internal/server/NetworkChannel.scala @@ -81,6 +81,7 @@ final class NetworkChannel( private val delimiter: Byte = '\n'.toByte private val out = connection.getOutputStream private var initialized = false + private var authenticated = false /** * Reference to the client-side custom options @@ -145,6 +146,7 @@ final class NetworkChannel( def name: String = self.name private[sbt] def authOptions: Set[ServerAuthentication] = self.authOptions private[sbt] def authenticate(token: String): Boolean = self.authenticate(token) + private[sbt] def isAuthenticated: Boolean = self.isAuthenticated private[sbt] def setInitialized(value: Boolean): Unit = self.setInitialized(value) private[sbt] def setInitializeOption(opts: InitializeOption): Unit = self.setInitializeOption(opts) @@ -169,7 +171,14 @@ final class NetworkChannel( private[sbt] def subscribeToAll: Boolean = Option(initializeOption.get).flatMap(_.subscribeToAll).getOrElse(false) - protected def authenticate(token: String): Boolean = instance.authenticate(token) + protected def authenticate(token: String): Boolean = { + val result = instance.authenticate(token) + if result then authenticated = true + result + } + + private[sbt] def isAuthenticated: Boolean = + authenticated || authOptions.isEmpty protected def setInitialized(value: Boolean): Unit = initialized = value diff --git a/main/src/main/scala/sbt/internal/server/VirtualTerminal.scala b/main/src/main/scala/sbt/internal/server/VirtualTerminal.scala index 9cc12a910..6f124dc40 100644 --- a/main/src/main/scala/sbt/internal/server/VirtualTerminal.scala +++ b/main/src/main/scala/sbt/internal/server/VirtualTerminal.scala @@ -12,6 +12,7 @@ package server import java.util.concurrent.{ ArrayBlockingQueue, ConcurrentHashMap } import java.util.UUID +import sbt.internal.langserver.ErrorCodes import sbt.internal.protocol.{ JsonRpcNotificationMessage, JsonRpcRequestMessage, @@ -196,13 +197,20 @@ object VirtualTerminal { private val requestHandler: Handler[JsonRpcRequestMessage] = callback => { case r if r.method == attach => - val isInteractive = r.params - .flatMap(Converter.fromJson[Attach](_).toOption.map(_.interactive)) - .exists(identity) - StandardMain.exchange.channelForName(callback.name) match { - case Some(nc: NetworkChannel) => nc.setInteractive(r.id, isInteractive) - case _ => - } + if (callback.isAuthenticated) { + val isInteractive = r.params + .flatMap(Converter.fromJson[Attach](_).toOption.map(_.interactive)) + .exists(identity) + StandardMain.exchange.channelForName(callback.name) match { + case Some(nc: NetworkChannel) => nc.setInteractive(r.id, isInteractive) + case _ => + } + } else + callback.jsonRpcRespondError( + Some(r.id), + ErrorCodes.InvalidRequest, + s"'$attach' is not allowed before authentication." + ) } private val responseHandler: Handler[JsonRpcResponseMessage] = callback => { @@ -265,12 +273,14 @@ object VirtualTerminal { private val notificationHandler: Handler[JsonRpcNotificationMessage] = callback => { case n if n.method == systemIn => - import sjsonnew.BasicJsonProtocol.* - n.params.flatMap(Converter.fromJson[Byte](_).toOption).foreach { byte => - StandardMain.exchange.channelForName(callback.name) match { - case Some(nc: NetworkChannel) => nc.write(byte) - case _ => + if (callback.isAuthenticated) { + import sjsonnew.BasicJsonProtocol.* + n.params.flatMap(Converter.fromJson[Byte](_).toOption).foreach { byte => + StandardMain.exchange.channelForName(callback.name) match { + case Some(nc: NetworkChannel) => nc.write(byte) + case _ => + } } - } + } else callback.log.warn(s"ignoring '$systemIn' before authentication") } } diff --git a/server-test/src/server-test/tcp/build.sbt b/server-test/src/server-test/tcp/build.sbt new file mode 100644 index 000000000..067d9d87c --- /dev/null +++ b/server-test/src/server-test/tcp/build.sbt @@ -0,0 +1,9 @@ +scalaVersion := "3.8.4" + +Global / serverConnectionType := ConnectionType.Tcp +Global / serverPort := 5002 + +lazy val root = (project in file(".")) + .settings( + name := "tcp", + ) diff --git a/server-test/src/test/scala/testpkg/ExecRequiresInitializeTest.scala b/server-test/src/test/scala/testpkg/ExecRequiresInitializeTest.scala new file mode 100644 index 000000000..63e5bd11d --- /dev/null +++ b/server-test/src/test/scala/testpkg/ExecRequiresInitializeTest.scala @@ -0,0 +1,132 @@ +/* + * sbt + * Copyright 2011 - 2018, Lightbend, Inc. + * Copyright 2008 - 2010, Mark Harrah + * Licensed under Apache License 2.0 (see LICENSE) + */ + +package testpkg + +import java.io.File +import java.nio.file.{ Files, Path } +import scala.concurrent.duration.* + +import sbt.internal.langserver.{ CancelRequestParams, ErrorCodes, SbtExecParams } +import sbt.internal.langserver.codec.JsonProtocol.given +import sbt.protocol.{ Attach, CompletionParams, SettingQuery } +import sbt.protocol.codec.JsonProtocol.given +import sbt.io.IO +import sbt.io.syntax.* +import sbt.protocol.ServerSession +import sbt.{ ForkOptions, OutputStrategy, RunFromSourceMain } +import sjsonnew.JsonWriter + +import org.scalatest.funsuite.AnyFunSuite + +/** + * Reproduces the reported vulnerability: a TCP server configured with token auth + * (the default whenever `serverConnectionType` is Tcp) must reject requests that + * mutate or read through the server from a client that never completed a + * token-authenticated `initialize`. Unlike the other tests in this suite, these + * deliberately skip `ServerSession#initialize` to play the part of an attacker who + * can reach the socket but does not know the token. + */ +class ExecRequiresInitializeTest extends AnyFunSuite { + private val testDirectory = "tcp" + + private val serverTestBase: File = { + val p0 = new File(".").getAbsoluteFile / "server-test" / "src" / "server-test" + val p1 = new File(".").getAbsoluteFile / "src" / "server-test" + if (p0.exists) p0 else p1 + } + + /** Forks a real sbt server for `testDirectory`, connects a raw (un-initialized) session. */ + private def withUnauthenticatedSession(f: ServerSession => Unit): Unit = { + val base: Path = Files.createTempDirectory(Path.of("/tmp"), "sbt-tcp-poc") + val buildDir = base.toFile / testDirectory + IO.copyDirectory(serverTestBase / testDirectory, buildDir) + info(s"test project created at: $buildDir") + + val classpath = TestProperties.classpath.split(File.pathSeparator).map(new File(_)) + val process = RunFromSourceMain.fork( + ForkOptions() + .withOutputStrategy(OutputStrategy.StdoutOutput) + .withRunJVMOptions( + Vector( + "-Djline.terminal=none", + "-Dsbt.io.virtual=false", + "-Dsbt.banner=false", + ) + ), + buildDir, + TestProperties.scalaVersion, + TestProperties.version, + classpath.toSeq + ) + + try { + val portfile = buildDir / "project" / "target" / "active.json" + ServerSession.waitForPortfile(portfile, process.isAlive()) + + val session = ServerSession.connect(portfile) + try + // Deliberately do NOT call session.initialize(...): this simulates an + // attacker who can reach the TCP socket but never authenticates. + f(session) + finally session.close() + } finally { + if (process.isAlive()) process.destroy() + IO.delete(base.toFile) + } + } + + /** Sends `method`/`params` on `session` and asserts the server rejected it pre-auth. */ + private def assertRejected[A: JsonWriter]( + session: ServerSession, + method: String, + params: A + ): Unit = { + val id = session.nextId() + session.sendJsonRpc(id, method, params).get + val response = session.waitForResponseMsg(30.seconds, id).get + + assert( + response.error.isDefined, + s"$method should have been rejected before initialize, but got: $response" + ) + assertResult(ErrorCodes.InvalidRequest)(response.error.get.code) + } + + test("sbt/exec is rejected over TCP before a token-authenticated initialize") { + withUnauthenticatedSession { session => + assertRejected(session, "sbt/exec", SbtExecParams("compile")) + } + } + + test("sbt/setting is rejected over TCP before a token-authenticated initialize") { + withUnauthenticatedSession { session => + assertRejected(session, "sbt/setting", SettingQuery("root/name")) + } + } + + test("sbt/cancelRequest is rejected over TCP before a token-authenticated initialize") { + withUnauthenticatedSession { session => + assertRejected(session, "sbt/cancelRequest", CancelRequestParams("some-id")) + } + } + + test("sbt/completion is rejected over TCP before a token-authenticated initialize") { + withUnauthenticatedSession { session => + assertRejected(session, "sbt/completion", CompletionParams("comp", None)) + } + } + + test("sbt/attach is rejected over TCP before a token-authenticated initialize") { + // Regression for the interactive-attach bypass: without this gate, a client could + // attach and feed raw command bytes via sbt/systemIn, running commands without + // ever completing the token handshake that sbt/exec itself requires. + withUnauthenticatedSession { session => + assertRejected(session, "sbt/attach", Attach(interactive = true)) + } + } +}