Don't use padding for streaming cipher modes (#155)

OFB, CFB[8], CTR, and GCM cipher modes don't require padding, since they act in a streaming manner, working byte-by-byte. Adding padding to them makes the output incompatible with MRI, and unable to be decrypted with it (and OpenSSL, underneath it). GCM is added to NO_PADDING_BLOCK_MODES despite not being in KNOWN_BLOCK_MODES to keep backward compatibility in getPaddingType. I'm happy removing it if others agree, since there shouldn't be any way for it to be supported currently. Fixes #13
jruby · Mar 6, 2018 · f76ec2f · f76ec2f
1 parent c52cf16
commit f76ec2f
Showing 2 changed files with 19 additions and 6 deletions.
diff --git a/src/main/java/org/jruby/ext/openssl/Cipher.java b/src/main/java/org/jruby/ext/openssl/Cipher.java
@@ -261,6 +261,10 @@ public static final class Algorithm {
             KNOWN_BLOCK_MODES.add("NONE"); // valid to pass into JCE
         }
 
+        // Subset of KNOWN_BLOCK_MODES that do not require padding (and shouldn't have it by default).
+        private static final List<String> NO_PADDING_BLOCK_MODES = Arrays.asList(
+            "CFB", "CFB8", "OFB", "CTR", "GCM");
+
         // Ruby to Java name String (or FALSE)
         static final HashMap<String, String[]> supportedCiphers = new LinkedHashMap<String, String[]>(120, 1);
         // we're _marking_ unsupported keys with a Boolean.FALSE mapping
@@ -557,12 +561,10 @@ String getPadding() {
         }
 
         private static String getPaddingType(final String padding, final String cryptoMode) {
-            //if ( "ECB".equals(cryptoMode) ) return "NoPadding";
-            // TODO check cryptoMode CFB/OFB
             final String defaultPadding = "PKCS5Padding";
 
             if ( padding == null ) {
-                if ( "GCM".equalsIgnoreCase(cryptoMode) ) {
+                if ( NO_PADDING_BLOCK_MODES.contains(cryptoMode) ) {
                     return "NoPadding";
                 }
                 return defaultPadding;

diff --git a/src/test/java/org/jruby/ext/openssl/CipherTest.java b/src/test/java/org/jruby/ext/openssl/CipherTest.java
@@ -1,4 +1,3 @@
-
 package org.jruby.ext.openssl;
 
 import org.junit.*;
@@ -97,13 +96,13 @@ private void doTestOsslToJsse() {
         assertEquals("DES", alg.base);
         assertEquals("EDE3", alg.version);
         assertEquals("CFB", alg.mode);
-        assertEquals("DESede/CFB/PKCS5Padding", alg.getRealName());
+        assertEquals("DESede/CFB/NoPadding", alg.getRealName());
 
         alg = Cipher.Algorithm.osslToJava("DES-CFB");
         assertEquals("DES", alg.base);
         assertEquals(null, alg.version);
         assertEquals("CFB", alg.mode);
-        assertEquals("DES/CFB/PKCS5Padding", alg.getRealName());
+        assertEquals("DES/CFB/NoPadding", alg.getRealName());
 
         alg = Cipher.Algorithm.osslToJava("DES3");
         assertEquals("DES", alg.base);
@@ -129,6 +128,18 @@ private void doTestOsslToJsse() {
         assertEquals("PKCS5Padding", alg.getPadding());
         assertEquals("AES/CBC/PKCS5Padding", alg.getRealName());
 
+        alg = Cipher.Algorithm.osslToJava("AES-256-OFB");
+        assertEquals("AES", alg.base);
+        assertEquals("256", alg.version);
+        assertEquals("OFB", alg.mode);
+        assertEquals("AES/OFB/NoPadding", alg.getRealName());
+
+        alg = Cipher.Algorithm.osslToJava("AES-256-CTR");
+        assertEquals("AES", alg.base);
+        assertEquals("256", alg.version);
+        assertEquals("CTR", alg.mode);
+        assertEquals("AES/CTR/NoPadding", alg.getRealName());
+
         alg = Cipher.Algorithm.osslToJava("AES-256-CBC-HMAC-SHA1");
         assertEquals("AES", alg.base);
         assertEquals("CBC", alg.mode);