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

🐛 BUG: EncryptDanger() avoids calling cipher.Encrypt() #770

Open
kazzmir opened this issue Oct 25, 2022 · 1 comment
Open

🐛 BUG: EncryptDanger() avoids calling cipher.Encrypt() #770

kazzmir opened this issue Oct 25, 2022 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@kazzmir
Copy link
Contributor

kazzmir commented Oct 25, 2022

What version of nebula are you using?

master as of today

What operating system are you using?

linux

Describe the Bug

out = s.c.(cipher.AEAD).Seal(out, nb, plaintext, ad)

this line bypasses the s.c.Encrypt() function and directly calls Seal(), I believe for the following reasons:

  1. it avoids allocating a []byte due to the noise library's use of c.nonce(n) that converts a uint64 to a []byte
  2. there is only one cipher in noise anyway, which simply calls Seal().

however I have a fork of noise where I implemented a new cipher suite (meaning a new implementation of Encrypt() and Decrypt()). nebula will not invoke my implementation's Encrypt/Decrypt function due to bypassing those functions entirely. Is the main reason to bypass Encrypt() because of the garbage collection issue due to the nonce?

Locally I have changed nebula's noise.go file to be like this

// EncryptDanger:
out = s.c.Encrypt(out, n, ad, plaintext)
// DecryptDanger
return s.c.Decrypt(out, n, ad, ciphertext)

If the garbage collection issue is the major concern it may be less of a concern due to more recent versions of the go garbage collector. I can do some measurements if so.

Logs from affected hosts

No response

Config files from affected hosts

No response

@nbrownus
Copy link
Collaborator

Yes, the garbage collector is the reason for this behavior. flynn/noise#30 was opened a while ago to try and address this upstream but hasn't seen any traction yet.

I agree that is may be less of an issue with modern go but I imagine that if you ran a test long enough to have the garbage collector kick in you will see why we do it.

@wadey wadey added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants