Correct padding when extracting r and s components of the DER encoded EC signature

[robotdan]

Issue

• A JWT configured to be signed using the ES512 algorithm may have an invalid signature fusionauth-issues#2661
• Fix padding on the EC signature when decoding r and s from the DER encoded value #57

Description

Correct padding when extracting r and s components of the DER encoded EC signature

There was a bug in how we were extracting the r and s components into a byte array from the DER encoded version.

For the ES512 algorithm, we build a 132 byte array to fill up with the r and s components. The array has to be split evenly so it can be parsed using a 0 and 64 byte offset. But within each of those segments, the r and s component need to be left padded correctly.

The code was previously assume we would at most have a single 0 to pad, but in some cases we would have a 64 byte value for r or s and then our value was left shifted by one byte.

So for example:

[--------------------- 132 bytes --------------------- ]
[----------- r 66 ---------][--------- s 66 -----------]

So in practice because the code assumed the maximum padding of 1 byte we ended up with 012340 instead of 001234 for one or both of these components.

So when the r value is only 64 bytes, we need to pad it correctly so that when we need to DER encode it again and we simply split the 132 byte array down the middle we have the correct components with the correct padding.

Your classic elliptic signature DER decoding mistake. 😎

[spwitt]

The results of this code are not wrong, but they give a false impression of possible states. I think that the dstPos is the only value that needs to be adjusted for the array copy.

[spwitt]

Add a comment for these after they are cleaned up about offsetting the destination position for left padding when r or s is shorter than 66 bytes in the ES512 case.

Do we know whether it only happens with that algorithm? Or just that we have never seen it with another?

[robotdan]

It can occur in any algorithm in theory. In my testing, I could get r and s src and dst positions to be greater than 0 for all three algorithms.

[spwitt]

Okay. The updated code seems good then. I still think there should be a comment about how these changes account for left padding with 0s in both the source and destination byte arrays.

[spwitt]

❤️

[spwitt]

Other things I thought of after I submitted the review.

• Do we have any issues with derEncode()? The length of r and s in encoded value will always be 66 for ES512. It may be valid to encode the padding, but the JDK avoids the extra bytes
• I think the DerInputStream.readLength() implementation may have a couple of minor issues
  • If there are more than 4 bytes in the length, we throw an exception for it being too long. According to spec, there can be up to 254 bytes in the length. (this may be because we have limited the implementation to the DER use case for signatures)
  • We do not account for the case where the first length byte is 255, which is reserved for future expansion (because of the previous point, the code would reject that anyway)
  • Not really an issue, but there are places where a byte b is being b & 0xFF. This is an identity operation for & and is not necessary

[robotdan]

re: these comments - #56 (comment) let's discuss further if you are concerned. I believe the DerInputStream is all correct.

This is all part of the DER spec, but you can review the JDK impl for comparision if you want.
sun.security.util.DerInputStream.getLength

The length portion of the encoding can only be 4 bytes at most, this allows us to represent a value up to 2^32 (4 * 8 bits).

The DER length encoding is kind of complicated, and I would not want to change any of it unless we can come up with a test that proves it is broken. If you do think we have a bug, see if you can recreate it in DerInputStreamTest and we can go from there.

[spwitt]

Looks good to me!