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
