Regression in BC v1.59 Base64 Encoder

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Regression in BC v1.59 Base64 Encoder

Bastian Fredriksson

Hi everyone!

It looks like there is a regression in BC v1.59 Base64 Encoder

The following test case passes in BC v1.58 but not in BC v1.59

@Test
public void testDecode() {
        Base64.decode("DAxFSkJDQSBTYW".getBytes());          
}

The corresponding test case using Apache Commons works fine.

@Test
public void testDecode() {
        org.apache.commons.codec.binary.Base64.decodeBase64("DAxFSkJDQSBTYW".getBytes());
}

The error is index out of bounds exception (off by one) when this line of code is executed

outLen += decodeLastBlock(out, (char) data[e0], (char) data[e1], (char) data[e2], (char) data[e3]);

The commit which introduced the regression was commit ea9172f.

https://github.com/bcgit/bc-java/commit/ea9172fe8437daa6223b928bfd85b18a6fa93caf#diff-1b5c6be257e18863c225183c8b6a26b3

I don't think this is a bug in Bouncy Castle since the input uses incorrect padding

> echo "DAxFSkJDQSBTYW" | base64 --decode | hd
base64: invalid input
00000000  0c 0c 45 4a 42 43 41 20  53 61                    |..EJBCA Sa|
0000000a

E.g. DAxFSkJDQSBTYW== works fine in the new BC lib.

The ea9172f commit added support for whitespace at the end of the base64 encoded data, in what I assume to be an attempt to make the decoding more robust. However, as a consequence the incorrectly padded string DAxFSkJDQSBTYW is no longer accepted, which in some sense leads to less robust decoding. Was it intentional to not support incorrectly padded base64 encoded strings such as DAxFSkJDQSBTYW and is this something which will be fixed in future versions of BC?

--

Bastian Fredriksson

+46 733 907 485

https://helix.stormhub.org

Reply | Threaded
Open this post in threaded view
|

Re: Regression in BC v1.59 Base64 Encoder

David Hook-3

Yes, the intention was to try make sure we reject any encoding that is invalid.

Regards,

David

On 08/01/18 23:42, Bastian Fredriksson wrote:

Hi everyone!

It looks like there is a regression in BC v1.59 Base64 Encoder

The following test case passes in BC v1.58 but not in BC v1.59

@Test
public void testDecode() {
        Base64.decode("DAxFSkJDQSBTYW".getBytes());          
}

The corresponding test case using Apache Commons works fine.

@Test
public void testDecode() {
        org.apache.commons.codec.binary.Base64.decodeBase64("DAxFSkJDQSBTYW".getBytes());
}

The error is index out of bounds exception (off by one) when this line of code is executed

outLen += decodeLastBlock(out, (char) data[e0], (char) data[e1], (char) data[e2], (char) data[e3]);

The commit which introduced the regression was commit ea9172f.

https://github.com/bcgit/bc-java/commit/ea9172fe8437daa6223b928bfd85b18a6fa93caf#diff-1b5c6be257e18863c225183c8b6a26b3

I don't think this is a bug in Bouncy Castle since the input uses incorrect padding

> echo "DAxFSkJDQSBTYW" | base64 --decode | hd
base64: invalid input
00000000  0c 0c 45 4a 42 43 41 20  53 61                    |..EJBCA Sa|
0000000a

E.g. DAxFSkJDQSBTYW== works fine in the new BC lib.

The ea9172f commit added support for whitespace at the end of the base64 encoded data, in what I assume to be an attempt to make the decoding more robust. However, as a consequence the incorrectly padded string DAxFSkJDQSBTYW is no longer accepted, which in some sense leads to less robust decoding. Was it intentional to not support incorrectly padded base64 encoded strings such as DAxFSkJDQSBTYW and is this something which will be fixed in future versions of BC?

--

Bastian Fredriksson

+46 733 907 485

https://helix.stormhub.org