rsa/ecb/oaepwithsha-256andmgf1padding support? about pc-dart HOT 9 OPEN

To add some weight to this issue, many implementations out there (including the recommended implementation of WebCrypto) of RSA-OAEP is using SHA-256 for hashing and MGF.

The implementation currently in pointycastle is hardcoding SHA1 for those, and it makes interoperability between, say, a Native Flutter and a Web-based application to be broken.

I tried playing in the internals of asymmetric/oaep.dart but I'm not cryptographer, so I feel clueless to help unfortunately.

from pc-dart.

Thanks @mwcw! I did a quick implementation that seems to work, although I'm not sure it's up to the PointyCastle standard right now: braverhealth@0be621f

To avoid clashing with existing API, I simply added an entirely new class that is a copy of pretty much the entire existing SHA1-based OAEP class, but using SHA-256 where appropriate. Tests against a reference implementation (from a Python equivalent library) indicate that encryption/decryption work fine with this minimal change.

If you don't have bandwidth to tackle that now, I can open a PR in a couple days to clean things up.

from pc-dart.

Maybe I'm out of line here, and asymmetric encryption isn't my specialty, but can we generalize OAEP to work with any hashing algorithm underneath? We do have a Digest class, so if users can pass a digest, we can do something similar to the PBKDF2 construction (or whatever).

from pc-dart.

Hi,

Thanks, I'll see how I go getting to it.

MW

from pc-dart.

@AKushWarrior

I think that we likely have to preserve SHA-1 as the default (in the interest of not breaking all RSA-OAEP code). Providing multiple OAEP classes seems ridiculous (the digest is already declared as a top-level field, we can literally just provide access to that through a parameter).

This can be mitigated with a major version change. Using SHA-1 makes encryption vulnerable - see the comment here - https://developer.mozilla.org/en-US/docs/Web/API/RsaHashedKeyGenParams

We should probably use: "RSA/OAEP/___", where the empty space is the hashing algorithm, for the registry. If a consumer is accessing the class directly, we can provide an optional "digest" parameter in the constructor which defines the hashing algorithm to use.

That might be ok as long as the default is secure. Preserving backwards compatibility in the face of existing security vulnerability is wrong.

I believe that this issue should be treated as a disclosed security vulnerability and OAEP implementation here should not be used until this issue is fixed. It also deserves a notice in readme once the major version change is released that OAEP implementation in previous versions is insecure.

from pc-dart.

I opened a PR (#98) with the mentioned changes. We are actively using it in a production app and encryption is compatible with other implementations.

from pc-dart.

@AKushWarrior it could in theory, but in practice and through different recommendations, SHA256 is most often used as the Hash function as well as in the MGF1 hash generation. SHA1 is considered less secure and I've never seen other hash functions used in place of those.

from pc-dart.

I think that we likely have to preserve SHA-1 as the default (in the interest of not breaking all RSA-OAEP code). Providing multiple OAEP classes seems ridiculous (the digest is already declared as a top-level field, we can literally just provide access to that through a parameter).

We should probably use: "RSA/OAEP/___", where the empty space is the hashing algorithm, for the registry. If a consumer is accessing the class directly, we can provide an optional "digest" parameter in the constructor which defines the hashing algorithm to use.

Though this allows novel constructions (which are discouraged), I think that this library is not strongly opinionated when it comes to "best practices"; we provide the tools and have faith that client programmers know enough not to shoot themselves in the foot.

from pc-dart.

Have you looked at the PR? Maybe it'd be more constructive to discuss there? Your comment seems out of date with the current situation.

from pc-dart.