In release 1.14 I fixed a patch made in 1.13 to reject decoding bytes that cannot be re-encoded to the same bytes (Codec-134 [1]). My fix was to correct the use of a mask to check the trailing bytes.

The implementation checks trailing bits that are to be discarded are all zero.

However the check does not go so far as to throw an exception when there are trailing bits below the count of 8. In this case there cannot be any more bytes and the bits are discarded.

I assume this is because before the trailing bit validation it was not necessary to do anything when the number of trailing bits was less than a single byte.

However this means it is still possible to decode some bytes and then encode them to create different bytes as shown here:

@Test
public void testTrailing6bits() {
    final Base64 codec = new Base64();
    // A 4 byte block plus an extra one
    byte[] bytes = new byte[] { 'A', 'B', 'C', 'D' };
    byte[] decoded = codec.decode(bytes);
    byte[] encoded = codec.encode(decoded);
    Assert.assertTrue("The round trip is possible", Arrays.equals(bytes, encoded));
    // A 4 byte block plus an extra one
    bytes = new byte[] { 'A', 'B', 'C', 'D', 'E' };
    decoded = codec.decode(bytes);
    encoded = codec.encode(decoded);
    Assert.assertFalse("The round trip is not possible", Arrays.equals(bytes, encoded));
}

@Test
public void testTrailing5bits() {
    final Base32 codec = new Base32();
    // An 8 byte block plus an extra one
    byte[] bytes = new byte[] { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H' };
    byte[] decoded = codec.decode(bytes);
    byte[] encoded = codec.encode(decoded);
    Assert.assertTrue("The round trip is possible", Arrays.equals(bytes, encoded));
    // An 8 byte block plus an extra one
    bytes = new byte[] { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I' };
    decoded = codec.decode(bytes);
    encoded = codec.encode(decoded);
    Assert.assertFalse("The round trip is not possible", Arrays.equals(bytes, encoded));
}

Previously when fixing the trailing bit validation I suggested validating all trailing bits, even when they cannot be converted into any bytes. However this was not done and there are still some invalid inputs that do not trigger an exception.

I propose to update the fix by throwing an IllegalArgumentException if there are left-over bits that cannot be decoded. That seems to be the intention of CODEC-134 to prevent security exploitation.

However note that stricter validation is causing users to complain about exceptions when they should be questioning their own data. Recently a user raised Codec-279 which stated that exceptions were being thrown [2]. I believe the code is functioning as expected. So adding extra validation may prove to be more of a headache to users who have somehow obtained invalid encodings.

One workaround is to fix the implementation to reject anything that cannot be re-encoded to the same bytes. Then add a property that allows this behaviour to be suppressed allowing for example:

Base64 codec = new Base64();
byte[] bytes = new byte[] { 'A', 'B', 'C', 'D', 'E' };
byte[] decoded;
try {
    decoded = codec.decode(bytes);
} catch (IllegalArgumentException ignore) {
    codec.setAllowTrailingBits(true);
    // OK
    decoded = codec.decode(bytes);
}

WDYT?

1. Fully fix CODEC-134
2. Optionally allow CODEC-134 behaviour to be suppressed

I would vote for option 1. It requires 1 line change in the code for Base32 and Base64.

I am open to opinions for option 2. It would allow users to upgrade and then opt-in to previous functionality.

Alex


[1] https://issues.apache.org/jira/browse/CODEC-134

[2] https://issues.apache.org/jira/browse/CODEC-279


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Reply via email to