Replace assertions with explicit runtime checks.

Numerous 'assert' statements were replaced with explicit runtime
checks where warranted. Required some changes to the JUnit tests since
different exceptions now resulted.

Author: kwwall

src/main/java/org/owasp/esapi/StringUtilities.java

@@ -174,6 +174,7 @@ public static int getLevenshteinDistance (String s, String t) {
      * 		assert StringUtils.notNullOrEmpty(cipherXform, true) :
      * 								"Cipher transformation may not be null or empty!";
      * </pre>
+     * or an equivalent runtime check that throws an {@code IllegalArgumentException}.
      *
      * @param str	The {@code String} to be checked.
      * @param trim	If {@code true}, the string is first trimmed before checking
@@ -196,4 +197,4 @@ public static boolean notNullOrEmpty(String str, boolean trim) {
     public static boolean isEmpty(String str) {
         return str == null || str.length() == 0;
     }
-}
+}

src/main/java/org/owasp/esapi/crypto/CipherSpec.java

@@ -153,6 +153,9 @@ private CipherSpec setCipherTransformation(String cipherXform, boolean fromCiphe
 			throw new IllegalArgumentException("Cipher transformation may not be null or empty string (after trimming whitespace).");
 		}
 		int parts = cipherXform.split("/").length;
+
+            // Assertion should be okay here as these conditions are checked
+            // elsewhere and this method is private.
 		assert ( !fromCipher ? (parts == 3) : true ) :
 			"Malformed cipherXform (" + cipherXform + "); must have form: \"alg/mode/paddingscheme\"";
 		if ( fromCipher && (parts != 3)  ) {
@@ -165,8 +168,8 @@ private CipherSpec setCipherTransformation(String cipherXform, boolean fromCiphe
 				// Only algorithm and mode was given.
 				cipherXform += "/NoPadding";
 			} else if ( parts == 3 ) {
-				// All three parts provided. Do nothing. Could happen if not compiled with
-				// assertions enabled.
+                        // All three parts provided. Do nothing. Could happen if not compiled with
+                        // assertions enabled, but there are explicit checks elsewhere.
 				;	// Do nothing - shown only for completeness.
 			} else {
 				// Should never happen unless Cipher implementation is totally screwed up.
@@ -177,6 +180,8 @@ private CipherSpec setCipherTransformation(String cipherXform, boolean fromCiphe
 			throw new IllegalArgumentException("Malformed cipherXform (" + cipherXform +
 											   "); must have form: \"alg/mode/paddingscheme\"");
 		}
+            // Assertion should also be okay here as these conditions are checked
+            // elsewhere and this method is private.
 		assert cipherXform.split("/").length == 3 : "Implementation error setCipherTransformation()";
 		this.cipher_xform_ = cipherXform;
 		return this;
@@ -404,6 +409,8 @@ protected boolean canEqual(Object other) {
 	private String getFromCipherXform(CipherTransformationComponent component) {
         int part = component.ordinal();
 		String[] parts = getCipherTransformation().split("/");
+            // Assertion should also be okay here as these conditions are checked
+            // elsewhere and this method is private.
 		assert parts.length == 3 : "Invalid cipher transformation: " + getCipherTransformation();	
 		return parts[part];
 	}

src/main/java/org/owasp/esapi/crypto/CryptoHelper.java

@@ -138,14 +138,23 @@ public static SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySiz
 	{
         // These really should be turned into actual runtime checks and an
         // IllegalArgumentException should be thrown if they are violated.
-		assert keyDerivationKey != null : "Key derivation key cannot be null.";
+		if ( keyDerivationKey == null ) {
+            throw new IllegalArgumentException("Key derivation key cannot be null.");
+        }
 			// We would choose a larger minimum key size, but we want to be
 			// able to accept DES for legacy encryption needs.
-		assert keySize >= 56 : "Key has size of " + keySize + ", which is less than minimum of 56-bits.";
-		assert (keySize % 8) == 0 : "Key size (" + keySize + ") must be a even multiple of 8-bits.";
-		assert purpose != null;
-		assert purpose.equals("encryption") || purpose.equals("authenticity") :
-			"Purpose must be \"encryption\" or \"authenticity\".";
+		if ( keySize < 56 ) {
+            throw new IllegalArgumentException("Key has size of " + keySize + ", which is less than minimum of 56-bits.");
+        }
+		if ( (keySize % 8) != 0 ) {
+            throw new IllegalArgumentException("Key size (" + keySize + ") must be a even multiple of 8-bits.");
+        }
+		if ( purpose == null ) {
+            throw new IllegalArgumentException("'purpose' may not be null.");
+        }
+		if ( ! ( purpose.equals("encryption") || purpose.equals("authenticity") ) ) {
+			throw new IllegalArgumentException("Purpose must be \"encryption\" or \"authenticity\".");
+        }
 
 		// DISCUSS: Should we use HmacSHA1 (what we were using) or the HMAC defined by
 		//			Encryptor.KDF.PRF instead? Either way, this is not compatible with
@@ -169,8 +178,12 @@ public static SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySiz
 	 */
 	public static boolean isCombinedCipherMode(String cipherMode)
 	{
-	    assert cipherMode != null : "Cipher mode may not be null";
-	    assert ! cipherMode.equals("") : "Cipher mode may not be empty string";
+	    if ( cipherMode == null ) {
+            throw new IllegalArgumentException("Cipher mode may not be null");
+        }
+	    if ( cipherMode.equals("") ) {
+            throw new IllegalArgumentException("Cipher mode may not be empty string");
+        }
 	    List<String> combinedCipherModes =
 	        ESAPI.securityConfiguration().getCombinedCipherModes();
 	    return combinedCipherModes.contains( cipherMode );

src/main/java/org/owasp/esapi/crypto/KeyDerivationFunction.java

@@ -106,12 +106,14 @@ public enum PRF_ALGORITHMS {
     // Check if versions of KeyDerivationFunction, CipherText, and
     // CipherTextSerializer are all the same.
     {
-    	// Ignore error about comparing identical versions and dead code.
-    	// We expect them to be, but the point is to catch us if they aren't.
-    	assert CipherTextSerializer.cipherTextSerializerVersion == CipherText.cipherTextVersion :
-            "Versions of CipherTextSerializer and CipherText are not compatible.";
-    	assert CipherTextSerializer.cipherTextSerializerVersion == KeyDerivationFunction.kdfVersion :
-    		"Versions of CipherTextSerializer and KeyDerivationFunction are not compatible.";
+        // Ignore error about comparing identical versions and dead code.
+        // We expect them to be, but the point is to catch us if they aren't.
+        if ( CipherTextSerializer.cipherTextSerializerVersion != CipherText.cipherTextVersion ) {
+            throw new ExceptionInInitializerError("Versions of CipherTextSerializer and CipherText are not compatible.");
+        }
+        if ( CipherTextSerializer.cipherTextSerializerVersion != KeyDerivationFunction.kdfVersion ) {
+            throw new ExceptionInInitializerError("Versions of CipherTextSerializer and KeyDerivationFunction are not compatible.");
+        }
     }
 
 	/**
@@ -295,14 +297,30 @@ public SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySize, Stri
 		//					to section 5.1 of NIST SP 800-108 based on feedback from
 		//					Jeffrey Walton.
 		//
-        // These probably should be turned into actual runtime checks and an
-        // IllegalArgumentException should be thrown if they are violated.
-		assert keyDerivationKey != null : "Key derivation key cannot be null.";
-			// We would choose a larger minimum key size, but we want to be
-			// able to accept DES for legacy encryption needs.
-		assert keySize >= 56 : "Key has size of " + keySize + ", which is less than minimum of 56-bits.";
-		assert (keySize % 8) == 0 : "Key size (" + keySize + ") must be a even multiple of 8-bits.";
-		assert purpose != null && !purpose.equals("") : "Purpose may not be null or empty.";
+
+		// These checks used to be assertions prior to ESAPI 2.1.0.1
+		if ( keyDerivationKey == null ) {
+			throw new IllegalArgumentException("Key derivation key cannot be null.");
+		}
+			// We would choose a larger minimum key size, but we want to allow
+            // this KDF to be able to accept DES for legacy encryption needs. (Note that
+            // elsewhere there are checks that disallow *encryption* for key size
+            // less than Encryptor.EncryptionKeyLength bits, so if they want
+            // ESAPI to encrypt stuff for DES, they would have to set that up to
+            // be 56 bits. But I can't think of any valid symmetric encryption
+            // algorithm whose key size is less than 56 bits that we would ever
+            // want to allow.
+		if ( keySize < 56 ) {
+			throw new IllegalArgumentException("Key has size of " + keySize +
+											   ", which is less than minimum of 56-bits.");
+		}
+		if ( (keySize % 8) != 0 ) {
+			throw new IllegalArgumentException("Key size (" + keySize +
+											   ") must be a even multiple of 8-bits.");
+		}
+		if ( purpose == null || "".equals(purpose) ) {
+			throw new IllegalArgumentException("Purpose may not be null or empty.");
+		}
 
 		keySize = calcKeySize( keySize );	// Safely convert to whole # of bytes.
 		byte[] derivedKey = new byte[ keySize ];
@@ -451,7 +469,9 @@ public static PRF_ALGORITHMS convertIntToPRF(int selection) {
      *              {@code ks} bits.
      */
     private static int calcKeySize(int ks) {
-        assert ks > 0 : "Key size must be > 0 bits.";
+        if ( ks <= 0 ) {
+            throw new IllegalArgumentException("Key size must be > 0 bits.");
+        }
         int numBytes = 0;
         int n = ks/8;
         int rem = ks % 8;

src/main/java/org/owasp/esapi/crypto/PlainText.java

@@ -42,7 +42,6 @@ public final class PlainText implements Serializable {
 	 */
 	public PlainText(String str) {
 		try {
-		    assert str != null : "String for plaintext cannot be null.";
 		    if ( str == null ) {
 		    	throw new IllegalArgumentException("String for plaintext may not be null!");
 		    }
@@ -61,8 +60,11 @@ public PlainText(String str) {
 	 */
 	public PlainText(byte[] b) {
 		// Must allow 0 length arrays though, to represent empty strings.
-		assert b != null : "Byte array representing plaintext cannot be null.";
-		    // Make copy so mutable byte array b can't change PlainText.
+		if ( b == null ) {
+            throw new IllegalArgumentException("Byte array representing plaintext cannot be null.");
+        }
+
+        // Make copy so mutable byte array b can't change PlainText.
 		rawBytes = new byte[ b.length ];
 		System.arraycopy(b, 0, rawBytes, 0, b.length);
 	}
@@ -156,4 +158,4 @@ public void overwrite() {
     protected boolean canEqual(Object other) {
         return (other instanceof PlainText);
     }
-}
+}

src/main/java/org/owasp/esapi/crypto/SecurityProviderLoader.java

@@ -155,7 +155,12 @@ public static int insertProviderAt(String algProvider, int pos)
         Class<?> providerClass = null;
         String clzName = null;
         Provider cryptoProvider = null;
-        assert (pos == -1 || pos >= 1) : "Position pos must be -1 or integer >= 1";
+
+        // Yes; I'm aware of DeMorgan's Law, but this is easier to grok.
+        if ( ! ( (pos == -1 || pos >= 1) ) ) {
+            throw new IllegalArgumentException("Position pos must be -1 or integer >= 1");
+        }
+
         try {
             // Does algProvider look like a class name?
             if (algProvider.indexOf('.') != -1) {

src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java

@@ -788,6 +788,7 @@ public String getEncryptionAlgorithm() {
 	 * {@inheritDoc}
 	 */
     public String getCipherTransformation() {
+        // Assertion should be okay here. An NPE is likely at runtime if disabled.
     	assert cipherXformCurrent != null : "Current cipher transformation is null";
     	return cipherXformCurrent;
     }
@@ -796,16 +797,17 @@ public String getCipherTransformation() {
      * {@inheritDoc}
      */
     public String setCipherTransformation(String cipherXform) {
-    	String previous = getCipherTransformation();
-    	if ( cipherXform == null ) {
-    		// Special case... means set it to original value from ESAPI.properties
-    		cipherXformCurrent = cipherXformFromESAPIProp;
-    	} else {
-    		assert ! cipherXform.trim().equals("") :
-    			"Cipher transformation cannot be just white space or empty string";
-    		cipherXformCurrent = cipherXform;	// Note: No other sanity checks!!!
-    	}
-    	return previous;
+        String previous = getCipherTransformation();
+        if ( cipherXform == null ) {
+            // Special case... means set it to original value from ESAPI.properties
+            cipherXformCurrent = cipherXformFromESAPIProp;
+        } else {
+            if ( cipherXform.trim().equals("") ) {
+                throw new ConfigurationException("Cipher transformation cannot be just white space or empty string");
+            }
+            cipherXformCurrent = cipherXform;   // Note: No other sanity checks!!!
+        }
+        return previous;
     }
 
     /**

src/main/java/org/owasp/esapi/util/ByteConversionUtil.java

@@ -77,14 +77,19 @@ public static byte[] fromLong(long input) {
 
     /**
      * Converts a given byte array to an {@code short}. Bytes are expected in
-     * network byte
-     * order.
+     * network byte order.
      *
-     * @param input A network byte-ordered representation of an {@code short}.
+     * @param input A network byte-ordered representation of an {@code short},
+     *              so exactly 2 bytes are expected.
      * @return The {@code short} value represented by the input array.
      */
     public static short toShort(byte[] input) {
-        assert input.length == 2 : "toShort(): Byte array length must be 2.";
+        if ( input == null ) {
+            throw new IllegalArgumentException("input: parameter may not be null.");
+        }
+        if ( input.length != 2 ) {
+            throw new IllegalArgumentException("input: Byte array length must be 2.");
+        }
         short output = 0;
         output = (short)(((input[0] & 0xff) << 8) | (input[1] & 0xff));
         return output;
@@ -95,10 +100,16 @@ public static short toShort(byte[] input) {
      * network byte order.
      *
      * @param input A network byte-ordered representation of an {@code int}.
+     *              Must be exactly 4 bytes.
      * @return The {@code int} value represented by the input array.
      */
     public static int toInt(byte[] input) {
-        assert input.length == 4 : "toInt(): Byte array length must be 4.";
+        if ( input == null ) {
+            throw new IllegalArgumentException("input: parameter may not be null.");
+        }
+        if ( input.length != 4 ) {
+            throw new IllegalArgumentException("input: Byte array length must be 4.");
+        }
         int output = 0;
         output = ((input[0] & 0xff) << 24) | ((input[1] & 0xff) << 16) |
                  ((input[2] & 0xff) << 8) | (input[3] & 0xff);
@@ -110,10 +121,16 @@ public static int toInt(byte[] input) {
      * network byte
      *
      * @param input A network byte-ordered representation of a {@code long}.
+     *              Must be exactly 8 bytes.
      * @return The {@code long} value represented by the input array
      */
     public static long toLong(byte[] input) {
-        assert input.length == 8 : "toLong(): Byte array length must be 8.";
+        if ( input == null ) {
+            throw new IllegalArgumentException("input: parameter may not be null.");
+        }
+        if ( input.length != 8 ) {
+            throw new IllegalArgumentException("input: Byte array length must be 8.");
+        }
         long output = 0;
         output  = ((long)(input[0] & 0xff) << 56);
         output |= ((long)(input[1] & 0xff) << 48);
@@ -125,4 +142,4 @@ public static long toLong(byte[] input) {
         output |= (input[7] & 0xff);
         return output;
     }
-}
+}

src/test/java/org/owasp/esapi/crypto/CryptoTokenTest.java

@@ -183,13 +183,11 @@ public final void testSetUserAccountName() {
         try {
             ctok.setUserAccountName(null);    // Can't be null
                 // Should get one of these, depending on whether or not assertions are enabled.
-            fail("Failed to throw expected AssertionError or NullPointerException");
-        } catch (ValidationException e) {
-            fail("Wrong type of exception thrown (ValidationException): " + e);
-        } catch (NullPointerException e) {
-            ;   // Success
-        } catch (AssertionError e) {
+            fail("Failed to throw expected IllegalArgumentException");
+        } catch (IllegalArgumentException e) {
             ;   // Success
+        } catch (Exception e) {
+            fail("Wrong type of exception thrown: " + e);
         }
         try {
             ctok.setUserAccountName("1773g4l");    // Can't start w/ numeric

src/test/java/org/owasp/esapi/crypto/PlainTextTest.java

@@ -65,20 +65,11 @@ public final void testNullCase() {
 	    try {
             byte[] bytes = null;
             PlainText pt = new PlainText(bytes);
-            assertTrue( pt != null );   // Should never get to here.
-            fail("testNullCase(): Expected NullPointerException or AssertionError");
-        } catch (NullPointerException e) {
-            // Will get this case if assertions are not enabled for PlainText.
-            // System.err.println("Caught NullPointerException; exception was: " + e);
-            // e.printStackTrace(System.err);
-            counter++;
-        } catch (AssertionError e) {
-            // Will get this case if assertions *are* enabled for PlainText.
-            // System.err.println("Caught AssertionError; exception was: " + e);
-            // e.printStackTrace(System.err);
-            counter++;
-        } finally {
-            assertTrue( counter > 0 );
+            fail("testNullCase(): Expected IllegalArgumentException");
+        } catch (IllegalArgumentException e) {
+            ;   // Success
+        } catch (Exception e) {
+            fail("testNullCase(): Caught unexpected exception: " + e);
         }
 	}