Closed tex0l closed 2 years ago
JiriHoffmann
commented
2 years ago Well, yes and no... If you are passing data as a string it has to be encoded but if you are passing it as an ArrayBuffer it's reasonable to assume that it will not be encoded as you mentioned. It would make sense to only use the encoding parameter for strings. The only issue is that utility functions should be able to use it for both a string and an ArrayBuffer so it could be confusing.
tex0l
commented
2 years ago I'm uncertain if I understand what you mean.. If I give an ArrayBuffer but the encoding is set at base64, will it interpret my data as base64-encded?
Having different parameter types and encodings for similar kinds of data is confusing to me.
The best interface (in my opinion) would be to have a common encoding setting for data, IV, key and the return value which could be either:
ArrayBuffer which contains binary data;base64 encoded string;hex encoded stringand leave the developer encode to and decode from one of these three encodings, as the encoding from UTF-8 to raw data is usually done at a higher level in the stack. Do you think it would be possible?
JiriHoffmann
commented
2 years ago . If I give an
ArrayBufferbut the encoding is set atbase64, will it interpret my data as base64-encded?
Yes, that's how it's set up right now.
Having different parameter types and encodings for similar kinds of data is confusing to me.
Definitely agreed! There has to be a unified way of passing parameters. ArrayBuffers should contain raw binary data that is not encoded. With strings, I'm not sure what makes the most sense because key and IV will most likely be hex encoded but data will bebase64 encoded.
tex0l
commented
2 years ago Digging further this issue, it's really blocking the development for me as I need to be able to encrypt a symmetric key with RSA, and currently the decrypt casts the symmetric key as a UTF8 string, which cannot work..
I saw that it's not possible to return an ArrayBuffer naively, and it's necessary to do a memcpy.
I implemented alternative encrypt & decrypt functions for RSA (I hardcoded the encryption scheme because to simplify) to prototype if what I wanted to do would work:
auto rsa_encrypt_array_buffer = jsi::Function::createFromHostFunction(
jsiRuntime, jsi::PropNameID::forAscii(jsiRuntime, "rsa_encrypt_array_buffer"), 2,
[](jsi::Runtime &rt, const jsi::Value &thisValue, const jsi::Value *args,
size_t count) -> jsi::Value {
std::string resultAsString;
rncryptopp::RSA::encryptArrayBuffer(rt, args, &resultAsString);
jsi::Function array_buffer_ctor =
rt.global().getPropertyAsFunction(rt, "ArrayBuffer");
jsi::Object obj =
array_buffer_ctor.callAsConstructor(rt, static_cast<int>(resultAsString.size())).getObject(rt);
jsi::ArrayBuffer buff = obj.getArrayBuffer(rt);
std::copy(resultAsString.begin(), resultAsString.end(), buff.data(rt));
return obj;
});
auto rsa_decrypt_array_buffer = jsi::Function::createFromHostFunction(
jsiRuntime, jsi::PropNameID::forAscii(jsiRuntime, "rsa_decrypt_array_buffer"), 2,
[](jsi::Runtime &rt, const jsi::Value &thisValue, const jsi::Value *args,
size_t count) -> jsi::Value {
std::string resultAsString;
rncryptopp::RSA::decryptArrayBuffer(rt, args, &resultAsString);
jsi::Function array_buffer_ctor =
rt.global().getPropertyAsFunction(rt, "ArrayBuffer");
jsi::Object obj =
array_buffer_ctor.callAsConstructor(rt, static_cast<int>(resultAsString.size())).getObject(rt);
jsi::ArrayBuffer buff = obj.getArrayBuffer(rt);
std::copy(resultAsString.begin(), resultAsString.end(), buff.data(rt));
return obj;
});and
void encryptArrayBuffer(jsi::Runtime &rt, const jsi::Value *args, std::string *result) {
const jsi::Value &value = args[0];
if (!value.isObject()) {
throwJSError(rt, "RNCryptopp: RSA encrypt data is not an ArrayBuffer");
return;
}
auto obj = value.asObject(rt);
if (!obj.isArrayBuffer(rt)) {
throwJSError(rt, "RNCryptopp: RSA encrypt data is not an ArrayBuffer");
return;
}
auto buf = obj.getArrayBuffer(rt);
std::string data = std::string((char *)buf.data(rt), buf.size(rt));
std::string publicKeyString;
if (!stringValueToString(rt, args[1], &publicKeyString))
throwJSError(rt, "RNCryptopp: RSA encrypt publicKey is not a string");
StringSource PKeyStringSource(publicKeyString, true);
CryptoPP::RSA::PublicKey publicKey;
PEM_Load(PKeyStringSource, publicKey);
AutoSeededRandomPool rng;
RSAES<OAEP<SHA1>>::Encryptor e(publicKey);
StringSource(data, true,
new PK_EncryptorFilter(
rng, e, new StringSink(*result)));
}
void decryptArrayBuffer(jsi::Runtime &rt, const jsi::Value *args, std::string *result) {
const jsi::Value &value = args[0];
if (!value.isObject()) {
throwJSError(rt, "RNCryptopp: RSA decrypt data is not an ArrayBuffer");
return;
}
auto obj = value.asObject(rt);
if (!obj.isArrayBuffer(rt)) {
throwJSError(rt, "RNCryptopp: RSA decrypt data is not an ArrayBuffer");
return;
}
auto buf = obj.getArrayBuffer(rt);
std::string data = std::string((char *)buf.data(rt), buf.size(rt));
std::string privateKeyString;
if (!stringValueToString(rt, args[1], &privateKeyString))
throwJSError(rt, "RNCryptopp: RSA decrypt privateKey is not a string");
StringSource PKeyStringSource(privateKeyString, true);
CryptoPP::RSA::PrivateKey privateKey;
PEM_Load(PKeyStringSource, privateKey);
AutoSeededRandomPool rng;
RSAES<OAEP<SHA1>>::Decryptor e(privateKey);
StringSource(data, true,
new PK_DecryptorFilter(rng, e, new StringSink(*result)));
}
It works.
The way it could work in a proper release could be:
base64 or hex string, this needs to be defined, but not configurable imho;What do you think? Would you be open to a PR (on RSA & AES at least)?
By the way, what's your development process? I based my work on main, but it was broken because of detox thingies, then I moved to your feature branch detox-testing, but it's also broken. I went from there and fixed things here and there (detox still doesn't work).
JiriHoffmann
commented
2 years ago Hey, sorry just got back from vacation. You are right, main is kinda a mess right now. I was rushing to get everything done before I left and then I ran out of time.
The way it could work in a proper release could be:
- the key would be
base64orhexstring, this needs to be defined, but not configurable imho;- the output data would be in the same format as in the input data, if you give a (binary) ArrayBuffer, you get a binary ArrayBuffer, and if you give a string, it would be interpreted as utf8 by default, unless an optional argument says otherwise is given (base64 or hex)
Yes, originally I was trying to avoid this to simplify the codebase, but I didn't realize it will cause issues. I'm open to a PR but I was gonna fix it for the next release anyways. It should be like this for everything probably. If the input is an ArrayBuffer, the returned value will also be an ArrayBuffer. For strings, a string will be returned based on a predefined encoding. So if you are fine just using your patched version, for now, I will hopefully get this fixed in the next 2 weeks.
tex0l
commented
2 years ago No worry! I'm more than fine if you spearhead this initiative, a PR that changes the signatures of all the functions is a rewrite basically :), you are most definitely more qualified than me to do so, and 2 weeks is soon enough for me!
In the meantime, I will check that I have all the functions I need for sscrypto, and that they are compatible with our other implementations.
I don't know if you've seen my other issues, one that I couldn't find a fix for is the compiling of cryptopp for iOS, which fails for the iPhoneSimulator for arm64 (on an M1 mac I believe). Your sed to add it to the setenv fails with a BSD-sed, you should probably rewrite it with a BSD-sed rather than a GNU-sed, or orchestrate the modifications of the setenv with JS. Don't hesitate to ask me if you need me to test or code something.
JiriHoffmann
commented
2 years ago Yeah, it is basically a rewrite as that change should be reflected on other functions as well not just RSA & AES. I have looked at the other issues that you posted and they are super helpful cause it's impossible for me to test everything myself, so keep them rolling in! Some of them should be easy fixes, some of them I will have to look into, but I'll post updates as I go through them and make changes.
tex0l
commented
2 years ago I will, and thank you for your reactivity!
To give you a bit of background, my goal is to implement a version of sscrypto optimized for RN with rn-cryptopp. To do so, I test every primitive I need against my JS reference implementation, and whenever I'm unable to make it work, I find why and make an issue 🤷.
So you know, the primitives we use are:
So far, I've tested:
I'll let you know of my findings as they come! :)
JiriHoffmann
commented
2 years ago Alright, I finally got to rewriting the CPP portion of the library. Functions should now return the same data types as their respective inputs. Meaning:
Please check it out and me know if you run into any other issues! I am planning on adding some E2E tests and making a GH action for it (that's why I added the detox-testing branch originally), but I just don't have enough time at the moment.
Also, I will be looking into chuck by chunk hashing and HMAC/CMAC which shouldn't take too long, while we wait for more updates on the RSA issue in cryptopp.
tex0l
commented
2 years ago Hi @JiriHoffmann Great, thank you for the wonderful work!
I'll check it out in the coming days. Since the rewrite is done, If I find stuff that still need fixing, I may be able to make some atomic PRs :)
On the RSA salt length issue (https://github.com/JiriHoffmann/react-native-cryptopp/issues/17), there's indeed still something going on with cryptopp directly, we won't be able to set the salt length fully dynamically like node crypto API does, however we'll be able to optionally have the same behavior as the default RSA-PSS signature of Node.js that uses the OpenSSL flag for max salt length.
However, there's still an issue on the RSA signatures in general which are not self-compatible (you can't verify nor recover a signature made by rn-cryptopp with the PSSR mode): https://github.com/JiriHoffmann/react-native-cryptopp/issues/16, I may be able to make a PR to fix it (independently of the salt length issue).
tex0l
commented
2 years ago Hi @JiriHoffmann I can confirm the AES and HMAC work like a charm with ArrayBuffer as I/O :)
I managed to encrypt and decrypt at 18.7MB/s (AES CBC & HMAC) compared to a full JS implementation at 0.4MB/s! Quite a game changer!
Hi!
Thank you for the wonderful work on this library, I couldn't manage to bind properly the JSI with a cpp crypto library (I tried with Botan).
I'm trying to code a version of https://github.com/seald/sscrypto optimized for react-native, and I'm unable to pass a data which is an ArrayBuffer to Cryptopp.AES.decrypt, because it seems that you use the encoding parameter for the data argument (https://github.com/JiriHoffmann/react-native-cryptopp/blob/fd4c948f6632d7a237d233dccff6e050af194735/cpp/aes-candidates.cpp#L79).
From my understanding of the documentation, it should be used only for the encoding of the return value of the function, not the encoding of the input value, right?