> On 20 Jan 2020, at 23:54, Gary Gregory <garydgreg...@gmail.com> wrote:
>
> On Mon, Jan 20, 2020 at 6:45 PM Alex Herbert <alex.d.herb...@gmail.com
> <mailto:alex.d.herb...@gmail.com>>
> wrote:
>
>>
>>
>>> 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.
>>
>
> Sure, and let's use the term 'lenient' instead of 'non-strict’.
OK. I’ll work on this in the next few days.
>
> Gary
>
>
>>>
>>> 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
>> <mailto:dev-unsubscr...@commons.apache.org>
>> For additional commands, e-mail: dev-h...@commons.apache.org
>> <mailto:dev-h...@commons.apache.org>
