diff --git a/auth/exchange/exchange_test.go b/auth/exchange/exchange_test.go index 2a842fa..634e054 100644 --- a/auth/exchange/exchange_test.go +++ b/auth/exchange/exchange_test.go @@ -1400,3 +1400,114 @@ func TestDroppedRuneCannotRejoinASecret(t *testing.T) { }) } } + + +// TestErrorDescriptionRedactsReplacementSplitCredentials: a credential split +// by U+FFFD, or by bytes that are not valid UTF-8 (which the JSON decoder +// turns into U+FFFD), must not dodge exact redaction and then rejoin when the +// rendering drops the non-ASCII between its halves. The secrets are shorter +// than the token-run backstop, so only the exact match can catch them, and +// both the terminal description and an infrastructure failure's rendered +// text are checked. +func TestErrorDescriptionRedactsReplacementSplitCredentials(t *testing.T) { + const ( + secret = "abcdef123456789" // #nosec G101 -- test fixture + subject = "subj0123456789" + ) + splitters := map[string]string{ + "escaped U+FFFD": `�`, + "literal U+FFFD": "�", + "lone surrogate": `\ud800`, + "invalid byte 0xff": "\xff", + "overlong NUL": "\xc0\x80", + "truncated multi-byte": "\xe2\x82", + "double U+FFFD": "��", + } + split := func(s, sep string) string { return s[:6] + sep + s[6:] } + for name, sep := range splitters { + for _, code := range []string{"invalid_grant", "invalid_client"} { + t.Run(name+"/"+code, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprintf(w, `{"error":%q,"error_description":"secret=%s subject=%s"}`, + code, split(secret, sep), split(subject, sep)) + })) + defer srv.Close() + + ex, err := New(Config{TokenURL: srv.URL, Auth: ClientSecretPost("id", secret)}) + if err != nil { + t.Fatal(err) + } + _, err = ex.Exchange(context.Background(), subject) + if err == nil { + t.Fatal("want an error") + } + texts := []string{err.Error()} + var te *TerminalError + if errors.As(err, &te) { + texts = append(texts, te.Description, upstream.ScrubDescription(te.Description)) + } + var ie *InfraError + if errors.As(err, &ie) { + texts = append(texts, ie.Description) + } + if te == nil && ie == nil { + t.Fatalf("want TerminalError or InfraError, got %T: %v", err, err) + } + for _, text := range texts { + // Also drop everything but printable ASCII, as a viewer + // or normalizer downstream might. + for _, s := range []string{text, upstream.PrintableASCII(text)} { + if strings.Contains(s, secret) || strings.Contains(s, subject) { + t.Fatalf("split credential rejoined: %q", s) + } + } + } + }) + } + } +} + +// TestInvisibleMarkCannotSplitASecret: a combining mark (variation selector +// 16, combining grapheme joiner) is invisible but neither a control nor a +// format character. Threaded through an echoed credential every few bytes, +// it splits the value into pieces shorter than both the fragment and the +// token-run redactions, and a viewer that drops it shows the credential +// whole. Only printable ASCII survives the strip, so the pieces rejoin before +// redaction sees them. +func TestInvisibleMarkCannotSplitASecret(t *testing.T) { + const secret = "abcdef123456789" // #nosec G101 -- test fixture + for _, mark := range []string{"͏", "️", "⃝"} { + t.Run(fmt.Sprintf("%U", []rune(mark)[0]), func(t *testing.T) { + var threaded strings.Builder + for i := 0; i < len(secret); i += 3 { + threaded.WriteString(secret[i:min(i+3, len(secret))] + mark) + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = fmt.Fprintf(w, `{"error":"invalid_client","error_description":"was %s"}`, threaded.String()) + })) + defer srv.Close() + ex, err := New(Config{TokenURL: srv.URL, Auth: ClientSecretPost("id", secret)}) + if err != nil { + t.Fatal(err) + } + _, err = ex.Exchange(context.Background(), "subject-tok") + if !IsInfra(err) { + t.Fatalf("want InfraError, got %v", err) + } + // As a viewer that drops what it can't show would render it. + shown := strings.Map(func(r rune) rune { + if r < 0x20 || r >= 0x7f { + return -1 + } + return r + }, err.Error()) + if strings.Contains(shown, secret) { + t.Fatalf("secret rejoined in the error text: %q", err.Error()) + } + }) + } +} diff --git a/auth/exchange/oautherror.go b/auth/exchange/oautherror.go index 4413fba..10bc61f 100644 --- a/auth/exchange/oautherror.go +++ b/auth/exchange/oautherror.go @@ -47,23 +47,13 @@ func ParseOAuthError(body []byte, form url.Values) (code, description string) { return code, description } -// stripControls removes the runes upstream.LogSafeRune rejects (controls, -// line and paragraph separators, format characters, U+FFFD). Applied to both the -// endpoint text and each redaction candidate so a secret carrying a tab -// arrives control-stripped in the echoed text and only an identically -// normalized candidate can match it. -func stripControls(v string) string { - var b strings.Builder - for _, r := range v { - // The same runes upstream.ScrubDescription drops when rendering: - // one left in here would split a credential past redaction and - // rejoin it in the rendered text. - if upstream.LogSafeRune(r) { - b.WriteRune(r) - } - } - return b.String() -} +// stripControls keeps only printable ASCII (upstream.PrintableASCII), the +// same strip upstream.ScrubDescription applies when rendering: a rune left in +// here would split a credential past redaction and rejoin it in the rendered +// text. Applied to both the endpoint text and each redaction candidate so a +// secret carrying a tab arrives stripped in the echoed text and only an +// identically normalized candidate can match it. +func stripControls(v string) string { return upstream.PrintableASCII(v) } // credentialFields are the form parameters whose values are secrets at ANY // length, spanning both the token-exchange form (subject_token, diff --git a/auth/upstream/failure.go b/auth/upstream/failure.go index 7f7bb82..8c55f0a 100644 --- a/auth/upstream/failure.go +++ b/auth/upstream/failure.go @@ -22,10 +22,17 @@ package upstream import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" "errors" "fmt" + "io" + "net" + "net/http" "strings" - "unicode" + "syscall" "unicode/utf8" ) @@ -37,7 +44,11 @@ const MaxDescriptionBytes = 200 // minTokenRun is the shortest run of token-alphabet characters a Failure's // description redacts. bc3's tokens, JWT client assertions and generated // client secrets are all longer; English words in an OAuth error_description -// are all shorter. +// are all shorter. A shorter run is kept, so a fragment of a credential under +// this length can render if an endpoint truncates its own echo: the guarantee +// is that no usable credential renders, not that no byte of one does. Every +// credential the request submitted is also removed whole, at any length, by +// exchange.ParseOAuthError before the description gets here. const minTokenRun = 20 // Failure is a failed call to a credentialed endpoint. Its Error text is safe @@ -89,6 +100,10 @@ func (f *Failure) Error() string { b.WriteString(": ") b.WriteString(f.Reason) } + if c := causeClass(f.cause); c != "" { + b.WriteString(": ") + b.WriteString(c) + } if d := ScrubDescription(f.Description); d != "" { fmt.Fprintf(&b, " (%s)", d) } @@ -107,31 +122,75 @@ func (f *Failure) As(target any) bool { return f.cause != nil && errors.As(f.cause, target) } -// LogSafeRune reports whether r may stay in a log line rendered from -// endpoint text. It drops every control character (C0, DEL and C1, which -// includes NEL), the Unicode line and paragraph separators, format -// characters (bidi overrides, zero-width joiners) and U+FFFD: anything a -// sink might render as a line break or use to disguise text. ScrubDescription -// and the exchange's redaction both strip with it, so a credential split by -// a dropped rune is rejoined before redaction rather than after. -func LogSafeRune(r rune) bool { - return r != utf8.RuneError && !unicode.IsControl(r) && !unicode.In(r, unicode.Zl, unicode.Zp, unicode.Cf) +// causeClass names what kind of failure cause is, in fixed text, so an +// operator reading a log line can still tell a timeout from a refused +// connection from a bad certificate without the cause's own text. Every +// phrase is chosen here; nothing is read out of the cause but its type and +// the identity of sentinel errors. The one exception is a JSON type error's +// Field, which is the path of the Go struct field being decoded: this +// server's own names, never response bytes. +func causeClass(err error) string { + if err == nil { + return "" + } + var ( + netErr net.Error + dnsErr *net.DNSError + opErr *net.OpError + unknownCA x509.UnknownAuthorityError + certErr x509.CertificateInvalidError + hostErr x509.HostnameError + recordErr tls.RecordHeaderError + syntaxErr *json.SyntaxError + typeErr *json.UnmarshalTypeError + maxBytes *http.MaxBytesError + protoError *http.ProtocolError + ) + switch { + case errors.Is(err, context.DeadlineExceeded): + return "deadline exceeded" + case errors.Is(err, context.Canceled): + return "canceled" + case errors.As(err, &netErr) && netErr.Timeout(): + return "timeout" + case errors.As(err, &dnsErr): + return "DNS lookup failed" + case errors.Is(err, syscall.ECONNREFUSED): + return "connection refused" + case errors.Is(err, syscall.ECONNRESET): + return "connection reset" + case errors.As(err, &unknownCA), errors.As(err, &certErr), errors.As(err, &hostErr): + return "TLS certificate verification failed" + case errors.As(err, &recordErr): + return "TLS handshake failed" + case errors.Is(err, io.ErrUnexpectedEOF), errors.Is(err, io.EOF): + return "response ended early" + case errors.As(err, &syntaxErr): + return "response is not valid JSON" + case errors.As(err, &typeErr): + if typeErr.Field != "" { + return "unexpected JSON value for " + typeErr.Field + } + return "unexpected JSON value" + case errors.As(err, &maxBytes): + return "response too large" + case errors.As(err, &protoError): + return "malformed HTTP response" + case errors.As(err, &opErr): + return "network error during " + opErr.Op + default: + return "" + } } -// ScrubDescription renders endpoint-supplied text for a log line: runes -// LogSafeRune rejects stripped, every run of at least minTokenRun token-alphabet +// ScrubDescription renders endpoint-supplied text for a log line: control +// and everything but printable ASCII stripped (PrintableASCII), every run of at least minTokenRun token-alphabet // characters replaced with [redacted], and the result bounded to // MaxDescriptionBytes. Redaction runs over the whole text before the bound, // so the bound can never cut a token down to a prefix that escapes the run // length. func ScrubDescription(s string) string { - var stripped strings.Builder - for _, r := range s { - if LogSafeRune(r) { - stripped.WriteRune(r) - } - } - text := stripped.String() + text := PrintableASCII(s) var b strings.Builder run := 0 @@ -172,6 +231,25 @@ func ScrubDescription(s string) string { return out[:cut] + "…" } +// PrintableASCII keeps only printable ASCII (space through tilde) from s. +// Redaction runs on the result, so nothing invisible can sit inside an +// echoed credential and split it into pieces that exact matching and the +// token-run length both miss, for a viewer or a normalizer that drops +// invisible characters to put back together. An allowlist, because the +// invisible set is not: C0 and DEL, then C1 and format characters, then +// variation selectors were each found in turn on review, and Unicode has +// more. OAuth error descriptions are ASCII by specification (RFC 6749 +// §5.2), so nothing a conformant endpoint says is lost. +func PrintableASCII(s string) string { + var b strings.Builder + for i := 0; i < len(s); i++ { + if c := s[i]; c >= 0x20 && c < 0x7f { + b.WriteByte(c) + } + } + return b.String() +} + // isTokenByte reports whether c can appear in a bearer token, a JWT, a // base64 or base64url secret, or a form-encoded one. func isTokenByte(c byte) bool { diff --git a/auth/upstream/failure_test.go b/auth/upstream/failure_test.go index faa1d51..07e90e0 100644 --- a/auth/upstream/failure_test.go +++ b/auth/upstream/failure_test.go @@ -2,10 +2,15 @@ package upstream import ( "context" + "encoding/json" "errors" "fmt" + "io" + "net" "net/url" + "os" "strings" + "syscall" "testing" ) @@ -23,7 +28,8 @@ func TestFailure_TextNeverRendersTheCause(t *testing.T) { if strings.Contains(f.Error(), secret) || strings.Contains(f.Error(), "Location") { t.Fatalf("cause text reached Error(): %q", f.Error()) } - if got, want := f.Error(), "token exchange infrastructure failure: token endpoint request failed"; got != want { + // The cause's class, in fixed text, is all of it that renders. + if got, want := f.Error(), "token exchange infrastructure failure: token endpoint request failed: deadline exceeded"; got != want { t.Fatalf("Error() = %q, want %q", got, want) } } @@ -96,7 +102,8 @@ func TestScrubDescription(t *testing.T) { {"controls stripped before the run is measured", "x bc_at_0123\n456789abcdef y", "x [redacted] y"}, {"Unicode line controls stripped", "a\u0085b\u2028c\u2029d", "abcd"}, {"format characters stripped", "a\u202eb\u200bc", "abc"}, - {"letters beyond ASCII kept", "café ünïcode", "café ünïcode"}, + {"letters beyond ASCII dropped", "caf\u00e9 \u00fcn\u00efcode", "caf ncode"}, + {"U+FFFD and invalid bytes dropped", "a\ufffdb\xffc", "abc"}, {"bounded", long, strings.TrimSpace(long)[:MaxDescriptionBytes] + "…"}, {"empty", "", ""}, } @@ -123,9 +130,52 @@ func TestScrubDescription_BoundNeverExposesATokenPrefix(t *testing.T) { } } -func TestScrubDescription_BoundKeepsUTF8Valid(t *testing.T) { - got := ScrubDescription(strings.Repeat("é", MaxDescriptionBytes)) - if !strings.HasSuffix(got, "…") || strings.ContainsRune(got, '�') { - t.Fatalf("bound cut a rune: %q", got) +// TestScrubDescription_KeepsOnlyPrintableASCII: no invisible character of +// any class (C1, format, combining mark) can split a token into short runs. +func TestScrubDescription_KeepsOnlyPrintableASCII(t *testing.T) { + in := "caf\u00e9 bc_at_0123\u0085456\u200b789\ufe0fabc\u034fdef\u2060ghij y" + if got, want := ScrubDescription(in), "caf [redacted] y"; got != want { + t.Fatalf("ScrubDescription = %q, want %q", got, want) + } +} + +func TestCauseClass(t *testing.T) { + var num int64 + typeErr := json.Unmarshal([]byte(`{"expires_in":`+"99999999999999999999"+`}`), &struct { + ExpiresIn *int64 `json:"expires_in"` + }{&num}) + syntaxErr := json.Unmarshal([]byte(`{"a":`+secret), &num) + tests := []struct { + name string + err error + want string + }{ + {"none", nil, ""}, + {"deadline", fmt.Errorf("wrapped: %w", context.DeadlineExceeded), "deadline exceeded"}, + {"refused", &net.OpError{Op: "dial", Err: os.NewSyscallError("connect", syscall.ECONNREFUSED)}, "connection refused"}, + {"other network", &net.OpError{Op: "read", Err: errors.New(secret)}, "network error during read"}, + {"dns", &net.DNSError{Err: "no such host", Name: secret}, "DNS lookup failed"}, + {"eof", fmt.Errorf("reading: %w", io.ErrUnexpectedEOF), "response ended early"}, + {"json type", typeErr, "unexpected JSON value for expires_in"}, + {"json syntax", syntaxErr, ""}, + {"unknown", errors.New(secret), ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := causeClass(tt.err) + if tt.name == "json syntax" { + // An unterminated value is an unexpected EOF to the decoder. + if got != "response ended early" && got != "response is not valid JSON" { + t.Fatalf("causeClass = %q", got) + } + return + } + if got != tt.want { + t.Fatalf("causeClass = %q, want %q", got, tt.want) + } + if strings.Contains(got, secret) { + t.Fatalf("causeClass rendered cause text: %q", got) + } + }) } } diff --git a/auth/upstream/upstreamtest/hostile.go b/auth/upstream/upstreamtest/hostile.go index 1e7ea7b..e21517f 100644 --- a/auth/upstream/upstreamtest/hostile.go +++ b/auth/upstream/upstreamtest/hostile.go @@ -11,6 +11,7 @@ package upstreamtest import ( + "encoding/json" "fmt" "net/http" "strings" @@ -97,6 +98,11 @@ const ( // TerminalDescription answers 400 invalid_grant with the echo in the // error_description. TerminalDescription Kind = "echoed invalid_grant description" + // ControlSplitDescription answers 401 invalid_client with the echo in + // the error_description, an invisible character after every third + // character, so neither exact redaction nor a run length sees it + // whole. + ControlSplitDescription Kind = "control-split error_description" // ServerError answers 500 with the echo as the body. ServerError Kind = "echoed 500 body" // RateLimited answers 429 with the echo as Retry-After and the body. @@ -107,7 +113,7 @@ const ( var Kinds = []Kind{ MalformedLocation, Redirect, MalformedStatus, MalformedHeader, MalformedTrailer, HugeNumber, NegativeLifetime, BadProfile, OAuthCode, OAuthDescription, - TerminalDescription, ServerError, RateLimited, + TerminalDescription, ControlSplitDescription, ServerError, RateLimited, } const accessTokenURN = "urn:ietf:params:oauth:token-type:access_token" // #nosec G101 -- RFC 8693 URN @@ -150,6 +156,9 @@ func Handler(kind Kind) http.Handler { case TerminalDescription: writeJSON(w, http.StatusBadRequest, fmt.Sprintf( `{"error":"invalid_grant","error_description":"grant rejected; request was %s"}`, echo)) + case ControlSplitDescription: + desc, _ := json.Marshal("client rejected; request was " + splitByInvisibles(echo)) + writeJSON(w, http.StatusUnauthorized, fmt.Sprintf(`{"error":"invalid_client","error_description":%s}`, desc)) case ServerError: writeText(w, http.StatusInternalServerError, echo) case RateLimited: @@ -161,6 +170,23 @@ func Handler(kind Kind) http.Handler { }) } +// invisibles are characters a viewer shows as nothing, from each class +// review found: C1 (NEL), format (zero-width space, word joiner), and +// combining marks (variation selector 16, combining grapheme joiner). +var invisibles = []rune{'\u0085', '\u200b', '\u2060', '\ufe0f', '\u034f'} + +// splitByInvisibles puts one of invisibles after every third character of s. +func splitByInvisibles(s string) string { + var b strings.Builder + for i, r := range s { + b.WriteRune(r) + if i%3 == 2 { + b.WriteRune(invisibles[(i/3)%len(invisibles)]) + } + } + return b.String() +} + func writeJSON(w http.ResponseWriter, status int, body string) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) @@ -194,8 +220,18 @@ const WindowBytes = 12 // AssertClean fails t when text, from the named sink, contains any // WindowBytes-long piece of any secret. +// +// It looks at text with everything but printable ASCII removed, as a viewer +// or normalizer that drops invisible characters might show it, so a +// credential threaded with any of them still counts as present. func AssertClean(t testing.TB, sink, text string, secrets ...string) { t.Helper() + text = strings.Map(func(r rune) rune { + if r < 0x20 || r >= 0x7f { + return -1 + } + return r + }, text) for _, s := range secrets { if len(s) < WindowBytes { if strings.Contains(text, s) {