MystenLabs/ed25519-unsafe-libs

horse25519 is not a library

Yawning opened this issue · 6 comments

The repository is intended to provide a standalone executable that does ed25519 vanity keypair generation. It includes a copy of djb's ref10 ed25519 implementation lifted from supercop to avoid pulling in another dependency.

While it does use the API in an odd way, this is intentional as it already is doing something extremely exotic and unusual with respect to key generation.

https://github.com/Yawning/horse25519/blob/master/Makefile#L57

Edit: Corrected why the included sign API is somewhat wacky.

Thanks for the update, I'll push a PR to remove it from the official list and explain that carrying the vulnerable public ref10 api is not meant to be used externally. In theory though you can still have a copy of that lib but make that function private/protected so the vulnerable api is not exposed in the first place.

@Yawning a possible clear solution to avoid any accidental public exposure of the vulnerable sign api of the horse25519 repo is to completely remove the public sign api (and any other non-used api) from the copy pasted ref10 dep.

I totally agree that the purpose of horse25519 is different but I've seen many times devs reusing another lib's code for a different than the intended purpose, either directly or as a dependency. As a rule of thumb, if a project carries publicly exposed functions, we cannot guarantee other projects/devs are not going to accidentally invoke them.

@kchalkias I don't disagree in principle, and acknowledge that people do tend to copy/paste and or cargo-cult a good amount of code.

That said, I would hope that no one will copy/paste things from the innards of a long-abandoned proof of concept, in a repository that I archived years ago. While I may still be open to fixing this since I can probably just remove the sign api, there has to be a way for software authors to disown responsibility for maintaining code (short of deleting the repository, since the concept that I was trying to demonstrate is still useful).

Whowa, didn't think my comment in my repository would close this, sorry about that.

As the PR notes, I went and changed my modified crypto_sign to do the scalar-basepoint multiply, so even if someone copy-pastes it, it will be "safe". It is somewhat unlikely that anyone would use a crypto_sign that takes an expanded form private key, but, I've seen code I wrote crop up in weirder places.

As a side note, thanks for the EdDSA verification semantics paper from a while back, it came in handy for a separate project.

I guess I'll reopen this since the issue is "addressed", sorry for the extra work.

Amazing job and a huge community thanks for your quick response and understanding; updated our the readme doc by putting horse25519 to the list of already fixed libs and pointing to @Yawning's PR.