crypto: relax failure message length check #59
Conversation
| @@ -250,9 +250,9 @@ func (o *OnionErrorDecrypter) DecryptError(encryptedData []byte) ( | |||
| *DecryptedError, error) { | |||
|
|
|||
| // Ensure the error message length is as expected. | |||
| if len(encryptedData) != onionErrorLength { | |||
| if len(encryptedData) < minOnionErrorLength { | |||
There was a problem hiding this comment.
To avoid a dos vector of trying to read very very large messages, should there be an upper limit? Or is that handled by the wire message size limits
There was a problem hiding this comment.
I don't think lightning-onion is the place to do it, as this is all independent of the wire message format.
But good question. I'd say there are checks higher up. Found this one (casting to uint16), but not sure if there are more:
https://github.com/lightningnetwork/lnd/blob/59a5f53cfe8b43b9caa70b5244dce4cd6813c042/brontide/noise.go#L869
There was a problem hiding this comment.
Yeah this'll be bounded ultimately by what can fit into a single p2p message (65 KB). We can add more constraints on the parsed error itself on the link level.
|
With his change we'll only check that the length is above some minimum. Is that independent to the spec change proposed here: lightning/bolts#1021 ? As that's only about what the sender should do. (so we can't enforce that the read error is exactly 1024 bytes) |
|
It is independent. We should already have allowed variable length failure messages. |
|
Wondering now, should the minimum check be removed too? If we'd receive a shorter failure message today, wouldn't we want to locate the error source and parse the message? On the other hand, failure messages shorter than 256 bytes may complicate the implementation in lnd: lightningnetwork/lnd#6913 (comment) |
I would say we should keep it for now. It would be valuable to check what other implementations are doing; if they all send (at least) 256 bytes, we could make this required in the spec. |
|
Updated spec: lightning/bolts#1021 (comment) |
The spec does not dictate but only recommends a length of 256 bytes. Future tlv extensions may push the failure message length over this limit. With this change, receivers can ignore the lengthier extensions without handling it as an unreadable failure.
8b364a6 to
d83e7f0
Compare
| @@ -250,9 +250,9 @@ func (o *OnionErrorDecrypter) DecryptError(encryptedData []byte) ( | |||
| *DecryptedError, error) { | |||
|
|
|||
| // Ensure the error message length is as expected. | |||
| if len(encryptedData) != onionErrorLength { | |||
| if len(encryptedData) < minOnionErrorLength { | |||
There was a problem hiding this comment.
Yeah this'll be bounded ultimately by what can fit into a single p2p message (65 KB). We can add more constraints on the parsed error itself on the link level.
The spec does not dictate but only recommends a length of 256 bytes. Future tlv extensions may push the failure message length over this limit. With this change, receivers can ignore the lengthier extensions without handling it as an unreadable failure.