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

Broken elliptic curve identities #994

Closed
mkskeller opened this issue Dec 30, 2020 · 3 comments
Closed

Broken elliptic curve identities #994

mkskeller opened this issue Dec 30, 2020 · 3 comments
Labels

Comments

@mkskeller
Copy link

mkskeller commented Dec 30, 2020

It seems that c9ef942 breaks basic elliptic curve identities. The following outputs three times 1 before and three times 0 after:

#include "eccrypto.h"
#include "oids.h"
#include <iostream>

int main()
{
  using namespace CryptoPP;
  auto params = DL_GroupParameters_EC<ECP>(ASN1::secp256k1());
  auto id = params.ExponentiateBase(0);
  std::cout << (id == params.GetCurve().Add(id, id)) << std::endl;
  auto gen = params.ExponentiateBase(1);
  std::cout << (gen == params.GetCurve().Add(id, gen)) << std::endl;
  std::cout << (params.ExponentiateBase(2) == params.GetCurve().Add(gen, gen)) << std::endl;
  return not (id == params.GetCurve().Add(id, id));
}

I'm using Ubuntu 20.04 with the system GCC.

@noloader
Copy link
Collaborator

noloader commented Dec 30, 2020

Ugh, thanks @mkskeller.

I'm going to back out most of the constant-time changes. My thinking is, the lesser of the two evils is the timing leak. The worse case is arriving at an incorrect result, and that is what currently happens.

Revert at Commit 4e56a6393dce.

For the upcoming Crypto++ 8.4 release, we re-activated CVE-2019-14318. The release notes can be found here.

Ping @mouse07410 .


Here is a simpler test program. It takes advantage of the fact the default constructor yields the identity element:

DL_GroupParameters_EC<ECP> params(oid);
DL_GroupParameters_EC<ECP>::Element id;
id = params.GetCurve().Add(id, id);
bool res = params.IsIdentity(id);

@noloader noloader added the Bug label Dec 30, 2020
noloader added a commit that referenced this issue Dec 30, 2020
Marcel Keller reports it broke curve operations
@noloader
Copy link
Collaborator

noloader commented Dec 30, 2020

Distro maintainers,

Here is the patch needed to back-out the broken code. The patch is a git diff. It can be applied against the Crypto++ 8.3.0 release.

cryptopp-gh994-fix.diff.zip.

EAddario pushed a commit to EAddario/cryptopp that referenced this issue Dec 30, 2020
Marcel Keller reports it broke curve operations
@mouse07410
Copy link
Collaborator

I concur.

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

No branches or pull requests

3 participants