Update error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #636

fxamacker · 2025-03-17T02:14:38Z

Closes #634

RawTag.UnmarshalCBOR() is intended to be called by the codec internally and the codec checks for malformed data before calling it. However, it is possible for user apps to directly call it, so user apps might provide malformed data which can cause panic.

This PR updates these 3 functions to use same error handling as cbor.Unmarshal():

ByteString.UnmarshalCBOR(data)
RawTag.UnmarshalCBOR(data)
SimpleValue.UnmarshalCBOR(data)

Basically, this adds the same well-formedness checks on input data already done by cbor.Unmarshal(), so UnmarshalCBOR() will return same error if input data is malformed (not panic).

Caveats

Unfortunately, this approach means the same data is checked twice for the intended case of the codec calling UnmarshalCBOR() internally. This can be revisited and maybe optimized in the future.

This PR passed very brief fuzzing on Sunday, March 16, 2025. However, the fuzzing needs to be run for longer duration before release tagging v2.7.1.

Thanks

Thanks @thomas-fossati for reporting issue #634. While looking into RawTag, I found the same issue in ByteString and SimpleValue.

When RawTag.UnmarshalCBOR() is called by codec (normal case), the codec will first check if data is well-formed before calling RawTag.UnmarshalCBOR(data). However, it can also be called by user app (not intended use) and user apps might not check if data is well-formed. In such cases, this function can panic if given malformed data. This commit updates RawTag.UnmarshalCBOR() to check for well-formedness inside the function, so it behaves the same whether it is called by codec internally or by user app. Unfortunately, this approach means the same data is checked twice for the normal case of the codec using Unmarshal(data, *RawTag). This can be revisited and maybe optimized in the future.

When SimpleValue.UnmarshalCBOR() is called by codec (normal case), the codec will first check if data is well-formed before calling SimpleValue.UnmarshalCBOR(data). However, it can also be called by user app (not intended use) and user apps might not check if data is well-formed. In such cases, this function can panic if given malformed data. This commit updates SimpleValue.UnmarshalCBOR() to check for well-formedness inside the function, so it behaves the same whether it is called by codec internally or by user app. Unfortunately, this approach means the same data is checked twice for the normal case of the codec using Unmarshal(data, *SimpleValue). This can be revisited and maybe optimized in the future.

When ByteString.UnmarshalCBOR() is called by codec (normal case), the codec will first check if data is well-formed before calling ByteString.UnmarshalCBOR(data). However, it can also be called by user app (not intended use) and user apps might not check if data is well-formed. In such cases, this function can panic if given malformed data. This commit updates ByteString.UnmarshalCBOR() to check for well-formedness inside the function, so it behaves the same whether it is called by codec internally or by user app. Unfortunately, this approach means the same data is checked twice for the normal case of the codec using Unmarshal(data, *ByteString). This can be revisited and maybe optimized in the future.

x448

Unfortunately, this approach means the same data is checked twice for the intended case of the codec calling UnmarshalCBOR() internally. This can be revisited and maybe optimized in the future.

That was going to be my feedback but you already thought of it.

thomas-fossati

LGMT, thanks!

Currently, unreleased changes in PR #636 and #645 cause the input data to be checked twice when UnmarshalCBOR() is called internally by Unmarshal() for: - ByteString - RawTag - SimpleValue UnmarshalCBOR() checks input data because it can be called by user apps providing bad data. However, the codec already checks input data before internally calling UnmarshalCBOR() so the 2nd check is redundant. This commit avoids redundant check on the input data by having Unmarshal() call the private unmarshalCBOR() if implemented by ByteString, RawTag, SimpleValue, etc.: - Internally, the codec calls the private unmarshalCBOR() to avoid the redundant check on input data. - Externally, MarshalCBOR() is available as a wrapper that checks input data before calling the private unmarshalCBOR(). MarshalCBOR() for ByteString, RawTag, and SimpleValue are marked as deprecated and Unmarshal() should be used instead.

Currently, u 8000 nreleased changes in PR #636 and #645 cause the input data to be checked twice when UnmarshalCBOR() is called internally by Unmarshal() for: - ByteString - RawTag - SimpleValue UnmarshalCBOR() checks input data because it can be called by user apps providing bad data. However, the codec already checks input data before internally calling UnmarshalCBOR() so the 2nd check is redundant. This commit avoids redundant check on the input data by having Unmarshal() call the private unmarshalCBOR() if implemented by ByteString, RawTag, SimpleValue, etc.: - Internally, the codec calls the private unmarshalCBOR() to avoid the redundant check on input data. - Externally, UnmarshalCBOR() is available as a wrapper that checks input data before calling the private unmarshalCBOR(). UnmarshalCBOR() for ByteString, RawTag, and SimpleValue are marked as deprecated and Unmarshal() should be used instead.

Currently, unreleased changes in PR #636 and #645 cause the input data to be checked twice when UnmarshalCBOR() is called internally by Unmarshal() for: - ByteString - RawTag - SimpleValue UnmarshalCBOR() checks input data because it can be called by user apps providing bad data. However, the codec already checks input data before internally calling UnmarshalCBOR() so the 2nd check is redundant. This commit avoids redundant check on the input data by having Unmarshal() call the private unmarshalCBOR() if implemented by ByteString, RawTag, SimpleValue, etc.: - Internally, the codec calls the private unmarshalCBOR() to avoid the redundant check on input data. - Externally, UnmarshalCBOR() is available as a wrapper that checks input data before calling the private unmarshalCBOR(). UnmarshalCBOR() for ByteString, RawTag, and SimpleValue are marked as deprecated and Unmarshal() should be used instead.

fxamacker added 3 commits March 16, 2025 19:50

fxamacker added the bugfix label Mar 17, 2025

fxamacker added this to the v2.7.1 milestone Mar 17, 2025

fxamacker self-assigned this Mar 17, 2025

fxamacker mentioned this pull request Mar 17, 2025

bug: RawTag.UnmarshalCBOR() panic with truncated tag #634

Closed

x448 approved these changes Mar 17, 2025

View reviewed changes

thomas-fossati approved these changes Mar 17, 2025

View reviewed changes

fxamacker merged commit efbaf6c into release-2.7 Mar 18, 2025
18 checks passed

This was referenced Mar 26, 2025

Port updated error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #645

Merged

Optimize internal calls to UnmarshalCBOR() for ByteString, RawTag, and SimpleValue #646

Closed

fxamacker mentioned this pull request Mar 28, 2025

Optimize internal calls to UnmarshalCBOR() for ByteString, RawTag, SimpleValue #647

Merged

fxamacker mentioned this pull request Mar 28, 2025

Port PR 647 to release-2.7 branch to optimize internal calls to UnmarshalCBOR() #648

Merged

fxamacker deleted the fxamacker/check-wellformedness-in-UnmarshalCBOR branch April 1, 2025 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #636

Update error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #636

Update error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #636

Update error handling in RawTag.UnmarshalCBOR(), etc. to match cbor.Unmarshal() #636

Conversation

Caveats

Thanks

Choose a reason for hiding this comment

Choose a reason for hiding this comment