Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-5vcm-3xc3-w7x3
Response splitting mitigation, 1.0
  • Loading branch information
rossabaker committed Sep 21, 2021
2 parents e26c789 + 6dd04b5 commit d02007d
Show file tree
Hide file tree
Showing 23 changed files with 656 additions and 192 deletions.
Expand Up @@ -33,6 +33,7 @@ import org.http4s.blazecore.{Http1Stage, IdleTimeoutStage}
import org.http4s.blazecore.util.Http1Writer
import org.http4s.client.RequestKey
import org.http4s.headers.{Connection, Host, `Content-Length`, `User-Agent`}
import org.http4s.internal.CharPredicate
import org.http4s.util.{StringWriter, Writer}
import org.typelevel.vault._
import scala.annotation.tailrec
Expand Down Expand Up @@ -432,6 +433,8 @@ private final class Http1Connection[F[_]](
else
Left(new IllegalArgumentException("Host header required for HTTP/1.1 request"))
else if (req.uri.path == Uri.Path.empty) Right(req.withUri(req.uri.copy(path = Uri.Path.Root)))
else if (req.uri.path.renderString.exists(ForbiddenUriCharacters))
Left(new IllegalArgumentException(s"Invalid URI path: ${req.uri.path}"))
else Right(req) // All appears to be well
}

Expand Down Expand Up @@ -473,4 +476,6 @@ private object Http1Connection {
writer
} else writer
}

private val ForbiddenUriCharacters = CharPredicate(0x0.toChar, ' ', '\r', '\n')
}
10 changes: 6 additions & 4 deletions blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala
Expand Up @@ -293,17 +293,19 @@ object Http1Stage {
Future.successful(buffer)
} else CachedEmptyBufferThunk

/** Encodes the headers into the Writer. Does not encode `Transfer-Encoding` or
* `Content-Length` headers, which are left for the body encoder. Adds
* `Date` header if one is missing and this is a server response.
/** Encodes the headers into the Writer. Does not encode
* `Transfer-Encoding` or `Content-Length` headers, which are left
* for the body encoder. Does not encode headers with invalid
* names. Adds `Date` header if one is missing and this is a server
* response.
*
* Note: this method is very niche but useful for both server and client.
*/
def encodeHeaders(headers: Iterable[Header.Raw], rr: Writer, isServer: Boolean): Unit = {
var dateEncoded = false
val dateName = Header[Date].name
headers.foreach { h =>
if (h.name != `Transfer-Encoding`.name && h.name != `Content-Length`.name) {
if (h.name != `Transfer-Encoding`.name && h.name != `Content-Length`.name && h.isNameValid) {
if (isServer && h.name == dateName) dateEncoded = true
rr << h << "\r\n"
}
Expand Down
Expand Up @@ -222,7 +222,7 @@ private[blaze] class Http1ServerStage[F[_]](
resp: Response[F],
bodyCleanup: () => Future[ByteBuffer]): Unit = {
val rr = new StringWriter(512)
rr << req.httpVersion << ' ' << resp.status.code << ' ' << resp.status.reason << "\r\n"
rr << req.httpVersion << ' ' << resp.status << "\r\n"

Http1Stage.encodeHeaders(resp.headers.headers, rr, isServer = true)

Expand Down
Expand Up @@ -19,7 +19,7 @@ package blaze
package server

import cats.data.Kleisli
import cats.syntax.eq._
import cats.syntax.all._
import cats.effect._
import cats.effect.kernel.Deferred
import cats.effect.std.Dispatcher
Expand Down Expand Up @@ -535,35 +535,79 @@ class Http1ServerStageSpec extends Http4sSuite {
}
}

fixture.test("Http1ServerStage: don't deadlock TickWheelExecutor with uncancelable request") {
tw =>
val reqUncancelable = List("GET /uncancelable HTTP/1.0\r\n\r\n")
val reqCancelable = List("GET /cancelable HTTP/1.0\r\n\r\n")

(for {
uncancelableStarted <- Deferred[IO, Unit]
uncancelableCanceled <- Deferred[IO, Unit]
cancelableStarted <- Deferred[IO, Unit]
cancelableCanceled <- Deferred[IO, Unit]
app = HttpApp[IO] {
case req if req.pathInfo === path"/uncancelable" =>
uncancelableStarted.complete(()) *>
IO.uncancelable { poll =>
poll(uncancelableCanceled.complete(())) *>
cancelableCanceled.get
}.as(Response[IO]())
case _ =>
cancelableStarted.complete(()) *> IO.never.guarantee(
cancelableCanceled.complete(()).void)
}
head <- IO(runRequest(tw, reqUncancelable, app))
_ <- uncancelableStarted.get
_ <- uncancelableCanceled.get
_ <- IO(head.sendInboundCommand(Disconnected))
head2 <- IO(runRequest(tw, reqCancelable, app))
_ <- cancelableStarted.get
_ <- IO(head2.sendInboundCommand(Disconnected))
_ <- cancelableCanceled.get
} yield ()).assert
fixture.test("Prevent response splitting attacks on status reason phrase") { tw =>
val rawReq = "GET /?reason=%0D%0AEvil:true%0D%0A HTTP/1.0\r\n\r\n"
val head = runRequest(
tw,
List(rawReq),
HttpApp { req =>
Response[IO](Status.NoContent.withReason(req.params("reason"))).pure[IO]
})
head.result.map { buff =>
val (_, headers, _) = ResponseParser.parseBuffer(buff)
assertEquals(headers.find(_.name === ci"Evil"), None)
}
}

fixture.test("Prevent response splitting attacks on field name") { tw =>
val rawReq = "GET /?fieldName=Fine:%0D%0AEvil:true%0D%0A HTTP/1.0\r\n\r\n"
val head = runRequest(
tw,
List(rawReq),
HttpApp { req =>
Response[IO](Status.NoContent).putHeaders(req.params("fieldName") -> "oops").pure[IO]
})
head.result.map { buff =>
val (_, headers, _) = ResponseParser.parseBuffer(buff)
assertEquals(headers.find(_.name === ci"Evil"), None)
}
}

fixture.test("Prevent response splitting attacks on field value") { tw =>
val rawReq = "GET /?fieldValue=%0D%0AEvil:true%0D%0A HTTP/1.0\r\n\r\n"
val head = runRequest(
tw,
List(rawReq),
HttpApp { req =>
Response[IO](Status.NoContent)
.putHeaders("X-Oops" -> req.params("fieldValue"))
.pure[IO]
})
head.result.map { buff =>
val (_, headers, _) = ResponseParser.parseBuffer(buff)
assertEquals(headers.find(_.name === ci"Evil"), None)
}

fixture.test("Http1ServerStage: don't deadlock TickWheelExecutor with uncancelable request") {
tw =>
val reqUncancelable = List("GET /uncancelable HTTP/1.0\r\n\r\n")
val reqCancelable = List("GET /cancelable HTTP/1.0\r\n\r\n")

(for {
uncancelableStarted <- Deferred[IO, Unit]
uncancelableCanceled <- Deferred[IO, Unit]
cancelableStarted <- Deferred[IO, Unit]
cancelableCanceled <- Deferred[IO, Unit]
app = HttpApp[IO] {
case req if req.pathInfo === path"/uncancelable" =>
uncancelableStarted.complete(()) *>
IO.uncancelable { poll =>
poll(uncancelableCanceled.complete(())) *>
cancelableCanceled.get
}.as(Response[IO]())
case _ =>
cancelableStarted.complete(()) *> IO.never.guarantee(
cancelableCanceled.complete(()).void)
}
head <- IO(runRequest(tw, reqUncancelable, app))
_ <- uncancelableStarted.get
_ <- uncancelableCanceled.get
_ <- IO(head.sendInboundCommand(Disconnected))
head2 <- IO(runRequest(tw, reqCancelable, app))
_ <- cancelableStarted.get
_ <- IO(head2.sendInboundCommand(Disconnected))
_ <- cancelableCanceled.get
} yield ()).assert
}
}
}
10 changes: 0 additions & 10 deletions build.sbt
Expand Up @@ -448,12 +448,6 @@ lazy val emberServer = libraryProject("ember-server", CrossType.Full, List(JVMPl
log4catsSlf4j.value,
javaWebSocket % Test
),
mimaBinaryIssueFilters ++= Seq(
ProblemFilters.exclude[DirectMissingMethodProblem]("org.http4s.ember.server.EmberServerBuilder#Defaults.maxConcurrency"),
ProblemFilters.exclude[DirectMissingMethodProblem]("org.http4s.ember.server.internal.ServerHelpers.isKeepAlive"),
ProblemFilters.exclude[DirectMissingMethodProblem]("org.http4s.ember.server.EmberServerBuilder#Defaults.maxConcurrency"),
ProblemFilters.exclude[IncompatibleMethTypeProblem]("org.http4s.ember.server.internal.ServerHelpers.runApp")
),
Test / parallelExecution := false
)
.jsSettings(
Expand All @@ -475,9 +469,6 @@ lazy val emberClient = libraryProject("ember-client", CrossType.Full, List(JVMPl
description := "ember implementation for http4s clients",
startYear := Some(2019),
libraryDependencies += keypool.value,
mimaBinaryIssueFilters := Seq(
ProblemFilters.exclude[DirectMissingMethodProblem]("org.http4s.ember.client.EmberClientBuilder.this")
)
)
.jvmSettings(libraryDependencies += log4catsSlf4j.value)
.jsSettings(
Expand Down Expand Up @@ -508,7 +499,6 @@ lazy val blazeClient = libraryProject("blaze-client")
.settings(
description := "blaze implementation for http4s clients",
startYear := Some(2014),
mimaBinaryIssueFilters ++= Seq()
)
.dependsOn(blazeCore % "compile;test->test", client % "compile;test->test")

Expand Down
Expand Up @@ -19,15 +19,16 @@ package client

import cats.effect._
import cats.syntax.all._
import com.comcast.ip4s.Host
import com.comcast.ip4s.SocketAddress
import com.comcast.ip4s.{Host, SocketAddress}
import fs2._
import org.http4s.client.dsl.Http4sClientDsl
import org.http4s.client.testroutes.GetRoutes
import org.http4s.dsl.io._
import org.http4s.implicits._
import org.http4s.multipart.Multipart
import org.http4s.multipart.Part
import org.http4s.server.Server
import org.typelevel.ci._

import java.util.Arrays
import java.util.Locale
Expand Down Expand Up @@ -117,6 +118,56 @@ abstract class ClientRouteTestBattery(name: String) extends Http4sSuite with Htt
}
}

test("Mitigates request splitting attack in URI path") {
for {
uri <- url("/").map(
_.withPath(Uri.Path(Vector(Uri.Path.Segment.encoded(
"request-splitting HTTP/1.0\r\nEvil:true\r\nHide-Protocol-Version:")))))
req = Request[IO](uri = uri)
c <- client()
status <- c.status(req).handleError(_ => Status.Ok)
} yield assertEquals(status, Status.Ok)
}

test("Mitigates request splitting attack in URI RegName") {
for {
srv <- server()
address = srv.address
hostname = address.host.toString
port = address.port.value
req = Request[IO](
uri = Uri(
authority = Uri
.Authority(None, Uri.RegName(s"${hostname}\r\nEvil:true\r\n"), port = port.some)
.some,
path = path"/request-splitting"))
c <- client()
status <- c.status(req).handleError(_ => Status.Ok)
} yield assertEquals(status, Status.Ok)
}

test("Mitigates request splitting attack in field name") {

for {
srv <- server()
uri <- url("/request-splitting")
req = Request[IO](uri = uri)
.putHeaders(Header.Raw(ci"Fine:\r\nEvil:true\r\n", "oops"))
c <- client()
status <- c.status(req).handleError(_ => Status.Ok)
} yield assertEquals(status, Status.Ok)
}

test("Mitigates request splitting attack in field value") {
for {
uri <- url("/request-splitting")
req = Request[IO](uri = uri)
.putHeaders(Header.Raw(ci"X-Carrier", "\r\nEvil:true\r\n"))
c <- client()
status <- c.status(req).handleError(_ => Status.Ok)
} yield assertEquals(status, Status.Ok)
}

private def checkResponse(rec: Response[IO], expected: Response[IO]): IO[Boolean] = {
// This isn't a generically safe normalization for all header, but
// it's close enough for our purposes
Expand Down
23 changes: 20 additions & 3 deletions core/shared/src/main/scala/org/http4s/Header.scala
Expand Up @@ -20,7 +20,8 @@ import cats.{Foldable, Hash, Order, Semigroup, Show}
import cats.data.NonEmptyList
import cats.syntax.all._
import org.typelevel.ci.CIString
import org.http4s.util.{Renderer, Writer}
import org.http4s.internal.CharPredicate
import org.http4s.util.{Renderer, StringWriter, Writer}
import cats.data.Ior

/** Typeclass representing an HTTP header, which all the http4s
Expand Down Expand Up @@ -49,6 +50,17 @@ trait Header[A, T <: Header.Type] {
object Header {
final case class Raw(val name: CIString, val value: String) {
override def toString: String = s"${name}: ${value}"

/** True if [[name]] is a valid field-name per RFC7230. Where it
* is not, the header may be dropped by the backend.
*/
def isNameValid: Boolean =
name.toString.nonEmpty && name.toString.forall(FieldNamePredicate)

def sanitizedValue: String = {
val w = new StringWriter
w.sanitize(_ << value).result
}
}

object Raw {
Expand All @@ -66,8 +78,10 @@ object Header {
case c => c
}

def render(writer: Writer, h: Raw): writer.type =
writer << h.name << ':' << ' ' << h.value
def render(writer: Writer, h: Raw): writer.type = {
writer << h.name << ':' << ' '
writer.sanitize(_ << h.value)
}
}
}

Expand Down Expand Up @@ -235,4 +249,7 @@ object Header {
}
}
}

private val FieldNamePredicate =
CharPredicate("!#$%&'*+-.^_`|~`") ++ CharPredicate.AlphaNum
}

0 comments on commit d02007d

Please sign in to comment.