Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Guard against XCM recursive bombs by setting a recursion limit #3555

Closed
wants to merge 42 commits into from
Closed

Guard against XCM recursive bombs by setting a recursion limit #3555

wants to merge 42 commits into from

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Aug 2, 2021

Part of #2841.

kianenigma
kianenigma reviewed Aug 2, 2021
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test? :p

@apopiak apopiak added the A0-please_review Pull request needs code review. label Aug 2, 2021
@KiChjang KiChjang added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 2, 2021
4meta5
4meta5 reviewed Aug 2, 2021
View reviewed changes
xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Aug 2, 2021

polkadot/runtime/parachains/src/ump.rs

Line 94 in 6a2c32c

VersionedXcm::<C::Call>::decode(&mut &data[..]).map(Xcm::<C::Call>::try_from);

Here we also need to restrict the decoding using https://docs.rs/parity-scale-codec/2.2.0/parity_scale_codec/trait.DecodeLimit.html#tymethod.decode_with_depth_limit

@apopiak apopiak mentioned this pull request Aug 3, 2021
Closed
47 tasks
kianenigma
kianenigma approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@KiChjang @kianenigma
Update xcm/xcm-simulator/example/src/lib.rs
76d9c7a 
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@shawntabrizi shawntabrizi changed the base branch from master to release-v0.9.9 August 6, 2021 16:29
@shawntabrizi shawntabrizi mentioned this pull request Aug 6, 2021
Merged
@KiChjang
Copy link
Contributor Author

KiChjang commented Aug 7, 2021

bot merge

@ghost
Copy link

ghost commented Aug 7, 2021

Trying merge.

@ghost
Copy link

ghost commented Aug 7, 2021

Merge failed: "You're not authorized to push to this branch. Visit https://docs.github.com/articles/about-protected-branches/ for more information."

@s3krit s3krit mentioned this pull request Aug 9, 2021
Merged
3 tasks
@gavofyork
Copy link
Member

Patched onto 0.9.9 directly.

@gavofyork gavofyork closed this Aug 9, 2021
@bkchr bkchr deleted the kckyeung/harden-xcm branch August 9, 2021 19:23
@louismerlin louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apopiak apopiak apopiak left review comments

@4meta5 4meta5 4meta5 left review comments

@gavofyork gavofyork gavofyork approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

@pepyakin pepyakin Awaiting requested review from pepyakin

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
v0.9.9
Development

Successfully merging this pull request may close these issues.
9 participants
@KiChjang @bkchr @gavofyork @shawntabrizi @kianenigma @apopiak @4meta5 @s3krit @louismerlin