Fix panic when deserializing Duration
#4172
Conversation
`Duration::new` adds any nanoseconds in excess of a second to the second part. This can overflow, however, panicking. In 0.2 we introduced a few further cases where we store `Duration`s, specifically some when handling network messages. Sadly, that introduced a remotely-triggerable crash where someone can send us, for example, a malicious blinded path context which can cause us to panic. Found by the `onion_message` fuzzer
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4172 +/- ##
==========================================
+ Coverage 88.78% 88.79% +0.01%
==========================================
Files 180 180
Lines 137066 137068 +2
Branches 137066 137068 +2
==========================================
+ Hits 121694 121715 +21
+ Misses 12552 12538 -14
+ Partials 2820 2815 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
Backported to 0.2 in #4185. |
|
Backporting to 0.1 in #4235 |
4 participants
Duration::newadds any nanoseconds in excess of a second to the second part. This can overflow, however, panicking. In 0.2 we introduced a few further cases where we storeDurations, specifically some when handling network messages.Sadly, that introduced a remotely-triggerable crash where someone can send us, for example, a malicious blinded path context which can cause us to panic.
Found by the
onion_messagefuzzer.This doesn't seem super critical in 0.1, its basically only a reachable panic when deserializing
ChannelManager(not a huge deal) or a scorer (which isn't great cause that can come from a third-party, but usually they're at least trusted enough to not be feeding you malicious panic-y crap). Still, worth backporting there in case we do another point release at some point.