Trailing extraneous bytes passed to cbor.Unmarshal(), also it may contain authenticator extension data

[fxamacker]

Version

0.8.6

Description

This issue has a public test from August 31, 2023 at gravitational/teleport#31322. I maintain fxamacker/cbor, my apologies for not catching some aspects of this sooner. 🙏

ParseCredentialCreationResponseBody() is called with data that causes code in webauthncbor.go to eventually pass 91 bytes to cbor.Unmarshal().

Those 91 bytes represent a CBOR Sequence (RFC 8742) instead of CBOR data item (RFC 8949).

cbor.Unmarshal() is for parsing a CBOR data item rather than CBOR Sequence (concatenation of CBOR data items).

In old versions of cbor library, the extra trailing bytes are ignored by cbor.Unmarshal() instead of returning error.

For some uses cases, ignoring trailing bytes is undesirable and can create risks. In this test case, the extraneous 14 bytes is a CBOR data item that appears to represent authenticator extension data {"credProtect": 2}.

Upgrading to fxamacker/cbor v2.5.0 makes it easier to detect and handle extraneous data:

• cbor.Unmarshal() detects trailing bytes and returns ExtraneousDataError.
• cbor.UnmarshalFirst() function was added to explicitly handle extraneous bytes without ExtraneousDataError.
• cbor.DiagnoseFirst() function was added to return human readable text (Diagnostic Notation) for logging/debugging.

// UnmarshalFirst decodes first CBOR data item and returns remaining bytes.
rest, err = cbor.UnmarshalFirst(b, &v)   // decode []byte b to v

If you choose to upgrade to v2.5.0, please see ⭐ Notable Changes to Review Before Upgrading in the release notes.

My apologies again for not catching some of this sooner. Please let me know if I can help make the upgrade go smoothly.

Reproduction

See gravitational/teleport#31322 for test and reproducer.

TestIssue31187_errorParsingAttestationResponse

// TestIssue31187_errorParsingAttestationResponse reproduces the root cause of
// https://github.com/gravitational/teleport/issues/31187 by attempting to parse
// a current CCR created using a Chrome/Yubikey pair.
//
// The test exposes a poor interaction between go-webauthn/webauthn v0.8.6 and
// fxamacker/cbor/v2 v2.5.0.
func TestIssue31187_errorParsingAttestationResponse(t *testing.T) {
	// Captured from an actual Yubikey 5Ci registration request.
	const body = `{"id":"ibfM_71b4q2_xWPZDyvhZmJ_KU8f-mOCCLXHp-fTVoHZpDelym5lvBJDPr1EtD_l","type":"public-key","rawId":"ibfM_71b4q2_xWPZDyvhZmJ_KU8f-mOCCLXHp-fTVoHZpDelym5lvBJDPr1EtD_l","response":{"clientDataJSON":"eyJ0eXBlIjoid2ViYXV0aG4uY3JlYXRlIiwiY2hhbGxlbmdlIjoidEdiVFhEbzBGMXRNUVlmamRSLWNETlV1TUNvVURTX0w0OElSWmY4MUVuWSIsIm9yaWdpbiI6Imh0dHBzOi8vemFycXVvbi5kZXY6MzA4MCIsImNyb3NzT3JpZ2luIjpmYWxzZSwib3RoZXJfa2V5c19jYW5fYmVfYWRkZWRfaGVyZSI6ImRvIG5vdCBjb21wYXJlIGNsaWVudERhdGFKU09OIGFnYWluc3QgYSB0ZW1wbGF0ZS4gU2VlIGh0dHBzOi8vZ29vLmdsL3lhYlBleCJ9","attestationObject":"o2NmbXRkbm9uZWdhdHRTdG10oGhhdXRoRGF0YVjCnNjmsqMh0nu-_tuMkxVZkZShAhdoz0tK9evxg8ys9CLFAAAAAQAAAAAAAAAAAAAAAAAAAAAAMIm3zP-9W-Ktv8Vj2Q8r4WZifylPH_pjggi1x6fn01aB2aQ3pcpuZbwSQz69RLQ_5aUBAgMmIAEhWCCJt8z_vVvirb_FY9kPpoIwbfhER3VHTmOV0Y6xs7uHySJYIMFARJxlUoR4DbDzlKYnfJKitWgR3GHK9_Lz211z-128oWtjcmVkUHJvdGVjdAI"}}`

	_, err := protocol.ParseCredentialCreationResponseBody(strings.NewReader(body))
	require.NoError(t, err, "ParseCredentialCreationResponseBody failed")
}

Reproducer calls ParseCredentialCreationResponseBody with data that causes code in webauthncbor.go to eventually pass 91 bytes to cbor.Unmarshal().

Those 91 bytes represent a CBOR Sequence (RFC 8742) instead of CBOR data item (RFC 8949).

In Diagnostic Notation, first CBOR data item represents:

{1: 2, 3: -7, -1: 1, 
-2: h'89b7ccffbd5be2adbfc563d90fa682306df8444775474e6395d18eb1b3bb87c9',
-3: h'c140449c655284780db0f394a6277c92a2b56811dc61caf7f2f3db5d73fb5dbc'}

The 2nd CBOR data item (aka extraneous data in RFC 8949) represents:

{"credProtect": 2}

Expectations

Detect or prevent extraneous bytes or CBOR Sequence being passed to cbor.Unmarshal().

If needed, handle authenticator extension data currently contained in the trailing bytes.

Documentation

No response

[james-d-elliott]

Hey thanks for making contact. I believe I had in mind some more rigorous testing before upgrading (since this dep is pretty important) and time got away from me. It makes sense that this is an important upgrade now that you explain it. I'll merge this and make a release today.

[fxamacker]

Hey @james-d-elliott thanks for super fast response! 🙌

You probably already know this, so I'm just being extra cautious. The cbor upgrade to v2.5.0 by itself might cause authentications to fail (as experienced by gravitational/teleport).

You may want to replace cbor.Unmarshal() with cbor.UnmarshalFirst() and/or make changes in the callers to prevent passing extraneous trailing bytes to cbor.Unmarshal().

Thanks again for amazing response time!

[james-d-elliott]

No worries. I was definitely aware there may be issues which is why I was waiting. I did plan to do more thorough testing but I can relatively easily release a fix if any issues are noticed. Thanks for the pointer about cbor.Unmarshal though. I'll do some reading and maybe make a hotfix later today if it makes sense.