Repository: istio/istio
File: pkg/wasm/imagefetcher.go (Go)

Code before:

func (o ImageFetcherOption) String() string {
	if o.PullSecret == nil {
		return fmt.Sprintf("{Insecure: %v}", o.Insecure)
	}
	return fmt.Sprintf("{Insecure: %v, PullSecret: <redacted>}", o.Insecure)
}

type ImageFetcher struct {
	fetchOpts []remote.Option
}

// ssrfProtectionTransport wraps http.RoundTripper to block SSRF via bearer realm
type ssrfProtectionTransport struct {
	inner http.RoundTripper
}

func (t *ssrfProtectionTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	resp, err := t.inner.RoundTrip(req)
	if err != nil || resp == nil {
		return resp, err
	}

	// check 401 responses for malicious WWW-Authenticate realm
	if resp.StatusCode == http.StatusUnauthorized {
		if auth := resp.Header.Get("WWW-Authenticate"); auth != "" {
			if err := validateBearerRealm(auth); err != nil {
				resp.Body.Close()
				return nil, fmt.Errorf("rejected unsafe bearer realm: %w", err)
			}
		}
	}

	return resp, nil
}

func validateBearerRealm(wwwAuth string) error {
	// parse realm from WWW-Authenticate header
	// format: Bearer realm="...",service="..."
	if !strings.Contains(wwwAuth, "realm=") {
		return nil
	}

	start := strings.Index(wwwAuth, "realm=\"")
	if start == -1 {
		return nil
	}
	start += 7 // skip 'realm="'

	end := strings.Index(wwwAuth[start:], "\"")
	if end == -1 {

Review comment (security):
I might be overlooking some additional context here, but server is allowed to return multiple different challenges in the response. It could be in form of multiple `WWW-Authenticate` headers, or, bizzarely, it could be in form of multiple comma-separted challenges in one `WWW-Authenticate` header or a combination of the above.

When it's former, I think, `Header.Get` returns only one of the values, depending on which one it returns, the check might not trigger.

The semantics of what authorization method is supposed to be used when multiple are present in the response is a bit fuzzy, but, I'd say a well behaved client should pick the most secure of the supported challenges. And that may create a situation, for example, where we have say Bearer and Basic methods advertised in separate `WWW-Authenticate` headers, Header.Get returns the Basic value with a realm that passes the check, but then later when the actual client tries to authorize itself it picks the Bearer auth method with a realm that does not pass the check.


When a bunch of auth challenges are lumped together into one header value, Header.Get will return all of those in one string, but I think it might cause a similar problem, but this time in `validateBearerRealm` below - it will find a realm in the response and will check it, but it may not necessarily be the realm that will get used in the end.

Should we combine all the headers together and check all the realms in the combined string?

Code after:

func (o ImageFetcherOption) String() string {
	if o.PullSecret == nil {
		return fmt.Sprintf("{Insecure: %v}", o.Insecure)
	}
	return fmt.Sprintf("{Insecure: %v, PullSecret: <redacted>}", o.Insecure)
}

type ImageFetcher struct {
	fetchOpts []remote.Option
}

// ssrfProtectionTransport wraps http.RoundTripper to block SSRF via bearer realm
type ssrfProtectionTransport struct {
	inner http.RoundTripper
}

func (t *ssrfProtectionTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	resp, err := t.inner.RoundTrip(req)
	if err != nil || resp == nil {
		return resp, err
	}

	// check 401 responses for malicious WWW-Authenticate realm
	// check ALL headers as the server can return multiple, and library might use any
	if resp.StatusCode == http.StatusUnauthorized {
		for _, auth := range resp.Header.Values("WWW-Authenticate") {
			if err := validateAllRealms(auth); err != nil {
				resp.Body.Close()
				return nil, fmt.Errorf("rejected unsafe bearer realm: %w", err)
			}
		}
	}

	return resp, nil
}

// validateAllRealms checks all realm parameters in a WWW-Authenticate header.
// in case of multiple challenges - validate all of them.
func validateAllRealms(wwwAuth string) error {
	// find all realm="..." occurrences
	remaining := wwwAuth
	for {
		idx := strings.Index(remaining, "realm=\"")
		if idx == -1 {
			break
		}
		remaining = remaining[idx+7:] // skip 'realm="'

		end := strings.Index(remaining, "\"")
		if end == -1 {