croc icon indicating copy to clipboard operation
croc copied to clipboard

Add Curve25519 as option

Open samuel-lucas6 opened this issue 2 years ago • 7 comments

Is your feature request related to a problem? Please describe. SIEC is currently the default curve, despite it being unknown and receiving insufficient cryptanalysis. It looks as if it was chosen because you're related to the designer. Then the NIST curves have seeds that haven't been explained, leading some cryptographers to question whether they were maliciously manipulated. Regardless of whether they were or not, the very possibility is enough to argue against their use unless required for interoperability.

Describe the solution you'd like At the very least, P-256 should be the default curve. However, support for Curve25519 would be better as it's now one of the main curves used in modern protocols, such as WireGuard and TLS 1.3.

samuel-lucas6 avatar Apr 04 '22 21:04 samuel-lucas6

I'd happily accept a PR for handling Curve25519. You can make one here: https://github.com/schollz/pake

Otherwise, if you just want to make P-256 the default curve you can do so pretty easily in the code by chaing this line: https://github.com/schollz/croc/blob/master/src/cli/cli.go#L97

schollz avatar Apr 04 '22 22:04 schollz

I'd happily accept a PR for handling Curve25519. You can make one here: https://github.com/schollz/pake

I currently don't code in Go, and I would argue this situation is more complicated than just adding Curve25519. Now that it's the morning, I would argue it's best if SIEC is removed completely and only one curve is used in croc. What's the rationale for allowing the curve to be changed?

Otherwise, if you just want to make P-256 the default curve you can do so pretty easily in the code by chaing this line: https://github.com/schollz/croc/blob/master/src/cli/cli.go#L97

Is this as a PR for everyone or just a change for a custom build? Because my intention with this issue is to hopefully improve the security of the program for everyone, not just myself if I go on to use this tool.

samuel-lucas6 avatar Apr 05 '22 08:04 samuel-lucas6

@samuel-lucas6 Even if you are new to Go - adding Curve25519 should not be too hard: the only reason it hasn't been added is being AFAIK there is not a Go port for curve25519 that encapsulates the Curve type used for all the other curves here. There is an implementation by keybase but it is missing the Add function and the Go std lib implementation is missing Add and IsOnCurve. You can take either of those as a starting point and just add the missing functions. That would be greatly appreciated.

I would argue this situation is more complicated than just adding Curve25519

Please make a new issue if you want to discuss this. I'd like to keep this issue open in case anyone wants to add Curve25519 and keep this issue related to that.

schollz avatar Apr 05 '22 12:04 schollz

I'm frankly baffled by your response. Your library has 18.9k stars, but you're using an experimental curve as the default, the only other curves you support may have malicious seeds, and you allow the user to change the curve, which is completely unnecessary. Your use of SIEC is a vulnerability, and yet you come across as unconcerned and seem to be trying to deflect the conversation. Then you're suggesting I should start tinkering with cryptographic code in a language I have practically no experience with, despite it being 'not too hard' for you to do yourself.

Why would you put all this work into a cool and useful project to then go and use experimental cryptography? This has to be the largest project I've seen get away with something like this.

samuel-lucas6 avatar Apr 05 '22 18:04 samuel-lucas6

Hi @samuel-lucas6, you opened this issue requesting support for Curve25519. Let's keep the discussion in this issue focused on code changes nessecary for implementing Curve25519. I'd appreciate if you can make a new issue to discuss your other claims. Especially if you know of a vulnerability in SIEC - I would really appreciate your report related to that.

schollz avatar Apr 05 '22 18:04 schollz

@samuel-lucas6 @schollz I've started a PR for adding the twisted Edwards25519 curve into PAKE. It's not the exact same curve, but birationally equivalent so effectively would close this issue if changes got fixed/merged/pulled. https://github.com/schollz/pake/pull/8

thearchitector avatar Feb 26 '23 01:02 thearchitector

This curve also makes it hard to create custom clients that follows the croc protocol in other programming languages... while I could use FFI to use pake I'd prefer not adding an other runtime to my project...

AsafFisher avatar Jul 15 '23 15:07 AsafFisher

Stale issue message

github-actions[bot] avatar Feb 25 '24 12:02 github-actions[bot]