> On 20 Jan 2020, at 23:20, Gary Gregory <garydgreg...@gmail.com> wrote:
>
> 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 prefer that name.
>
> I do not think we want to go as far as creating an enum for various
> enforcement features.
At the moment the implementation is half way between strict and relaxed.
Setting it back to relaxed by default removes changes attempted in CODEC-134.
This perhaps needs some input from the originator of CODEC-134 for their use
case. If it is for an uncommon edge case with a secure context then having a
non-strict default seems more sensible. The default just throws away stuff that
it cannot decode at the end. In most cases this is fine. User reports on this
contain statements along the lines of "other libraries can do the decoding, why
does codec error?"
So set this to strict=false by default and then allow a strict=true via a
BaseNCodec property.
I don’t think we want to add more static overrides with this flag so the
default for the static methods would go back to non-strict.
If no objections I’ll raise a Jira ticket and PR to implement this.
>
> 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
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
