New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.6.x]: Update Akka HTTP to 10.0.14 #8598

Merged
merged 1 commit into from Sep 6, 2018

Conversation

Projects
None yet
4 participants
@marcospereira
Member

marcospereira commented Sep 6, 2018

Purpose

Update Akka HTTP to version 10.0.14.

@dwijnand

LGTM

@marcospereira marcospereira merged commit 416de3b into playframework:2.6.x Sep 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@marcospereira marcospereira added this to the Play 2.6.19 milestone Sep 6, 2018

@marcospereira marcospereira deleted the marcospereira:akka-http-10.0.14 branch Sep 6, 2018

@Ap0c

This comment has been minimized.

Ap0c commented Sep 12, 2018

Hi, would you be able to confirm if this fixes the gzip request vulnerability reported against akka-http? Part of the fix involved adding this line to decodeRequestWith. However, we noticed that Play appears to define its own versions of decodeRequest and decodeRequestWith which don't perform this check:

private def decodeRequestWith(decoderFlow: Flow[ByteString, ByteString, NotUsed], request: HttpRequest): HttpRequest = {
request.withEntity(request.entity.transformDataBytes(decoderFlow))
.withHeaders(request.headers.filterNot(_.isInstanceOf[`Content-Encoding`]))
}
/**
* Decode the request body if it is encoded and we know how to decode it.
*/
def decodeRequest(request: HttpRequest): HttpRequest = {
request.encoding match {
case HttpEncodings.gzip => decodeRequestWith(Compression.gunzip(), request)
case HttpEncodings.deflate => decodeRequestWith(Compression.inflate(), request)
// Handle every undefined decoding as is
case _ => request
}
}

Could Play be independently vulnerable to this problem?

Thanks for your help.

Relevant CVE

@marcospereira

This comment has been minimized.

Member

marcospereira commented Sep 12, 2018

Hi @Ap0c,

Play is not affected by this because it uses body parsers to enforce the max content length, not the underlying server backend (Akka HTTP or Netty). The relevant code for Play is here:

/**
* Enforce the max length on the stream consumed by the given accumulator.
*/
private[play] def enforceMaxLength[A](request: RequestHeader, maxLength: Long, accumulator: Accumulator[ByteString, Either[Result, A]]): Accumulator[ByteString, Either[Result, A]] = {
val takeUpToFlow = Flow.fromGraph(new BodyParsers.TakeUpTo(maxLength))
Accumulator(takeUpToFlow.toMat(accumulator.toSink) { (statusFuture, resultFuture) =>
import play.core.Execution.Implicits.trampoline
val defaultCtx = materializer.executionContext
statusFuture.flatMap {
case MaxSizeExceeded(_) =>
val badResult = Future.successful(()).flatMap(_ => createBadResult("Request Entity Too Large", REQUEST_ENTITY_TOO_LARGE)(request))(defaultCtx)
badResult.map(Left(_))
case MaxSizeNotExceeded => resultFuture
}
})
}

We conduct the same test that was breaking Akka HTTP against a Play application without being able to reproduce the problem. So this is more like a regular update than one fixing a vulnerability.

@rtyley

This comment has been minimized.

Contributor

rtyley commented Sep 12, 2018

Play is not affected by this because it uses body parsers to enforce the max content length

Thanks - and my understanding is that BodyParsers.TakeUpTo(maxLength) is working on the decompressed request body, rather than the incoming gzip'd data, so that makes sense.

@Ap0c

This comment has been minimized.

Ap0c commented Sep 12, 2018

Ah ok, that's good to know, thank you for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment