terl / lazysodium-java

A Java implementation of the Libsodium crypto library. For the lazy dev.
https://github.com/terl/lazysodium-java/wiki
Mozilla Public License 2.0
134 stars 46 forks source link

Padding methods take wrong data type #108

Closed Traderjoe95 closed 2 years ago

Traderjoe95 commented 3 years ago

The sodium_pad and sodium_unpad methods both take a char[] buf in Lazysodium, which is not a correct representation of the underlying native library. This should be changed to byte[] buf.

gurpreet- commented 3 years ago

Right OK so this seemingly benign function has bested me. At first I tried to pass in byte[] buf as you suggested, but the array does not get resized on the Java side. So then I tried to use a Pointer like so:

String test = "test";
byte[] testArr = Native.toByteArray(test, StandardCharsets.UTF_8);
Pointer arrPtr = new Memory(testArr.length);
arrPtr.write(0, testArr, 0, testArr.length);

if (!lazySodium.sodiumPad(ref, arrPtr, testArr.length, 12, 30)) {
    throw new IllegalArgumentException("Failed to pad");
}

byte[] resultArr = new byte[ref.getValue()]; // ref.getValue contains new size of array
Pointer ptr2 = new Memory(ref.getValue());
ptr2.setPointer(0, arrPtr);
ptr2.read(0, resultArr, 0, ref.getValue());

// resultArr is an array of garbled nonsense instead of "test    "

I'm going slightly crazy here, do you have any insight as to how to pass in a pointer and then retrieve the memory block (that has a new length) pointed to that pointer?

Traderjoe95 commented 3 years ago

You definitely need to pass an input array to libsodium that's large enough to hold the message and the padding. Here's what I would do: Take the input data with length len and the block size b and allocate an array that has length len + b. This array will always be able to hold the padded data, as the padding will never be longer than one full block:

1, When len is not divisible by b, the padding will be len % b bytes long which is less than b

  1. When len is divisible by b, a full padding block will be appended, i.e. the padding length is b

You could even go a little further by using a temporary buffer that already has the correct length ((floor(len / b) + 1) * b or, even simpler, len - len % b + b), which saves one array copy operation.

gurpreet- commented 3 years ago

If we allocate the size of the array that we know is going to be the final size, then padding it would be redundant because Java already allocates 0'd bytes to the end of that array.

As far as I understand, the C function pads the end of the array with 0s. So my question is, what's the point in padding it with 0s if the Java array would already have 0d its bytes at the end of the array?

Therefore that's why I was trying to pass in a Pointer in the first place because on the C side it seems to be doing more magic to the array passed in (and also we don't have to predetermine the size of the array in Java land).

These sodiumPad functions seem ever so redundant in Java land and would be better off if I removed them to avoid confusion.

Traderjoe95 commented 3 years ago

As far as I understand, the C function pads the end of the array with 0s. So my question is, what's the point in padding it with 0s if the Java array would already have 0d its bytes at the end of the array?

The function is applying the ISO/IEC 7816-4 padding which is not all zeroes, but 0x80 0x00 ... 0x00. That's what the barrier_mask variable is for in the C function, I believe. This is still relatively easy to achieve in Java, but at least not by simply allocating a fresh array. I would not remove the function, even though it does not do much, for the sake of completeness.

The sodium_unpad function, however, will also perform checking on the padding which is easy to get wrong, so I would also leave it there.

gurpreet- commented 3 years ago

After a lot of head-scratching, I got this to work and will be available in the next release

gurpreet- commented 2 years ago

Available in release 5.1.0

Traderjoe95 commented 2 years ago

Cool, thanks a lot!