advisories: Eclair exhausts CPU and memory from a race that orphans channel actors - #13
Conversation
| The first `open_channel` inserts the channel; the second sees the entry and is dropped, so no orphan is ever created. | ||
| And with the race closed there are no duplicate admissions for the rate limiter to mishandle. | ||
|
|
||
| The same PR also hardened the early duplicate check to compare a `temporary_channel_id` against existing final `channel_id`s, addressing a related ID-confusion issue found by Erick Cestari. |
There was a problem hiding this comment.
@erickcestari I'll add the link to your disclosure here when it's published.
There was a problem hiding this comment.
I've published mine disclosure. Once the advisory is published, I’ll also update it to include a link to the advisory.
| } | ||
| ``` | ||
|
|
||
| The channel map is consulted, and if no duplicate is found an asynchronous round trip through the interceptor is initiated. |
There was a problem hiding this comment.
| The channel map is consulted, and if no duplicate is found an asynchronous round trip through the interceptor is initiated. | |
| The channel map is consulted, and if no duplicate is found, an asynchronous round trip through the interceptor is initiated. |
Arvin21M
left a comment
There was a problem hiding this comment.
Approved, with a few edit requests.
Marked as approved because the suggested edits are not critical.
| Eclair spawns two channel actors for the `temporary_channel_id`, and each answers with its own `accept_channel`. | ||
| The second insertion overwrites the first under the same key, and the first actor becomes orphaned: | ||
|
|
||
| - it is no longer in the channel map, so no further messages can reach it, and |
There was a problem hiding this comment.
| - it is no longer in the channel map, so no further messages can reach it, and | |
| - it is no longer in the channel map, so no further channel messages routed through the `Peer` can reach it, and |
| case Event(TickChannelOpenTimeout, _) => stay() | ||
| ``` | ||
|
|
||
| The only thing that reaps the orphan is a peer disconnect, so the orphan stays in memory as long as the attacker remains connected. |
There was a problem hiding this comment.
| The only thing that reaps the orphan is a peer disconnect, so the orphan stays in memory as long as the attacker remains connected. | |
| The only thing that reaps the orphan in testing is a peer disconnect, so the orphan stays in memory as long as the attacker remains connected. |
There was a problem hiding this comment.
I skipped this suggestion. It's true that nothing else reaped it in testing, but I'm also fairly confident that nothing else can reap it at all.
| - the node froze, unable to process incoming messages, until the attacking peer was eventually disconnected. | ||
| On disconnect, the leaked memory was freed and Eclair was able to recover. | ||
|
|
||
| The Eclair node came back on restart or disconnect, and nothing was lost. |
There was a problem hiding this comment.
| The Eclair node came back on restart or disconnect, and nothing was lost. | |
| The Eclair node came back on restart or disconnect, and nothing was lost in testing. |
There was a problem hiding this comment.
Skipped this suggestion. I think "in testing" is redundant since the entire section is describing what we observed in our test.
|
|
||
| The Eclair node came back on restart or disconnect, and nothing was lost. | ||
| The attacker could repeat the attack, but it took four more hours to reach the end state again. | ||
| Eclair had plenty of time to handle on-chain events, and funds were not seriously at risk. |
There was a problem hiding this comment.
| Eclair had plenty of time to handle on-chain events, and funds were not seriously at risk. | |
| Eclair had plenty of time to handle on-chain events in testing, and no loss of funds were observed. |
There was a problem hiding this comment.
I agree the "funds were not seriously at risk" statement may be too strong. But I don't want to say "no loss was observed" since we didn't even try to steal funds in the experiment.
I've changed the sentence to "This delay gives Eclair plenty of time to handle on-chain events, so the risk to funds is likely low." I think this hedges enough without implying we tried to steal funds.
|
|
||
| Because the `Peer` actor processes one message at a time and the check now shares its turn with the insertion, there is no longer a gap for a second `open_channel` to race through. | ||
| The first `open_channel` inserts the channel; the second sees the entry and is dropped, so no orphan is ever created. | ||
| And with the race closed there are no duplicate admissions for the rate limiter to mishandle. |
There was a problem hiding this comment.
Consider editing to:
"And with the race closed there are no orphaned actors for the rate limiter behavior to leave behind."
There was a problem hiding this comment.
I agree that "duplicate admissions" sounds funny, but it's not 100% accurate to say the rate limiter was leaving behind orphans.
Instead I changed the phrase to "duplicate temporary_channel_ids".
c2f249b to
7cd1376
Compare
|
Thank you all for the reviews! |
No description provided.