I hope this would be at the level of BaseNCodec, not just Base64.
I am not sure I like BaseNCodec#setAllowTrailingBits(boolean), maybe
someone more general like BaseNCodec.setStrictDecoding(boolean) where the
default is false for backward compatibility.
I do not think we want to go as far as creating an enum for various
enforcement features.
Gary
On Mon, Jan 20, 2020 at 10:25 AM Alex Herbert <alex.d.herb...@gmail.com>
wrote:
> 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
>
>
