On Mon, Jan 20, 2020 at 6:45 PM Alex Herbert <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'.
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
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
