Skip to content

Commit 814f805

Browse files
tmousaw-ptcjgallimore
tmousaw-ptc
authored and
jgallimore
committed
CODEC-134: Update commons-codec to reject decoding any impossible string encoding for Base32 and Base64. (apache#19)
[CODEC-134] Squash and merge. * CODEC-134: Update to reject decoding strings that are not possible in the given encoding for Base32 and Base64. * CODEC-134: Fix tabs instead of spaces. * CODEC-134: Update such that the right shift is not indirectly accomplished via a method call. * CODEC-134: Fix spaces versus tabs (again). * CODEC-134: Add test to cover missed exception case in BCodec.java. * CODEC-134: Update changes.xml to describe change.
1 parent e9da3d1 commit 814f805

8 files changed

Lines changed: 138 additions & 3 deletions

File tree

‎src/changes/changes.xml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ The <action> type attribute can be add,update,fix,remove.
4242
<author>Gary Gregory</author>
4343
</properties>
4444
<body>
45+
<release version="1.10-TT.1" date="20 December 2013" description="Feature and fix release.">
46+
<action issue="CODEC-134" dev="tmousaw-ptc" type="fix">Reject any decode request for a value that is impossible to encode to for Base32/Base64 rather than blindly decoding.</action>
47+
</release>
4548
<release version="1.10" date="5 November 2014" description="Feature and fix release.">
4649
<action dev="ggregory" type="add" issue="CODEC-192" due-to="Thomas Neidhart">Add Daitch-Mokotoff Soundex</action>
4750
<action dev="ggregory" type="add" issue="CODEC-121" due-to="Thomas Neidhart, Java John">QuotedPrintableCodec does not support soft line break per the 'quoted-printable' example on Wikipedia</action>

‎src/main/java/org/apache/commons/codec/binary/Base32.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ public Base32(final int lineLength, final byte[] lineSeparator, final boolean us
327327
* @param inPos
328328
* Position to start reading data from.
329329
* @param inAvail
330-
* Amount of bytes available from input for encoding.
330+
* Amount of bytes available from input for decoding.
331331
* @param context the context to be used
332332
*
333333
* Output is written to {@link Context#buffer} as 8-bit octets, using {@link Context#pos} as the buffer position
@@ -377,29 +377,35 @@ void decode(final byte[] in, int inPos, final int inAvail, final Context context
377377
// we ignore partial bytes, i.e. only multiples of 8 count
378378
switch (context.modulus) {
379379
case 2 : // 10 bits, drop 2 and output one byte
380+
validateCharacter(2, context);
380381
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 2) & MASK_8BITS);
381382
break;
382383
case 3 : // 15 bits, drop 7 and output 1 byte
384+
validateCharacter(7, context);
383385
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 7) & MASK_8BITS);
384386
break;
385387
case 4 : // 20 bits = 2*8 + 4
388+
validateCharacter(4, context);
386389
context.lbitWorkArea = context.lbitWorkArea >> 4; // drop 4 bits
387390
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
388391
buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
389392
break;
390393
case 5 : // 25bits = 3*8 + 1
394+
validateCharacter(1, context);
391395
context.lbitWorkArea = context.lbitWorkArea >> 1;
392396
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
393397
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
394398
buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
395399
break;
396400
case 6 : // 30bits = 3*8 + 6
401+
validateCharacter(6, context);
397402
context.lbitWorkArea = context.lbitWorkArea >> 6;
398403
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
399404
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
400405
buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
401406
break;
402407
case 7 : // 35 = 4*8 +3
408+
validateCharacter(3, context);
403409
context.lbitWorkArea = context.lbitWorkArea >> 3;
404410
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 24) & MASK_8BITS);
405411
buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
@@ -536,4 +542,21 @@ void encode(final byte[] in, int inPos, final int inAvail, final Context context
536542
public boolean isInAlphabet(final byte octet) {
537543
return octet >= 0 && octet < decodeTable.length && decodeTable[octet] != -1;
538544
}
545+
546+
/**
547+
* <p>
548+
* Validates whether the character is possible in the context of the set of possible base 32 values.
549+
* </p>
550+
*
551+
* @param numBits number of least significant bits to check
552+
* @param context the context to be used
553+
*
554+
* @throws IllegalArgumentException if the bits being checked contain any non-zero value
555+
*/
556+
private void validateCharacter(int numBits, Context context) {
557+
if ((context.lbitWorkArea & numBits) != 0) {
558+
throw new IllegalArgumentException(
559+
"Last encoded character (before the paddings if any) is a valid base 32 alphabet but not a possible value");
560+
}
561+
}
539562
}

‎src/main/java/org/apache/commons/codec/binary/Base64.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,7 @@ void encode(final byte[] in, int inPos, final int inAvail, final Context context
420420
* @param inPos
421421
* Position to start reading data from.
422422
* @param inAvail
423-
* Amount of bytes available from input for encoding.
423+
* Amount of bytes available from input for decoding.
424424
* @param context
425425
* the context to be used
426426
*/
@@ -469,10 +469,12 @@ void decode(final byte[] in, int inPos, final int inAvail, final Context context
469469
// TODO not currently tested; perhaps it is impossible?
470470
break;
471471
case 2 : // 12 bits = 8 + 4
472+
validateCharacter(4, context);
472473
context.ibitWorkArea = context.ibitWorkArea >> 4; // dump the extra 4 bits
473474
buffer[context.pos++] = (byte) ((context.ibitWorkArea) & MASK_8BITS);
474475
break;
475476
case 3 : // 18 bits = 8 + 8 + 2
477+
validateCharacter(2, context);
476478
context.ibitWorkArea = context.ibitWorkArea >> 2; // dump 2 bits
477479
buffer[context.pos++] = (byte) ((context.ibitWorkArea >> 8) & MASK_8BITS);
478480
buffer[context.pos++] = (byte) ((context.ibitWorkArea) & MASK_8BITS);
@@ -781,4 +783,21 @@ protected boolean isInAlphabet(final byte octet) {
781783
return octet >= 0 && octet < decodeTable.length && decodeTable[octet] != -1;
782784
}
783785

786+
/**
787+
* <p>
788+
* Validates whether the character is possible in the context of the set of possible base 64 values.
789+
* </p>
790+
*
791+
* @param numBits number of least significant bits to check
792+
* @param context the context to be used
793+
*
794+
* @throws IllegalArgumentException if the bits being checked contain any non-zero value
795+
*/
796+
private long validateCharacter(int numBitsToDrop, Context context) {
797+
if ((context.ibitWorkArea & numBitsToDrop) != 0) {
798+
throw new IllegalArgumentException(
799+
"Last encoded character (before the paddings if any) is a valid base 64 alphabet but not a possible value");
800+
}
801+
return context.ibitWorkArea >> numBitsToDrop;
802+
}
784803
}

‎src/main/java/org/apache/commons/codec/net/BCodec.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,8 @@ public String decode(final String value) throws DecoderException {
181181
return this.decodeText(value);
182182
} catch (final UnsupportedEncodingException e) {
183183
throw new DecoderException(e.getMessage(), e);
184+
} catch (final IllegalArgumentException e) {
185+
throw new DecoderException(e.getMessage(), e);
184186
}
185187
}
186188

‎src/test/java/org/apache/commons/codec/binary/Base32Test.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,30 @@ public class Base32Test {
3838
{"foobar" ,"MZXW6YTBOI======"},
3939
};
4040

41+
private static final String[] BASE32_IMPOSSIBLE_CASES = {
42+
"MC======",
43+
"MZXE====",
44+
"MZXWB===",
45+
"MZXW6YB=",
46+
"MZXW6YTBOC======"
47+
};
48+
49+
private static final String[] BASE32_IMPOSSIBLE_CASES_CHUNKED = {
50+
"M2======\r\n",
51+
"MZX0====\r\n",
52+
"MZXW0===\r\n",
53+
"MZXW6Y2=\r\n",
54+
"MZXW6YTBO2======\r\n"
55+
};
56+
57+
private static final String[] BASE32HEX_IMPOSSIBLE_CASES = {
58+
"C2======",
59+
"CPN4====",
60+
"CPNM1===",
61+
"CPNMUO1=",
62+
"CPNMUOJ1E2======"
63+
};
64+
4165
private static final String [][] BASE32HEX_TEST_CASES = { // RFC 4648
4266
{"" ,""},
4367
{"f" ,"CO======"},
@@ -151,4 +175,30 @@ public void testBase32SamplesNonDefaultPadding() throws Exception {
151175
assertEquals(element[1], codec.encodeAsString(element[0].getBytes(Charsets.UTF_8)));
152176
}
153177
}
178+
179+
@Test
180+
public void testBase32ImpossibleSamples() {
181+
testImpossibleCases(new Base32(), BASE32_IMPOSSIBLE_CASES);
182+
}
183+
184+
@Test
185+
public void testBase32ImpossibleChunked() {
186+
testImpossibleCases(new Base32(20), BASE32_IMPOSSIBLE_CASES_CHUNKED);
187+
}
188+
189+
@Test
190+
public void testBase32HexImpossibleSamples() {
191+
testImpossibleCases(new Base32(true), BASE32HEX_IMPOSSIBLE_CASES);
192+
}
193+
194+
private void testImpossibleCases(Base32 codec, String[] impossible_cases) {
195+
for (String impossible : impossible_cases) {
196+
try {
197+
codec.decode(impossible);
198+
fail();
199+
} catch (IllegalArgumentException ex) {
200+
// expected
201+
}
202+
}
203+
}
154204
}

‎src/test/java/org/apache/commons/codec/binary/Base64Test.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,13 @@
4141
*/
4242
public class Base64Test {
4343

44+
private static final String[] BASE64_IMPOSSIBLE_CASES = {
45+
"ZE==",
46+
"ZmC=",
47+
"Zm9vYE==",
48+
"Zm9vYmC=",
49+
};
50+
4451
private final Random random = new Random();
4552

4653
/**
@@ -1255,4 +1262,16 @@ public void testHugeLineSeparator() {
12551262
assertEquals("testDEFAULT_BUFFER_SIZE", strOriginal, strDecoded);
12561263
}
12571264

1265+
@Test
1266+
public void testBase64ImpossibleSamples() {
1267+
Base64 codec = new Base64();
1268+
for (String s : BASE64_IMPOSSIBLE_CASES) {
1269+
try {
1270+
codec.decode(s);
1271+
fail();
1272+
} catch (IllegalArgumentException ex) {
1273+
// expected
1274+
}
1275+
}
1276+
}
12581277
}

‎src/test/java/org/apache/commons/codec/binary/Base64TestData.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
*/
3232
public class Base64TestData {
3333

34-
public static final String CODEC_101_MULTIPLE_OF_3 = "123";
34+
public static final String CODEC_101_MULTIPLE_OF_3 = "124";
3535

3636
public static final String CODEC_98_NPE
3737
= "YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXpBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWjAxMjM";

‎src/test/java/org/apache/commons/codec/net/BCodecTest.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@
3434
* @version $Id$
3535
*/
3636
public class BCodecTest {
37+
private static final String[] BASE64_IMPOSSIBLE_CASES = {
38+
"ZE==",
39+
"ZmC=",
40+
"Zm9vYE==",
41+
"Zm9vYmC=",
42+
};
3743

3844
static final int SWISS_GERMAN_STUFF_UNICODE[] =
3945
{ 0x47, 0x72, 0xFC, 0x65, 0x7A, 0x69, 0x5F, 0x7A, 0xE4, 0x6D, 0xE4 };
@@ -148,4 +154,17 @@ public void testDecodeObjects() throws Exception {
148154
// Exception expected, test segment passes.
149155
}
150156
}
157+
158+
@Test
159+
public void testBase64ImpossibleSamples() {
160+
BCodec codec = new BCodec();
161+
for (String s : BASE64_IMPOSSIBLE_CASES) {
162+
try {
163+
codec.decode(s);
164+
fail();
165+
} catch (DecoderException ex) {
166+
// expected
167+
}
168+
}
169+
}
151170
}

0 commit comments

Comments
 (0)