PointyCastle / pointycastle

Moved into the Bouncy Castle project: https://github.com/bcgit/pc-dart
MIT License
270 stars 76 forks source link

OAEP unit test #212

Closed hoylen closed 4 years ago

hoylen commented 4 years ago

Here is a new unit test for RSA-OAEP that is correct.

It uses a test vector found in "RSAES-OAEP Encryption Scheme: Algorithm specification and supporting documentation", published by RSA Laboratories.

The FixedSecureRandom (an implementation of a SecureRandom for testing) was enhanced so it can be used to "provide" the seed value from the test vector, so that the RSAES-OAEP encryption operation will always produce the exact ciphertext from the test vector. The _test/key_generators/rsa_key_generatortest.dart was updated to work with the new FixedSecureRandom.

The code for OAEPEncoding was documented so it can be easily followed when comparing it to the specification of RSA-OAEP in RFC 2437. Most of the code was not changed, except for the very end of OAEPENcoding._decodeBlock, which used to copy the decoded message into two temporary arrays, and then never use those arrays, before copying the decoded message into the output buffer. Those two temporary arrays seemed pointless.

stevenroose commented 4 years ago

ACK d364fa96f16e95a9e22d5dce240851ab4a949d75

Thanks, those code comments are amazing!

stevenroose commented 4 years ago

Could you rebase so I can merge?

duncanhoggan commented 4 years ago

@hoylen, very nice contribution. Sorry about the pointless arrays, I tried to follow the bouncy castle implementation as closely as possible.

hoylen commented 4 years ago

Thanks Duncan.

The Bouncy Castle heritage would explain it.

It also explains why OAEPEncoding._decodeBlock contains this strange code (where the condition is always true and the first copy just copies block to itself - fortunately, _arrayCopy is implemented in such a way that such a copy doesn't mess things up):

if (block.length <= block.length) {
   block = _arraycopy(block, 0, block, block.length - block.length, block.length);
} else {
   block = _arraycopy(block, 0, block, 0, block.length);
   wrongData = true;
}

In the Bouncy Castle OAEPEncoding.java it does something different:

if (data.length <= block.length)
{
    System.arraycopy(data, 0, block, block.length - data.length, data.length);
}
 else
 {
     System.arraycopy(data, 0, block, 0, block.length);
     wrongData = true;
}

So shall we delete that "does nothing" code from Pointy Castle? Or is it worth investigating more why they are different? At a quick glance, it seems the Dart code allows block to be shorter than the underlying engine's maximum output block size, but the Java code makes sure the block is always that maximum size.

Is diverging from Bouncy Castle code a problem (e.g. detecting changes/fixes in the Bouncy Castle and porting them across)? Or is once the Dart code has been written, the ties to the Java code are no longer important?

P.S. Unrelated question: are you able to clarify the real difference between using the registry or not? The README says using the registry "can possibly increase the compiled size of your program". But my quick and simple testing with Dart Native and compiling to JavaScript, both produced the same size outputs.

stevenroose commented 4 years ago

It's been a long time since I worked on Dart stuff. But my largest contribution to this lib was the registry. It was leveraging dart:mirrors so that you could use the convenience constructors like Bouncy Castle has.

But at the time I was doing that work, there was some movement in the Dart community around reflections, some alternative way to do introspection. I don't know how that turned out. But since Flutter was not allowing mirrors, I believe at some point I had two reflation-based registries: one with dart:mirrors and one with reflections.

Then I redid the whole thing and now the registry is not based on reflection. When you invoke the registry for the first time, the algorithm index is built. I don't know the latest on tree shaking etc, but this would mean that if you don't use the registry, all that registration code should be shaken out by the tree shaking algorithm. Not sure, though.

stevenroose commented 4 years ago

About PC <> BC, I have no opinion on that. I don't think parity for the sake of parity makes sense. But if they find bugs/optimizations, it can obviously be important or worth it to port them over. But certainly not necessary if they are not crucial.

hoylen commented 4 years ago

Ok, I have removed the redundant code that used to make sense in Bouncy Castle, but no longer does anything in Dart.

stevenroose commented 4 years ago

Great!