Security issue: recording query params insecure (tokens)

[codyaray]

What version of OpenCensus are you using?

go.opencensus.io v0.23.0

What version of Go are you using?

Varies between services, but ranges from 1.16 to 1.18 mostly

What did you do?

With a service that uses ochttp, made an HTTP request with a secret in the query param, and check the http.url span

What did you expect to see?

Ideally we wouldn't have the secret logged at all. In this case, that means no http.url span... just the http.path span.

(Obviously the implicit assumption here is that all secrets must be in query params rather than paths)

What did you see instead?

I could see the secret, in all its glory

Additional context

Normally I agree about avoiding secrets in URLs entirely (path or query), but one of the most common usages is tokens for email validation, password resets, and similar which are click-through from emails. Unfortunately, emails can't add custom HTTP headers so this means that the token has to be in the URL. And yes, though these are generally considered one-time tokens, any issue could mean we can't guarantee that there will never be an error response returned without marking the token as invalidated.

So to be on the safe side our security team is demanding that we remove this parameter.

Now that leaves us with a few options:

1. fork this library just to remove this ourselves - obviously we'd like to stay on upstream
2. use the IsHealthEndpoint as a hack to disable tracing for these endpoints - but then we lose observability into common workflows (e.g., password resets, email validation, user signup, etc)
3. add some sort of thing between the services and the eventual destination (DataDog in our case) that can remove these attributes from all spans.
4. see if we can get this change made in the core ochttp plugin and bump our versions - this issue :)

[dashpole]

Are you using opencensus for traces, or metrics, or both? OpenTelemetry is the successor project to this one, and we would prefer not to add functionality to this repo if possible.

Where is the log line in question?

[codyaray]

Traces. This is the http.url span attribute.

Here's the line in question that logs the full URL instead of just the URL path: https://github.com/census-instrumentation/opencensus-go/blob/master/plugin/ochttp/trace.go#L159. (There's a separate http.path attriibute)

Interestingly, http.url was actually removed here to be compatible with the spec: #493

But I think it was accidentally re-introduced here: https://github.com/census-instrumentation/opencensus-go/pull/928/files#diff-4f3bb36754d7aefaf1db34617488498a4968dd75a4ef66e2a25597cd258404edR158

The most minimal fix wouldn't be adding functionality, it would just be removing the http.url attribute (so removing 2 lines plus fixing tests). We're happy to send a PR if this is acceptable.

[dashpole]

Unfortunately, we aren't able to make breaking changes. We might be able to remove the attribute with some (default off) configuration.

For traces, OpenTelemetry go is a stable alternative that might already handle this case: https://github.com/open-telemetry/opentelemetry-go/blob/main/semconv/internal/http.go#L150. Would something like that fix your use-case?

[codyaray]

Ironically opentelemetry has this same issue (http.target has query params): spec and code - it still records the entire URL.String() which includes (potentially sensitive) query params.

We might be able to remove the attribute with some (default off) configuration.

Can you tell me more about how this would work?

[dashpole]

There are options on https://github.com/census-instrumentation/opencensus-go/blob/master/plugin/ochttp/server.go#L44. In theory, we could add an option to disable setting certain attributes.

Do you know what particular pieces of the URL string is problematic? Otel does nil-out the request.URL.User portion since it seems to often contain passwords. Maybe there are other particular parts of the URL that are known to contain sensitive information?

But i'm not sure that is something we should solve here, since this repository is in maintenance mode, and you are only using tracing (which is GA in OTel). It would be better to solve the issue in OpenTelemetry than to make changes here.

[codyaray]

Do you know what particular pieces of the URL string is problematic?

Yes, it's an issue with tokens in the query string. For example, you could imagine the link in a "Password Reset" email
to validate a password reset being a link in an email like

<a href="https://example.com/api/reset_password?token=sdijfi8h2rh3lkfh">Reset My Password!</a>

This token query parameter is then included as an attribute in the emitted logs.

Otel does nil-out the request.URL.User portion since it seems to often contain passwords.

Both OpenCensus and OTel include the query string in attributes.

Unfortunately this is a high-severity incident at my workplace, so "migrate everything to opentelemetry" doesn't satisfy the urgency needed there. (Separately, we are already working to migrate to opentelemetry)

Right now we're hotfixing all of our services to use the IsHealthEndpoint to fix this for the known endpoints with secrets and dropping all visibility on certain endpoints. 😭

[codyaray]

If adding this option is a real option here (meaning: you would accept it), that would be super helpful so that we're not flying blind. I'm not going to do it tonight, but we might explore it tomorrow if you allow.

We couldn't get a fork in time to patch this issue tonight anyway. (Internal IT/legal/compliance/whatever process)