fix!: fix payment id type confusion#6901
fix!: fix payment id type confusion#6901hansieodendaal wants to merge 2 commits intotari-project:developmentfrom
Conversation
Fixed payment id type confusion - adversaries can craft arbitrary PaymentId due to type confusion in deserialisation.
WalkthroughThe changes add an explicit type tag in the serialization and deserialization of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PaymentId
participant PTag
Client->>PaymentId: Serialize PaymentId
PaymentId->>PTag: Call to_tag() to get type tag bytes
PaymentId->>Client: Return tag prepended to serialized data
sequenceDiagram
participant Client
participant PaymentId
participant PTag
Client->>PaymentId: Deserialize PaymentId from bytes
PaymentId->>PTag: Use from_u8() to parse type tag from first byte
PaymentId->>Client: Process remaining bytes based on tag and return PaymentId
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs (1)
192-213: Return an error or safer fallback for unknown tag.Currently,
from_u8defaults toTransactionInfofor unrecognized values, which may cause unintended deserialization outcomes. Consider defaulting toPTag::Emptyor returning an error to avoid surprises and strengthen type safety.Here is a possible minimal adjustment:
- _ => PTag::TransactionInfo, + _ => PTag::Empty,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (22)
base_layer/core/src/transactions/transaction_components/encrypted_data.rs (22)
218-228: Consistent tagging logic.
to_tagappropriately distinguishes each variant by introducing a tag byte. Returning an empty vector forPaymentId::Emptyaligns well with the logic infrom_bytes, preventing unnecessary payload overhead.
232-234: Proper size accounting for U64 and U256.Adding 1 byte for the tag plus the fixed size of each variant is correct and consistent with
to_tag.
239-239: Accurate size for AddressAndData.Including 1 byte for the tag and 1 byte for the
tx_typeensures robust size calculation.
244-244: TransactionInfo size calculation.Adding the tag byte plus
SIZE_VALUE_AND_META_DATA(which includestx_type) and user data length is consistent with the updated serialization format.
386-389: Appending tag and U64 value.The logic to prepend the tag byte, followed by the little-endian
u64bytes, is clear and correct.
391-395: Appending tag and U256 value.Constructing a 32-byte buffer, writing the U256 in little-endian, and then appending it after the tag is correctly implemented.
398-399: Inserting Open variant tag and tx_type.It’s sensible to store the open variant tag byte, followed by
tx_type.as_bytes(), then user data. Matches the stated serialization plan.
408-409: AddressAndData: prepend tag and address.Including the
sender_addressbytes and then thetx_typeis well-structured. This keeps the format predictable.
420-421: TransactionInfo: prepend tag and amount.Serializing the tag first, then the 8-byte amount, facilitates consistent parsing on deserialization.
432-438: Graceful handling of empty bytes.Interpreting empty slices as
PTag::Emptyand then processing the remaining bytes is a clean approach that ensuresPaymentId::Emptyis handled consistently.
439-440: Immediate return for an empty PaymentId.This check properly short-circuits parsing and clarifies the logic flow.
443-443: Immediate return for U64 parsing.Extracting 8 bytes into a
u64and returning is straightforward and avoids additional overhead.
445-449: Immediate return for U256 parsing.Correctly brushing 32 bytes into a U256 in little-endian form. Straightforward and efficient.
449-470: Flexible address parsing for AddressAndData.The dual- or single-address checks correctly match variable lengths, with graceful fallback upon parse failure.
471-493: Parsing TransactionInfo with structured offsets.Reading amount, metadata, and addresses in order is well-structured. Good approach ensuring partial address parse does not break the entire flow.
534-535: Safe minimal catch-all.Quietly ignoring other cases here is intentional, but ensure any truly malformed or unexpected data yields a safe fallback.
536-544: Fallback to Open variant.Unrecognized data eventually falls back to
PaymentId::Open, which is consistent with the design of enabling partial user data storage. Keep an eye on potential confusion if the data was intended for a recognized variant.
821-877: Comprehensive confusion test.The test methodically checks for trick payloads that could be misparsed between
OpenandTransactionInfo. Excellent coverage against type confusion attacks.
896-901: Open variant with no data.Straightforward addition to the test matrix, ensuring coverage for empty user data.
911-920: AddressAndData (dual) with no data.Covers corner cases of minimal user data while ensuring the new tag logic still holds for addresses.
938-944: AddressAndData (single) with no data.Confirms correct serialization-deserialization flow in a single-address scenario.
956-957: Test labeling updated.Only a remark describing the new “TransactionInfo - single + amount, no data”. No additional review required.
Test Results (CI) 3 files 129 suites 42m 38s ⏱️ Results for commit 0b8c0cc. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)36 tests 36 ✅ 16m 28s ⏱️ Results for commit 0b8c0cc. |
|
replaced by: #6974 |
2 participants
Description
Fixed payment ID type confusion - adversaries can craft arbitrary PaymentId due to type confusion in deserialisation.
Fixes #6891
Motivation and Context
See #6891
How Has This Been Tested?
New unit test added
Expanded current unit tests
What process can a PR reviewer use to test or verify this change?
Breaking Changes
BREAKING CHANGE: The new
PaymentIdserialization specification introduces a type byte as the first byte. Any payment id's that were been generated with prior versions will be deserialized as either open or empty.Summary by CodeRabbit