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

[receiver/otlp] CWE-1327 in OTLP receiver #6151

Closed
mx-psi opened this issue Sep 26, 2022 · 4 comments · Fixed by #6267
Closed

[receiver/otlp] CWE-1327 in OTLP receiver #6151

mx-psi opened this issue Sep 26, 2022 · 4 comments · Fixed by #6267

Comments

@mx-psi
Copy link
Member

mx-psi commented Sep 26, 2022

Introduction

Recently there was a CVE on the Go runtime (CVE-2022-27664) that could be leveraged to cause a denial of service on applications built with affected runtimes through a faulty HTTP/2 connection. We addressed this CVE on the official binaries by bumping the Go version (see open-telemetry/opentelemetry-collector-releases/pull/198).

This CVE impacted in particular the OTLP receiver. While some level of impact was unavoidable on this receiver since exposing an HTTP/2 server is part of its core functionality, the OTLP receiver default endpoints (0.0.0.0:4317 and 0.0.0.0:4318) made it so that this vulnerability could be leveraged more easily than it could have been. In particular, these endpoints fall prey to CWE-1327 ("Binding to an Unrestricted IP Address"), instead of using a more restricted address.

My intention with this issue is to discuss if we can in some way mitigate the impact of future CVEs for OTLP receiver users through either code changes or improved documentation, by effectively making so that less users use 0.0.0.0 when they don't actually need to be so unrestricted.

Possible ways to address this

We could do all, none or any other combination of the proposals below.

Change the default endpoint to use localhost instead of 0.0.0.0

Description: Change the default endpoint to be localhost:4317 and localhost:4318 for OTLP/gRPC and OTLP/HTTP receivers respectively.

Advantages:

  1. It fits with the OpenTelemetry "secure by default" philosophy.
  2. It is a reasonable default for some use cases (e.g. running the bare binary).

Downsides:

  1. It is a breaking change for some users. We define the OTLP receiver configuration schema to be 'stable'. While the definition leaves some leeway for breaking changes under special circumstances, we don't have a consensus on what these circumstances should be.
  2. This default will not work as nicely in some use cases (e.g. running in a containerized setup).

Add a warning when the endpoint is 0.0.0.0

Description: Add a warning message when using an 0.0.0.0 endpoint.

Advantages:

  1. Can be done in a backwards compatible way.

Downsides:

  1. Most people (probably) don't read the logs.
  2. It's hard (impossible?) to show this warning only when it's really useful. This would be logged in cases where no action is needed (e.g. containerized setups).

Document security recommendations for these endpoints

Description: We add a section to our security recommendations recommending users to change the endpoint to limit exposure.

Advantages:

  1. Can be done in a backwards compatible way.

Downsides:

  1. Most people (probably) don't read the security documentation.
@jpkrohling
Copy link
Member

It fits with the OpenTelemetry "secure by default" philosophy.

We could argue that this is our intention and that having a less-secure option should be considered a bug, justifying the break in backward compatibility. We should still follow the deprecation process, letting users know the new default will kick in soon.

@Aneurysm9
Copy link
Member

What would the mechanics of following the deprecation process look like here? Start with warnings and then make the change? Would this be a situation where a feature gate for the changed behavior is appropriate? That would allow us to warn and let users start to opt-in for testing in release N, change the default behavior in N+1 or N+2 while still allowing users to opt-out and get the old behavior, and then eventually remove the gate entirely when we're confident it's no longer needed.

In any event, I think we probably want to do all three of the things that @mx-psi has suggested. We should change the default, warn when the listening address is 0.0.0.0 in case a user has explicitly set that value, and document that it should not be done without consideration of the security implications.

@codeboten
Copy link
Contributor

As a starting point, let's warn to tell users they should configure an endpoint. This warning should link to a security best practices section in our documentation to allow them to act on it.

@mx-psi
Copy link
Member Author

mx-psi commented Sep 30, 2022

First stab at security recommendations doc over at #6212, please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants