Header value matching (including Host) only supports case sensitive matching

[bburky]

I saw some issues that FilterChain matcher and TriggerRule might be merged? There is another issue in the filter chain matching that I wanted to point out while you are working on that area of the code. #172 #171

The prefix and equality match operators match the header value case sensitively, and this is a problem for matching Host:.

It is possible to bypass an authservice chain with an equality match on Host: example.com by with a request like curl http://EXAMPLE.COM. If an Istio AuthorizationPolicy is used after Authservice, this isn't an auth bypass because the request would be rejected with RBAC: access denied due to a missing JWT. I can't tell if using Istio AuthZ is considered optional or required though. Regardless this still a bug that I wanted your team to be aware of if you're fixing up that area of code.

The related code is:

authservice/src/filters/filter_chain.cc

Lines 93 to 113 in e91ca69

	bool FilterChainImpl::Matches(
	const ::envoy::service::auth::v3::CheckRequest* request) const {
	spdlog::trace("{}", __func__);
	if (config_.has_match()) {
	auto matched = request->attributes().request().http().headers().find(
	config_.match().header());
	if (matched != request->attributes().request().http().headers().cend()) {
	switch (config_.match().criteria_case()) {
	case config::Match::kPrefix:
	return absl::StartsWith(matched->second, config_.match().prefix());
	case config::Match::kEquality:
	return matched->second == config_.match().equality();
	default:
	throw std::runtime_error(
	"invalid FilterChain match type"); // This should never happen.
	}
	}
	return false;
	}
	return true;
	}

I haven't tested this on a recent version of authservice, but I think the case sensitive behavior is still there.

[bburky]

Currently only HTTP headers request->attributes().request().http().headers() may be matched. I think an enhancement could be to allow matching on request->attributes().destination().principal() (or anything in the request attributes).

For the specific use case of matching the Host: header, I just wanted to select workloads in the filter chain. If deploying Authservice at the sidecar (not the gateway), each workload will have a unique service account and this could allow easier matching. Also, this principal can't be influenced by an attacker and could be considered safer to match.

[Shikugawa]

@incfly I think there are two problems here.

1. Even though header vaue is case-insensitive from the RFC3982, header matching in current code is case-sensitive. It allows attackers to violate protected services.
2. If requests are not matched, the request should be ALLOWed. This is the same as Authservice should implicitly deny requests that don't match config prefixes #140.

The expected behavior is like the following.

if (request) {
  should_allow = false; // Resolve problem 2. Default behavior should be false/
  for (chain <- chains) {
    if (chain.Match(req) { // Resolve problem 1, header matching must be case-insensitive.
      should_allow = chain.Process(req);
    }
  } 

  if (!should_allow) {
    return DENY;
  }
  
  return ALLOW;
}

[bburky]

I think RFC 3982 and RFC7230 only say header "field names" are case-insensitive, not the field value. You have to read the spec to determine how to match each header.

https://httpwg.org/specs/rfc7230.html#rfc.section.2.7.3

The scheme and host are case-insensitive and normally provided in lowercase; all other components are compared in a case-sensitive manner.

I'm not sure how to best handle this in authservice. You could special case some headers and lowercase them before processing. Or provide an optional case insensitive matching mode. Or allow matching the request.host attribute which I think may already be normalized by envoy.

[incfly]

I will +1 on the option to provide a option to match with case insenstive, similiar to Envoy's StringMatcher API

https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/matcher/v3/string.proto#string-matcher

Where you can specify ignore_case.

but back to this overall issue, it'll be solved if #140 is solved as well right? If we provide an option in the overall matches, that if no match found, return 403 instead of pass through at authservice level. EXAMPLE.COM will returns 403 even earlier. Is that right?

[bburky]

Yes, addressing #140 will resolve the security impact of this issue.

There is already no security risk if an Istio AuthorizationPolicy is applied after authservice and requires a JWT for all requests (for example, requestPrincipals: ["*"]). The docs don't discuss whether this is considered required, I recommend clarifying this.