CODEC-134: Update commons-codec to reject decoding any impossible str…

…ing encoding for Base32 and Base64. (#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.
apache · May 20, 2019 · 48b6157 · 48b6157
1 parent 0bf5b8a
commit 48b6157
Show file tree

Hide file tree

Showing 8 changed files with 134 additions and 4 deletions.
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
@@ -45,6 +45,7 @@ The <action> type attribute can be add,update,fix,remove.
 
     <release version="1.13" date="YYYY-MM-DD" description="TBD">
       <action issue="CODEC-257" dev="ggregory" type="update">Update from Java 7 to Java 8</action>      
+      <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>      
     </release>
 
     <release version="1.12" date="2019-02-04" description="Feature and fix release.">

diff --git a/src/main/java/org/apache/commons/codec/binary/Base32.java b/src/main/java/org/apache/commons/codec/binary/Base32.java
@@ -332,7 +332,7 @@ public Base32(final int lineLength, final byte[] lineSeparator, final boolean us
      * @param inPos
      *            Position to start reading data from.
      * @param inAvail
-     *            Amount of bytes available from input for encoding.
+     *            Amount of bytes available from input for decoding.
      * @param context the context to be used
      *
      * Output is written to {@link Context#buffer} as 8-bit octets, using {@link Context#pos} as the buffer position
@@ -381,29 +381,35 @@ void decode(final byte[] in, int inPos, final int inAvail, final Context context
             //  we ignore partial bytes, i.e. only multiples of 8 count
             switch (context.modulus) {
                 case 2 : // 10 bits, drop 2 and output one byte
+                    validateCharacter(2, context);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 2) & MASK_8BITS);
                     break;
                 case 3 : // 15 bits, drop 7 and output 1 byte
+                    validateCharacter(7, context);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 7) & MASK_8BITS);
                     break;
                 case 4 : // 20 bits = 2*8 + 4
+                    validateCharacter(4, context);
                     context.lbitWorkArea = context.lbitWorkArea >> 4; // drop 4 bits
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
                     break;
                 case 5 : // 25bits = 3*8 + 1
+                    validateCharacter(1, context);
                     context.lbitWorkArea = context.lbitWorkArea >> 1;
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
                     break;
                 case 6 : // 30bits = 3*8 + 6
+                    validateCharacter(6, context);
                     context.lbitWorkArea = context.lbitWorkArea >> 6;
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 8) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea) & MASK_8BITS);
                     break;
                 case 7 : // 35 = 4*8 +3
+                    validateCharacter(3, context);
                     context.lbitWorkArea = context.lbitWorkArea >> 3;
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 24) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.lbitWorkArea >> 16) & MASK_8BITS);
@@ -540,4 +546,21 @@ void encode(final byte[] in, int inPos, final int inAvail, final Context context
     public boolean isInAlphabet(final byte octet) {
         return octet >= 0 && octet < decodeTable.length && decodeTable[octet] != -1;
     }
+
+    /**
+     * <p>
+     * Validates whether the character is possible in the context of the set of possible base 32 values.
+     * </p>
+     * 
+     * @param numBits number of least significant bits to check
+     * @param context the context to be used
+     * 
+     * @throws IllegalArgumentException if the bits being checked contain any non-zero value
+     */
+    private void validateCharacter(int numBits, Context context) {
+        if ((context.lbitWorkArea & numBits) != 0) {
+            throw new IllegalArgumentException(
+                "Last encoded character (before the paddings if any) is a valid base 32 alphabet but not a possible value");
+        }
+    }
 }
diff --git a/src/main/java/org/apache/commons/codec/binary/Base64.java b/src/main/java/org/apache/commons/codec/binary/Base64.java
@@ -421,7 +421,7 @@ void encode(final byte[] in, int inPos, final int inAvail, final Context context
      * @param inPos
      *            Position to start reading data from.
      * @param inAvail
-     *            Amount of bytes available from input for encoding.
+     *            Amount of bytes available from input for decoding.
      * @param context
      *            the context to be used
      */
@@ -469,10 +469,12 @@ void decode(final byte[] in, int inPos, final int inAvail, final Context context
                     // TODO not currently tested; perhaps it is impossible?
                     break;
                 case 2 : // 12 bits = 8 + 4
+                    validateCharacter(4, context);
                     context.ibitWorkArea = context.ibitWorkArea >> 4; // dump the extra 4 bits
                     buffer[context.pos++] = (byte) ((context.ibitWorkArea) & MASK_8BITS);
                     break;
                 case 3 : // 18 bits = 8 + 8 + 2
+                    validateCharacter(2, context);
                     context.ibitWorkArea = context.ibitWorkArea >> 2; // dump 2 bits
                     buffer[context.pos++] = (byte) ((context.ibitWorkArea >> 8) & MASK_8BITS);
                     buffer[context.pos++] = (byte) ((context.ibitWorkArea) & MASK_8BITS);
@@ -781,4 +783,21 @@ protected boolean isInAlphabet(final byte octet) {
         return octet >= 0 && octet < decodeTable.length && decodeTable[octet] != -1;
     }
 
+    /**
+     * <p>
+     * Validates whether the character is possible in the context of the set of possible base 64 values.
+     * </p>
+     * 
+     * @param numBits number of least significant bits to check
+     * @param context the context to be used
+     * 
+     * @throws IllegalArgumentException if the bits being checked contain any non-zero value
+     */
+    private long validateCharacter(int numBitsToDrop, Context context) {
+        if ((context.ibitWorkArea & numBitsToDrop) != 0) {
+        throw new IllegalArgumentException(
+            "Last encoded character (before the paddings if any) is a valid base 64 alphabet but not a possible value");
+        }
+        return context.ibitWorkArea >> numBitsToDrop;
+    }
 }
diff --git a/src/main/java/org/apache/commons/codec/net/BCodec.java b/src/main/java/org/apache/commons/codec/net/BCodec.java
@@ -178,7 +178,7 @@ public String decode(final String value) throws DecoderException {
         }
         try {
             return this.decodeText(value);
-        } catch (final UnsupportedEncodingException e) {
+        } catch (final UnsupportedEncodingException | IllegalArgumentException e) {
             throw new DecoderException(e.getMessage(), e);
         }
     }

diff --git a/src/test/java/org/apache/commons/codec/binary/Base32Test.java b/src/test/java/org/apache/commons/codec/binary/Base32Test.java
@@ -46,6 +46,30 @@ public class Base32Test {
         {"foobar" ,"MZXW6YTBOI======"},
     };
 
+    private static final String[] BASE32_IMPOSSIBLE_CASES = {
+        "MC======",
+        "MZXE====",
+        "MZXWB===",
+        "MZXW6YB=",
+        "MZXW6YTBOC======"
+        };
+
+    private static final String[] BASE32_IMPOSSIBLE_CASES_CHUNKED = {
+        "M2======\r\n",
+        "MZX0====\r\n",
+        "MZXW0===\r\n",
+        "MZXW6Y2=\r\n",
+        "MZXW6YTBO2======\r\n"
+    };
+
+    private static final String[] BASE32HEX_IMPOSSIBLE_CASES = {
+        "C2======",
+        "CPN4====",
+        "CPNM1===",
+        "CPNMUO1=",
+        "CPNMUOJ1E2======"
+    };
+
     private static final Object[][] BASE32_BINARY_TEST_CASES;
 
     //            { null, "O0o0O0o0" }
@@ -248,4 +272,29 @@ public void testSingleCharEncoding() {
         }
     }
 
+    @Test
+    public void testBase32ImpossibleSamples() {
+        testImpossibleCases(new Base32(), BASE32_IMPOSSIBLE_CASES);
+    }
+
+    @Test
+    public void testBase32ImpossibleChunked() {
+        testImpossibleCases(new Base32(20), BASE32_IMPOSSIBLE_CASES_CHUNKED);
+    }
+
+    @Test
+    public void testBase32HexImpossibleSamples() {
+        testImpossibleCases(new Base32(true), BASE32HEX_IMPOSSIBLE_CASES);
+    }
+
+    private void testImpossibleCases(Base32 codec, String[] impossible_cases) {
+        for (String impossible : impossible_cases) {
+            try {
+                codec.decode(impossible);
+                fail();
+            } catch (IllegalArgumentException ex) {
+                // expected
+            }
+        }
+    }
 }
diff --git a/src/test/java/org/apache/commons/codec/binary/Base64Test.java b/src/test/java/org/apache/commons/codec/binary/Base64Test.java
@@ -44,6 +44,13 @@ public class Base64Test {
 
     private static final Charset CHARSET_UTF8 = Charsets.UTF_8;
 
+    private static final String[] BASE64_IMPOSSIBLE_CASES = {
+        "ZE==",
+        "ZmC=",
+        "Zm9vYE==",
+        "Zm9vYmC=",
+    };
+
     private final Random random = new Random();
 
     /**
@@ -1297,4 +1304,16 @@ public void testHugeLineSeparator() {
         assertEquals("testDEFAULT_BUFFER_SIZE", strOriginal, strDecoded);
     }
 
+    @Test
+    public void testBase64ImpossibleSamples() {
+        Base64 codec = new Base64();
+        for (String s : BASE64_IMPOSSIBLE_CASES) {
+            try {
+                codec.decode(s);
+                fail();
+            } catch (IllegalArgumentException ex) {
+                // expected
+            }
+        }
+    }
 }
diff --git a/src/test/java/org/apache/commons/codec/binary/Base64TestData.java b/src/test/java/org/apache/commons/codec/binary/Base64TestData.java
@@ -30,7 +30,7 @@
  */
 public class Base64TestData {
 
-    public static final String CODEC_101_MULTIPLE_OF_3 = "123";
+    public static final String CODEC_101_MULTIPLE_OF_3 = "124";
 
     public static final String CODEC_98_NPE
         = "YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXpBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWjAxMjM";

diff --git a/src/test/java/org/apache/commons/codec/net/BCodecTest.java b/src/test/java/org/apache/commons/codec/net/BCodecTest.java
@@ -33,6 +33,12 @@
  *
  */
 public class BCodecTest {
+    private static final String[] BASE64_IMPOSSIBLE_CASES = {
+            "ZE==",
+            "ZmC=",
+            "Zm9vYE==",
+            "Zm9vYmC=",
+    };
 
     static final int SWISS_GERMAN_STUFF_UNICODE[] =
         { 0x47, 0x72, 0xFC, 0x65, 0x7A, 0x69, 0x5F, 0x7A, 0xE4, 0x6D, 0xE4 };
@@ -147,4 +153,17 @@ public void testDecodeObjects() throws Exception {
             // Exception expected, test segment passes.
         }
     }
+
+    @Test
+    public void testBase64ImpossibleSamples() {
+        BCodec codec = new BCodec();
+        for (String s : BASE64_IMPOSSIBLE_CASES) {
+            try {
+                codec.decode(s);
+                fail();
+            } catch (DecoderException ex) {
+                // expected
+            }
+        }
+    }
 }