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.

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

Improper nonce handling #102

Closed
dnkolegov opened this issue Jul 5, 2021 · 3 comments · Fixed by #118
Closed

Improper nonce handling #102

dnkolegov opened this issue Jul 5, 2021 · 3 comments · Fixed by #118
Assignees

Comments

@dnkolegov
Copy link

dnkolegov commented Jul 5, 2021

There are two issues with nonce handling:

  1. Nonces are 32 bits long. The Noise spec requires 64-bit nonce.

https://github.com/NodeFactoryIo/js-libp2p-noise/blob/master/src/%40types/handshake.d.ts#L12-L15

  1. Nonce overflow.
    The spec requires returning an error if the maximum number is reached. This is the same issue as in the flynn package but more realistic due to 32-bit length.

https://github.com/NodeFactoryIo/js-libp2p-noise/blob/master/src/handshakes/abstract-handshake.ts#L46-L48

@mpetrunic
Copy link
Member

mpetrunic commented Jul 5, 2021

Hey @dnkolegov tnx for opening issue.

  1. Because of javascript limitations this would force us to use BigInt which will degrade performance to some degree. @vasco-santos should be switch nonce to BigInt?
  2. I think this is valid. Would it be ok if throw error when nonce is > MAX_SAFE_INTEGER?

@dnkolegov
Copy link
Author

dnkolegov commented Jul 5, 2021

Would it be ok if throw error when nonce is > MAX_SAFE_INTEGER?

Yes. That's correct. For example, https://github.com/flynn/noise/blob/master/state.go#L44-L46.

Because of javascript limitations this would force us to use BigInt which will degrade performance to some degree. should be switch nonce to BigInt?

It should be noted that all other libp2p-noise implementations use Noise with 64-bit nonce. My concern here is that your implementation will throw an exception while a peer having another implementation (e.g., go-libp2p) would process it correctly.

@mpetrunic
Copy link
Member

It should be noted that all other libp2p-noise implementations use Noise with 64-bit nonce. My concern here is that your implementation will throw an exception while a peer having another implementation (e.g., go-libp2p) would process it correctly.

Pretty sure we will close connection and new connection will start from 0 nonce. That said it's highly unlikely anyone will ever pass uint32 number of messages in single connection 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants