Skip to content
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

Prevent the serialization of header containing a line terminator #3717

Closed
gontard opened this issue Jan 4, 2021 · 6 comments
Closed

Prevent the serialization of header containing a line terminator #3717

gontard opened this issue Jan 4, 2021 · 6 comments
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on t:core Issues related to the akka-http-core module t:model Issues around the model classes and its functionality
Milestone

Comments

@gontard
Copy link

gontard commented Jan 4, 2021

akka-http should throw an Exception when an header value containing a line terminator (LF) is sent in a response

For example

package com.example

import akka.actor.typed.ActorSystem
import akka.actor.typed.scaladsl.Behaviors
import akka.http.scaladsl.Http
import akka.http.scaladsl.model._
import akka.http.scaladsl.model.headers.{ModeledCustomHeader, ModeledCustomHeaderCompanion}
import akka.http.scaladsl.server.Directives._

import scala.util.Try

object HttpServerRoutingMinimal {
  def main(args: Array[String]): Unit = {
    implicit val system = ActorSystem(Behaviors.empty, "my-system")
    implicit val executionContext = system.executionContext
    val route =
      path("hello") {
        get {
          complete(StatusCodes.OK, Seq[HttpHeader](
            MyHeader("abc\ndef")
          ), "<h1>Say hello to akka-http</h1>")
        }
      }

    Http().newServerAt("localhost", 8080).bind(route)
  }

  final class MyHeader(token: String) extends ModeledCustomHeader[MyHeader] {
    override def renderInRequests = true
    override def renderInResponses = true
    override val companion = MyHeader
    override def value: String = token
  }
  object MyHeader extends ModeledCustomHeaderCompanion[MyHeader] {
    override val name = "my-header"
    override def parse(value: String) = Try(new MyHeader(value))
  }
}

We spotted this issue because our envoy reverse proxy complains of an Invalid HTTP header field was received.

@jrudolph jrudolph added 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on t:core Issues related to the akka-http-core module t:model Issues around the model classes and its functionality labels Feb 1, 2021
@jrudolph
Copy link
Member

jrudolph commented Feb 1, 2021

Good point, thanks for reporting (and sorry for the delay), @gontard.

I don't have a good idea how to check this cheaply. I don't think we want that check for our own modeled headers. It would certainly make sense for RawHeader but not sure where to put this for more generic user-defined modeled headers. Any ideas?

@gontard
Copy link
Author

gontard commented Feb 4, 2021

Thanks for your answer.

I don't think we want that check for our own modeled headers

Is it a performance concern?

Any ideas?

IDK the akka-http internals, but naively i would check that systematically.

@nitikagarw
Copy link
Contributor

Hi @jrudolph,
How about adding the check here and here?

@jrudolph
Copy link
Member

How about adding the check here and here?

It should go into the constructor of ModeledCustomHeader (instead of its companion). We can put a verification method into object HttpHeader to call from the constructors.

@nitikagarw
Copy link
Contributor

Hi @jrudolph, I have created a PR #3763 to discuss the fix.

jrudolph added a commit to jrudolph/akka-http that referenced this issue Oct 25, 2021
Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking
jrudolph added a commit to jrudolph/akka-http that referenced this issue Oct 25, 2021
Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking
@jrudolph jrudolph added this to the 10.2.7 milestone Nov 2, 2021
jrudolph added a commit that referenced this issue Nov 2, 2021
Refs #3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking
@jrudolph
Copy link
Member

jrudolph commented Nov 2, 2021

#3922 adds a general solution that checks headers for invalid characters while rendering and if occurred drops those headers.

@jrudolph jrudolph closed this as completed Nov 2, 2021
jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 18, 2021
…a#3922)

Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking

(cherry picked from commit afecb31)
jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 18, 2021
…a#3922)

Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking

(cherry picked from commit afecb31)
jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 18, 2021
…a#3922)

Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking

(cherry picked from commit afecb31)
jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 18, 2021
…a#3922)

Refs akka#3717

To avoid accidental response splitting when applications put unsanitized request
data into outgoing responses.

We previously discussed introducing a check in the header models which could avoid
overhead during rendering. There are a lot of header models, however, which
all had to be checked one by one for potential issues. This would require a significant
effort now to check all the existing headers and would also make it easy to
reintroduce problems later on when headers are added or changed.

The approach here requires that all headers are rendered using the new overload of
`Rendering.~~`. When that method is used, we render the header and check afterwards
that the rendered data does not contain any CR or LF characters.

The performance overhead seems negligible, the new method did not turn up in profiling.
An explanation why going over the rendered header data directly after it has been rendered
is so cheap could be that
 - it has good locality because the data has just been written
 - it has good branch prediction because test will almost never fail
 - even if it has linear cost for big headers, those big headers will also have significant
   memory allocation cost which might dwarf the extra checking

(cherry picked from commit afecb31)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on t:core Issues related to the akka-http-core module t:model Issues around the model classes and its functionality
Projects
None yet
Development

No branches or pull requests

3 participants