margelo / react-native-quick-crypto

⚡️ A fast implementation of Node's `crypto` module written in C/C++ JSI
Other
682 stars 82 forks source link

🐛 Signature is created wrong with EC based keys (?) #387

Closed Apollon77 closed 1 month ago

Apollon77 commented 1 month ago

What's happening?

We sign data with an sha256 hash and an EC based Private key and the response signature can not be used to verify the same data and is also too long (should be 64 bytes but is72 bytes)

Reproducible Code

const crypto = require('crypto'); // Here for node.js to proof it is working, when replaces with quick-crypto it fails

const privateKeyPem = `-----BEGIN PRIVATE KEY-----
    MIGTAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBHkwdwIBAQQgaTunuTuxJ0oduU8A
gchLiPEO5p8URkI0YyUlkNMR+8KgCgYIKoZIzj0DAQehRANCAAS2bcRIxh29Yf49
8bnSu4y3bmVDiJjg0SCWD1mHN8DC5gM8uAaTdnz2IYRsvy+UAbqMc8J1xBeQanwV
nkT8PPPD
-----END PRIVATE KEY-----`;

const publicKeyPem = `-----BEGIN PUBLIC KEY-----
    MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEtm3ESMYdvWH+PfG50ruMt25lQ4iY
4NEglg9ZhzfAwuYDPLgGk3Z89iGEbL8vlAG6jHPCdcQXkGp8FZ5E/Dzzww==
-----END PUBLIC KEY-----`;

const data = Buffer.from("lets try if we can check the crypto fun here", "utf8");

// Do the signing

const signer = crypto.createSign("sha256");
signer.update(data);
const signature = signer.sign({
    key: privateKeyPem,
    format: "pem",
    type: "pkcs8",
    dsaEncoding: "ieee-p1363",
});

console.log(signature.toString("base64"));
console.log("Signature length", signature.length);

// Do verify

const verifier = crypto.createVerify("sha256");
verifier.update(data);
const success = verifier.verify(
    {
        key: publicKeyPem,
        format: "pem",
        type: "spki",
        dsaEncoding: "ieee-p1363",
    },
    signature,
);

console.log("Verification success", success);

Relevant log output

Working with Node.js:
fPU/CnOIgecOiRci/rdtZZ3+2PeYCfQhCSrTgxMCfbt0KgPwyW90YNdJAsUhRJlkr7CkO651x707frQIZH+NdA==
Signature length 64
Verification success true

Not working with Quick-crypto in RN Simulator (but also on real device e.g. ios):
MEYCIQC6CnP6Eip5BXCgjlP6hjtKQNgn4yKX8Htczz2hls9D3wIhAJk2yz3vQENVs/qbkJQPLsZXSMP1Pz47l7nqqZLflJB0
Signature length 72
Verification success false

Device

Simulator and ios

QuickCrypto Version

0.7.1

Can you reproduce this issue in the QuickCrypto Example app?

I didn't try (⚠️ your issue might get ignored & closed if you don't try this)

Additional information

Apollon77 commented 1 month ago

I have no React native environment myself but two independent developers could reproduce this. I asked other developers to try in the quick crypto example app too ...

Apollon77 commented 1 month ago

Further test results point to ieee-p1363 dsaEncoding to be the issue.

Orzech99 commented 1 month ago

I recreated this issue in the Quick Crypto Example App

but ...

The first argument in both sign and verify methods is of type EncodingOptions presented below:

export type EncodingOptions = {
  key?: any;
  type?: KType;
  encoding?: string;
  format?: KFormat;
  padding?: number;
  cipher?: string;
  passphrase?: string | ArrayBuffer;
};

There is no dsaEncoding.

When I tried to pass ieee-p1363 to encoding I got the following error:

fail: ECDSA - error: Unknown encoding: ieee-p1363

So I looked through a source code and I realized that both sign and verify use this line

const dsaSigEnc = getDSASignatureEncoding(options);

to get the dsaEncoding from the options argument and then pass it to the internal verify.

Here we can see that this method handles the 'ieee-p1363'.

function getDSASignatureEncoding(options: any) {
  if (typeof options === 'object') {
    const { dsaEncoding = 'der' } = options;
    if (dsaEncoding === 'der') return DSASigEnc.kSigEncDER;
    else if (dsaEncoding === 'ieee-p1363') return DSASigEnc.kSigEncP1363;
    throw new Error(`options.dsaEncoding: ${dsaEncoding} not a valid encoding`);
  }

  return DSASigEnc.kSigEncDER;
}

I added the dsaEncoding to the type Encoding Options and run the test again, this is the result:

MEUCIQDAUh2lq+fkAEHHOcJPPIBz4XFAUPUHm8IWeT/SoHYboAIgafWT5PLuVwTw3oj4uRcjr864adyXbgdtmRj4TH2QaNwA
Signature length 72
fail: ECDSA - error: kSignMalformedSignature

I hope this helps.

TL;DR

The issue is with the dsaEncoding option, which doesn't exist in QuickCrypto's sign and verify methods. QuickCrypto's methods internally use a function that expects this option, leading to errors when it's provided. To try and fix the issue, I added dsaEncoding to the EncodingOptions type, but the signature length issue persisted, and the verification still failed with a kSignMalformedSignature error.

Apollon77 commented 1 month ago

I assume the reason is https://github.com/margelo/react-native-quick-crypto/blob/3aaa252f0817e040b18f5970efa3fa5d010c8952/cpp/Sig/MGLSignHostObjects.cpp#L288 … in sign the support for the encoding is not implemented but verify seems to support it.

Sooo if the code which is commented out works then that could be an easy fix and with the example above could be easy verifiable.

boorad commented 1 month ago

Please test this with 0.7.2 and let me know if things are looking good. I used your repro code as a new test in the suite, and it passes. But I didn't look at 64 vs 72 length...

Apollon77 commented 1 month ago

Thank you very much, we will verify in the next days!