fix: full depth tracking across Codable container hierarchy#124
Open
hamchapman wants to merge 2 commits intohc/fix-unimplemented-codable-superencoder-methodsfrom
Open
Conversation
This change includes 2 main changes. Previously, the `maximumDepth` option was defined but not actually used in `CodableCBORDecoder`, creating a DoS vulnerability where deeply nested structures could cause stack overflow. **Problem:** 1. `CodableCBORDecoder` had a `maximumDepth` property but it wasn't included in the options computed property, so it defaulted to `.max` 2. `SingleValueDecodingContainer` called `CBOR.decode()` without passing options 3. `KeyedDecodingContainer` and `UnkeyedDecodingContainer` created `CBORDecoder` instances without passing options 4. This meant depth checking was never enforced for Codable decoding **Fix:** - Add `maximumDepth` property to `CodableCBORDecoder` (public API) - Include `maximumDepth` in options computed property - Update `setOptions()` to set `maximumDepth` - Pass options to all `CBOR.decode()` calls in `SingleValueDecodingContainer` - Pass options to `CBORDecoder()` calls in `KeyedDecodingContainer` - Pass options to `CBORDecoder()` calls in `UnkeyedDecodingContainer` Changes: - Added `currentDepth` field to `_CBORDecoder` and all container classes (`KeyedContainer`, `UnkeyedContainer`, `SingleValueContainer`) - Thread `currentDepth` through all container creation and nested decoding - Check depth before creating keyed/unkeyed containers in `_CBORDecoder` - Increment depth when creating nested decoders in `decode()` methods - Pass incremented depth through `superDecoder()` methods Previously, each `CBOR.decode()` call would reset `currentDepth` to 0, allowing deeply nested structures to bypass the depth limit. For example, a 10-level nested array with `maximumDepth=5` would incorrectly succeed. Now depth is properly tracked across the entire container hierarchy.
This fixes two related issues introduced when CBOR tag processing was enabled:
1. Date decoding: Tag 1 timestamps are converted to CBOR.date values, but
SingleValueDecodingContainer didn't handle the .date case when decoding
Double, causing type mismatch errors. Fixed by returning
date.timeIntervalSinceReferenceDate.
2. Byte string as array: Foundation types (CharacterSet, Data) encode
internal data as CBOR byte strings but expect to decode via
unkeyedContainer(). The decoder now:
- Accepts byte strings (0x40-0x5f) in ensureArray()
- Passes an isByteString flag to UnkeyedContainer constructor
- Only sets this flag for top-level byte string containers, not nested
containers, preventing false positives when nested arrays happen to
have format bytes in the byte string range
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change includes 2 main changes.
1. Enforcing
maximumDepthinCodableCBORDecoderPreviously, the
maximumDepthoption was defined but not actually used inCodableCBORDecoder, creating a DoS vulnerability where deeply nestedstructures could cause stack overflow.
Problem:
CodableCBORDecoderhad amaximumDepthproperty but it wasn't includedin the options computed property, so it defaulted to
.maxSingleValueDecodingContainercalledCBOR.decode()without passing optionsKeyedDecodingContainerandUnkeyedDecodingContainercreatedCBORDecoderinstances without passing options
Fix:
maximumDepthproperty toCodableCBORDecoder(public API)maximumDepthin options computed propertysetOptions()to setmaximumDepthCBOR.decode()calls inSingleValueDecodingContainerCBORDecoder()calls inKeyedDecodingContainerCBORDecoder()calls inUnkeyedDecodingContainer2. Implement full depth tracking across Codable container hierarchy
Changes:
currentDepthfield to_CBORDecoderand all container classes(
KeyedContainer,UnkeyedContainer,SingleValueContainer)currentDepththrough all container creation and nested decoding_CBORDecoderdecode()methodssuperDecoder()methodsPreviously, each
CBOR.decode()call would resetcurrentDepthto 0, allowingdeeply nested structures to bypass the depth limit. For example, a 10-level
nested array with
maximumDepth=5would incorrectly succeed. Now depth isproperly tracked across the entire container hierarchy.