Forbid setting the Location and Refresh custom response headers

[legrego]

Summary

The server.customResponseHeaders allows administrators to configure a custom set of headers that the Kibana server will send with every response. We have historically not had much validation around this because we can't know all the valid use cases for this feature in advance.

Two headers that I don't see a valid need for is Location and Refresh -- setting these response headers would force the browser to redirect every response to a fixed value, which does not make a whole lot of sense.

This PR adds a deny list of headers, with Location and Refresh specified

[jportner]

I tried several ways to circumvent this restriction:

• Adding the location header with different suffixes (:, ., -, etc)
• Adding the location header with different prefixes
• HTTP response splitting with CRLF characters (reference)

All of these attempts were unsuccessful because Node is pretty strict with HTTP header key/value validation (https://github.com/nodejs/node/blob/ee9e2a2eb6143fa7b4b1454f7aed009a8703d4d7/lib/_http_common.js#L214-L233 via https://github.com/nodejs/node/blob/ee9e2a2eb6143fa7b4b1454f7aed009a8703d4d7/lib/_http_outgoing.js#L556-L570).

However, I did find one alternative way to control where the browser goes:

server.customResponseHeaders:
  refresh: "0; URL=https://example.com"

So I think this denylist approach will work, but perhaps we should take a step back and evaluate all HTTP response headers. At a minimum it sounds like we will need to have location and refresh in the denylist.

[legrego]

So I think this denylist approach will work, but perhaps we should take a step back and evaluate all HTTP response headers. At a minimum it sounds like we will need to have location and refresh in the denylist.

Thanks for testing this Joe, much appreciated! I agree we should add refresh to the list. I thought I had evaluated the response headers, but missed that one. I debated adding Content-Location to the list, but I don't think that's necessary based on my understanding of that header.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: a53b0db
• Storybooks Preview

Metrics [docs]

✅ unchanged

History

• 💛 Build #122038 was flaky 0fec11b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[kibanamachine]

💔 Backport failed

Status	Branch	Result
❌	7.12	Commit could not be cherrypicked due to conflicts
✅	7.13
✅	7.x

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 98129

[legrego]

Manual backport to 7.12.2: #98206

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)