Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: relax failure message length check #59

Merged
merged 1 commit into from
Dec 2, 2022
Merged

crypto: relax failure message length check #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions crypto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,10 @@ func onionEncrypt(sharedSecret *Hash256, data []byte) []byte {
return p
}

// onionErrorLength is the expected length of the onion error message.
// Including padding, all messages on the wire should be 256 bytes. We then add
// the size of the sha256 HMAC as well.
const onionErrorLength = 2 + 2 + 256 + sha256.Size
// minOnionErrorLength is the minimally expected length of the onion error
// message. Including padding, all messages on the wire should be at least 256
// bytes. We then add the size of the sha256 HMAC as well.
const minOnionErrorLength = 2 + 2 + 256 + sha256.Size

// DecryptError attempts to decrypt the passed encrypted error response. The
// onion failure is encrypted in backward manner, starting from the node where
Expand All @@ -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 {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return nil, fmt.Errorf("invalid error length: "+
"expected %v got %v", onionErrorLength,
"expected at least %v got %v", minOnionErrorLength,
len(encryptedData))
}

Expand Down
2 changes: 1 addition & 1 deletion obfuscation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestOnionFailure(t *testing.T) {
// able to receive the error not only from last hop.
errorPath := paymentPath[:len(paymentPath)-1]

failureData := bytes.Repeat([]byte{'A'}, onionErrorLength-sha256.Size)
failureData := bytes.Repeat([]byte{'A'}, minOnionErrorLength-sha256.Size)
sharedSecrets, err := generateSharedSecrets(paymentPath, sessionKey)
if err != nil {
t.Fatalf("Unexpected error while generating secrets: %v", err)
Expand Down