CVE: Fix Receiver malicious tenant

[danielmellado]

If running as root or with enough privileges, receiver can create a
directory outside of the configured TenantHeader.

This commit fixes it up by sanitizing the user input and explicity not
allowing such behavior.

Signed-off-by: Daniel Mellado [email protected]

[saswatamcode]

So there are a couple of things here. I don't think this is the right place/input to sanitize, as this is the HTTP header name (by default THANOS-TENANT) and this is passed to the Prometheus registry for collecting metrics. We would need to sanitize the value of this HTTP header instead.

To highlight the full flow, this header value is fetched in the receiveHTTP handler, and then goes through handleRequest -> forward -> fanoutForward and passed to h.writer.Write() which creates a multiTSDB TenantAppendable to write data for that tenant. There is some logic (getOrLoadTenant) around how a tenant TSDB is started, but the HTTP header value is eventually used to decide dataDir for the tenant using the defaultTenantDataDir method.

So, we can sanitize this in receiveHTTP itself, but I wonder if we should treat tenant id like a path at all. If tenant header value is set to something like ///, path.Base would return /. Instead maybe we should use some regex that fails if tenant id has some backward/forward slash. WDYT? 🙂

Also, I think we'd need to sanitize --receive.default-tenant-id in a similar way.

[GiedriusS]

What do you think about having an aliased type that would be used for this? For example:

type tenantID string

func (t *tenantID) String() string {
  return string(*t)
}

func NewTenantID(s string) *tenantID {
  s = path.Base(s)
  return &tenantID{s}
}

(untested but shows my idea). We could put this in a separate package so that it would be impossible to use tenantID directly. This way, we would have the cleaning-up logic only in one place.

[saswatamcode]

Yup, I think this would make it way cleaner!

[squat]

Hi all I think that using path.Base is only safe at configuration parse-time. Any time this is used at run-time we are liable to run into very sketchy situations where we think we are safe but we actually aren't. Consider for example nice tenant A and malicious tenant B/A. If we path.Base at run-time and let tenant B/A write data to disk, they will be writing into tenant A's directory.

This means that we cannot just rely on doing path.Base(tenantHeaderValue) to find the directory to write to. We actually have to test path.Base(tenantHeaderValue) == tenantHeaderValue and return a 503 or something if not.

[danielmellado]

Hi o/ thanks for the review! totally, I didn't think of that, but deff can be used for overriding a tenant :\

[danielmellado]

@GiedriusS how about putting that for now in the receive package? would you folks prefer other follow-up PR for that?

[squat]

I think that the logic can be simplified so that there is less room for error if we keep the original code as-is and then add the following four lines right here. We should determine the tenant value like we did originally and then sanitize it. Otherwise, we are doing path.Base too many times, which spreads out our sanitation logic and makes it more likely that we will miss one of the lines in future maintenance:

if tenant != path.Base(tenant) {
	http.Error(w, err.Error(), http.StatusInternalServerError)
	return
}

[danielmellado]

Yep, totally agree, still new to the codebase so thanks for pointing that out. I'll update the PR.

[squat]

Thanks, that's better! Thinking about it more, we should actually move this sanitization down below the next if block where we get the tenant from the certificate.

Maybe we even want to extract tenant value extraction and sanitization into its own, isolated function to keep the logic very tight.

[saswatamcode]

Thanks! I think @GiedriusS recommended something similar too. 🙂

I wonder if we should use path.Base at all here, though. In case the tenant header value is set to something like /, we can still have path.Base("/") == "/". Maybe instead just checking if the string contains any / would be easier here?

[saswatamcode]

Maybe we can return an error here, and then log + respond 400 in receiveHTTP instead? 🙂

[danielmellado]

+1, will make the changes, it'll make testing easier. Will push a draft so we can iterate

[matej-g]

It's shaping up nicely @danielmellado! 👍 Adding a test case for this would be nice.

[matej-g]

Accidentally changed formatting?

[danielmellado]

damn, blame the IDE, I'll put it back on my next commit xD

[matej-g]

Should be server error? 🤔 Or bad request?

[danielmellado]

hmm that's a good point. It could be either but in any case I'll go with Saswata's suggestion, stay tuned!

[saswatamcode]

LGTM!
Just a few small suggestions.

[danielmellado]

Failure on the tests seems totally unrealted