Open jlokier opened 3 years ago
Can confirm thal on my Goerli block collection test. Reproducible event:
/home/jordan/nimbus-eth1/nimbus/nimbus.nim(264) nimbus /home/jordan/nimbus-eth1/nimbus/nimbus.nim(226) process /home/jordan/nimbus-eth1/vendor/nim-chronos/chronos/asyncloop.nim(279) poll /home/jordan/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(74) colonanonymous /home/jordan/nimbus-eth1/vendor/nim-eth/eth/p2p/rlpx.nim(619) dispatchMessages /home/jordan/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/chcks.nim(23) raiseIndexError2 /home/jordan/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(49) sysFatal [[reraised from: /home/jordan/nimbus-eth1/nimbus/nimbus.nim(264) nimbus /home/jordan/nimbus-eth1/nimbus/nimbus.nim(226) process /home/jordan/nimbus-eth1/vendor/nim-chronos/chronos/asyncloop.nim(279) poll /home/jordan/nimbus-eth1/vendor/nim-chronos/chronos/asyncmacro2.nim(96) colonanonymous ]] Error: unhandled exception: index 33 not in 0 .. 32 [IndexError]
with always the same index bat varying time until the error occurs (best run was to collect 500k blocks which was enough for my replay.)
Can confirm
No, it's not the same issue. What you've reported there is a different, unrelated bug also in RLPx.
Look at the exception: It's an IndexError
rather than a RangeError
, and it's raised in a different area of the code, which is shown in the stacktrace: rlpx.nim(619) dispatchMessages
versus rlpx.nim(1170) rlpxAccept
.
These stacktrace may be confusing if you're not used to Chronos. It's good to ignore the [[reraised from:...]]
section. It's just an artifact of how exceptions are processed in a proc with pragma {.async.}
.
It's a useful report though, thanks, so I've filed it as issue #767.
Looks like a check is needed on the input versus that eciesOverheadLength
. This is used on some other locations too, and yes, in general the code will need systematically checking on input sizes and such. I don't think in the past much attention has gone to this in the eth1 code.
There is a decoding vulnerability to untrusted network data in the RLPx auth code.
While connected to Ropsten test network, Nimbus-eth1 raised this error in
rlpxAccess
/decodeAuthMessageEip8
and took down the process.It would be appropriate to go through and systematically check for similar length trust issues in all input in RLPx.
The Geth version shown is interesting. The hex part of
clientId
is not a valid commit in the Geth repo. Is it a branched old version of Geth, in which case perhaps it's correct protocol we are incorrectly decoding? Or is it a fake version, intentionally sending out adversarial messages?(Ignore the
eth/65
mention, this is running a small patch that doesn't touch any relevant code.