Changes to close issue #679 (#680)

* Changes to close issue #679

* Updated comment about support for fixed IVs in ESAPI.properties file.

Author: kwwall

configuration/esapi/ESAPI.properties

@@ -194,9 +194,6 @@ Encryptor.cipher_modes.combined_modes=GCM,CCM,IAPM,EAX,OCB,CWC
 # Additional cipher modes allowed for ESAPI 2.0 encryption. These
 # cipher modes are in _addition_ to those specified by the property
 # 'Encryptor.cipher_modes.combined_modes'.
-# Note: We will add support for streaming modes like CFB & OFB once
-# we add support for 'specified' to the property 'Encryptor.ChooseIVMethod'
-# (probably in ESAPI 2.1).
 # DISCUSS: Better name?
 Encryptor.cipher_modes.additional_allowed=CBC
 
@@ -223,37 +220,27 @@ Encryptor.EncryptionKeyLength=128
 Encryptor.MinEncryptionKeyLength=128
 
 # Because 2.x uses CBC mode by default, it requires an initialization vector (IV).
-# (All cipher modes except ECB require an IV.) There are two choices: we can either
-# use a fixed IV known to both parties or allow ESAPI to choose a random IV. While
-# the IV does not need to be hidden from adversaries, it is important that the
-# adversary not be allowed to choose it. Also, random IVs are generally much more
-# secure than fixed IVs. (In fact, it is essential that feed-back cipher modes
-# such as CFB and OFB use a different IV for each encryption with a given key so
-# in such cases, random IVs are much preferred. By default, ESAPI 2.0 uses random
-# IVs. If you wish to use 'fixed' IVs, set 'Encryptor.ChooseIVMethod=fixed' and
-# uncomment the Encryptor.fixedIV.
-#
-# Valid values:		random|fixed|specified		'specified' not yet implemented; planned for 2.3
-#                                               'fixed' is deprecated as of 2.2
-#                                               and will be removed in 2.3.
+# (All cipher modes except ECB require an IV.) Previously there were two choices: we can either
+# use a fixed IV known to both parties or allow ESAPI to choose a random IV. The
+# former was deprecated in ESAPI 2.2 and removed in ESAPI 2.3. It was not secure
+# because the Encryptor (as are all the other major ESAPI components) is a
+# singleton and thus the same IV would get reused each time. It was not a
+# well-thought out plan. (To do it correctly means we need to add a setIV() method
+# and get rid of the Encryptor singleton, thus it will not happen until 3.0.)
+# However, while the IV does not need to be hidden from adversaries, it is important that the
+# adversary not be allowed to choose it. Thus for now, ESAPI just chooses a random IV.
+# Originally there was plans to allow a developer to provide a class and method
+# name to define a custom static method to generate an IV, but that is just
+# trouble waiting to happen. Thus in effect, the ONLY acceptable property value
+# for this property is "random". In the not too distant future (possibly the
+# next release), I will be removing it, but for now I am leaving this and
+# checking for it so a ConfigurationException can be thrown if anyone using
+# ESAPI ignored the deprecation warning message and still has it set to "fixed".
+#
+# Valid values:		random
 Encryptor.ChooseIVMethod=random
 
 
-# If you choose to use a fixed IV, then you must place a fixed IV here that
-# is known to all others who are sharing your secret key. The format should
-# be a hex string that is the same length as the cipher block size for the
-# cipher algorithm that you are using. The following is an *example* for AES
-# from an AES test vector for AES-128/CBC as described in:
-# NIST Special Publication 800-38A (2001 Edition)
-# "Recommendation for Block Cipher Modes of Operation".
-# (Note that the block size for AES is 16 bytes == 128 bits.)
-#
-#   @Deprecated -- fixed IVs are deprecated as of the 2.2 release and support
-#                  will be removed in the next release (tentatively, 2.3).
-#                  If you MUST use this, at least replace this IV with one
-#                  that your legacy application was using.
-Encryptor.fixedIV=0x000102030405060708090a0b0c0d0e0f
-
 # Whether or not CipherText should use a message authentication code (MAC) with it.
 # This prevents an adversary from altering the IV as well as allowing a more
 # fool-proof way of determining the decryption failed because of an incorrect

documentation/esapi4java-core-2.0-symmetric-crypto-user-guide.html

@@ -149,8 +149,9 @@ <H2>ESAPI.properties Properties Relevant to Symmetric Encryption</H2>
 			compatibility with legacy or third party software. If set to
 			“fixed”, then the property Encryptor.fixedIV must also be
 			set to hex-encoded specific IV that you need to use.
-            <B>NOTE:</B> "fixed" is deprecated and will be removed by
-            release 2.3.
+            <B>NOTE:</B> "fixed" had been deprecated since 2.2.0.0 and finally
+            was removed for release 2.3.0.0. Using it in versions 2.3.0.0 or
+            later will result in a <code>ConfigurationException</code> being thrown.
             </FONT></P><P><FONT SIZE=2>
             <B>CAUTION:</B> While it is not required that the IV be kept
             secret, encryption relying on fixed IVs can lead to a known

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

@@ -389,35 +389,22 @@ public interface SecurityConfiguration extends EsapiPropertyLoader {
      * fixed IVs, but the use of non-random IVs is inherently insecure,
      * especially for any supported cipher mode that is considered a streaming mode
      * (which is basically anything except CBC for modes that support require an IV).
-     * For this reason, 'fixed' is considered <b>deprecated</b> and will be
-     * removed during the next ESAPI point release (tentatively, 2.3).
-     * However, note that if a "fixed" IV is chosen, then the
-     * the value of this fixed IV must be specified as the property
-     * {@code Encryptor.fixedIV} and be of the appropriate length.
+     * For this reason, 'fixed' has now been removed (it was considered <b>deprecated</b>
+     * since release 2.2.0.0). An <b>ESAPI.properties</b> value of {@Code fixed} for the property
+     * {@Code Encryptor.ChooseIVMethod} will now result in a {@Code ConfigurationException}
+     * being thrown.
      * 
-     * @return A string specifying the IV type. Should be "random" or "fixed" (dereprected).
+     * @return A string specifying the IV type. Should be "random". Anything
+     * else should fail with a {@Code ConfigurationException} being thrown.
      * 
      * @see #getFixedIV()
      * @deprecated Use SecurityConfiguration.getStringProp("appropriate_esapi_prop_name") instead.
+     *             This method will be removed in a future release as it is now moot since
+     *             it can only legitimately have the single value of "random".
      */
 	@Deprecated
     String getIVType();
 
-    /**
-     * If a "fixed" (i.e., static) Initialization Vector (IV) is to be used,
-     * this will return the IV value as a hex-encoded string.
-     * @return The fixed IV as a hex-encoded string.
-     * @deprecated Short term: use SecurityConfiguration.getByteArrayProp("appropriate_esapi_prop_name")
-     *             instead. Longer term: There will be a more general method in JavaEncryptor
-     *             to explicitly set an IV. This whole concept of a single fixed IV has
-     *             always been a kludge at best, as a concession to those who have used
-     *             a single fixed IV in the past to support legacy applications. This method will be
-     *             killed off in the next ESAPI point release (likely 2.3). It's time to put it to death
-     *             as it was never intended for production in the first place.
-     */
-	@Deprecated
-    String getFixedIV();
-    
     /**
      * Return a {@code List} of strings of combined cipher modes that support
      * <b>both</b> confidentiality and authenticity. These would be preferred

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

@@ -320,11 +320,10 @@ public int getRawCipherTextByteLength() {
      * base64-encoding is performed.
      * <p>
      * If there is a need to store an encrypted value, say in a database, this
-     * is <i>not</i> the method you should use unless you are using a <i>fixed</i>
-     * IV or are planning on retrieving the IV and storing it somewhere separately
-     * (e.g., a different database column). If you are <i>not</i> using a fixed IV
-     * (which is <strong>highly</strong> discouraged), you should normally use
-     * {@link #getEncodedIVCipherText()} instead.
+     * is <i>not</i> the method you should use unless you are using are storing the
+     * IV separately (i.e., in a separate DB column), which doesn't make a lot of sense.
+     * Normally, you should prefer the method {@link #getEncodedIVCipherText()} instead as
+     * it will return the IV prepended to the ciphertext.
      * </p>
      * @see #getEncodedIVCipherText()
      */
@@ -338,11 +337,6 @@ public String getBase64EncodedRawCipherText() {
      * base64-encoding. If an IV is not used, then this method returns the same
      * value as {@link #getBase64EncodedRawCipherText()}.
      * <p>
-     * Generally, this is the method that you should use unless you only
-     * are using a fixed IV and a storing that IV separately, in which case
-     * using {@link #getBase64EncodedRawCipherText()} can reduce the storage
-     * overhead.
-     * </p>
      * @return The base64-encoded ciphertext or base64-encoded IV + ciphertext.
      * @see #getBase64EncodedRawCipherText()
      */
@@ -591,8 +585,8 @@ public void setIVandCiphertext(byte[] iv, byte[] ciphertext)
 // TODO: FIXME: As per email from Jeff Walton to Kevin Wall dated 12/03/2013,
 //            this is not always true. E.g., for CCM, the IV length is supposed
 //            to be 7, 8,  7, 8, 9, 10, 11, 12, or 13 octets because of
-//            it's formatting function, the restof the octets used by the
-//            nonce/counter.
+//            it's formatting function, the rest of the octets are used by the
+//            nonce/counter. E.g., see RFCs 4309, 8750, and related RFCs.
                     throw new EncryptionException("Encryption failed -- bad parameters passed to encrypt",  // DISCUSS - also log? See below.
                                                   "IV length does not match cipher block size of " + getBlockSize());
             }

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

@@ -121,10 +121,9 @@ public static SecurityConfiguration getInstance() {
     public static final String CIPHER_TRANSFORMATION_IMPLEMENTATION = "Encryptor.CipherTransformation";
     public static final String CIPHERTEXT_USE_MAC = "Encryptor.CipherText.useMAC";
     public static final String PLAINTEXT_OVERWRITE = "Encryptor.PlainText.overwrite";
-    public static final String IV_TYPE = "Encryptor.ChooseIVMethod";
 
     @Deprecated
-    public static final String FIXED_IV = "Encryptor.fixedIV";
+    public static final String IV_TYPE = "Encryptor.ChooseIVMethod";    // Will be removed in future release.
 
     public static final String COMBINED_CIPHER_MODES = "Encryptor.cipher_modes.combined_modes";
     public static final String ADDITIONAL_ALLOWED_CIPHER_MODES = "Encryptor.cipher_modes.additional_allowed";
@@ -251,6 +250,13 @@ public static SecurityConfiguration getInstance() {
      */
     public DefaultSecurityConfiguration(Properties properties) {
     	resourceFile = DEFAULT_RESOURCE_FILE;
+    	try {
+            this.esapiPropertyManager = new EsapiPropertyManager();
+            // Do NOT call loadConfiguration() here!
+        } catch( IOException e ) {
+	        logSpecial("Failed to load security configuration", e );
+	        throw new ConfigurationException("Failed to load security configuration", e);
+        }
     	this.properties = properties; 
     	this.setCipherXProperties();
     }
@@ -265,7 +271,7 @@ private void setCipherXProperties() {
 		// TODO: FUTURE: Replace by future CryptoControls class???
 		// See SecurityConfiguration.setCipherTransformation() for
 		// explanation of this.
-        // (Propose this in 2.1 via future email to ESAPI-DEV list.)
+        // (Propose this in a future 2.x release via future email to ESAPI-DEV list.)
 		cipherXformFromESAPIProp =
 			getESAPIProperty(CIPHER_TRANSFORMATION_IMPLEMENTATION,
 							 "AES/CBC/PKCS5Padding");
@@ -832,49 +838,26 @@ public boolean overwritePlainText() {
     /**
 	 * {@inheritDoc}
 	 */
+    @Deprecated
     public String getIVType() {
     	String value = getESAPIProperty(IV_TYPE, "random");
     	if ( value.equalsIgnoreCase("random") ) {
             return value;
         } else if ( value.equalsIgnoreCase("fixed") ) {
-            logSpecial("WARNING: Property '" + IV_TYPE + "=fixed' is DEPRECATED. It was intended to support legacy applications, but is inherently insecure, especially with any streaming mode. Support for this will be completed dropped next ESAPI minor release (probably 2.3");
-    		return value;
+            logSpecial("WARNING: Property '" + IV_TYPE + "=fixed' is no longer supported AT ALL!!! It had been deprecated since 2.2.0.0 and back then, was announced it would be removed in release 2.3.0.0. It was originally intended to support legacy applications, but is inherently insecure, especially with any streaming mode.");
+    		throw new ConfigurationException("'" + IV_TYPE + "=fixed' is no longer supported AT ALL. It has been deprecated since release 2.2 and has been removed since 2.3.");
     	} else if ( value.equalsIgnoreCase("specified") ) {
-    		// This is planned for future implementation where setting
-    		// Encryptor.ChooseIVMethod=specified   will require setting some
-    		// other TBD property that will specify an implementation class that
-    		// will generate appropriate IVs. The intent of this would be to use
-    		// such a class with various feedback modes where it is imperative
-    		// that for a given key, any particular IV is *NEVER* reused. For
-    		// now, we will assume that generating a random IV is usually going
-    		// to be sufficient to prevent this.
-    		throw new ConfigurationException("'" + IV_TYPE + "=specified' is not yet implemented. Use 'random' for now.");
-    	} else {
-    		// TODO: Once 'specified' is legal, adjust exception msg, below.
-    		// DISCUSS: Could just log this and then silently return "random" instead.
-    		throw new ConfigurationException(value + " is illegal value for " + IV_TYPE +
-    										 ". Use 'random'.");
-    	}
-    }
-
-    /**
-	 * {@inheritDoc}
-	 */
-    @Deprecated
-    public String getFixedIV() {
-    	if ( getIVType().equalsIgnoreCase("fixed") ) {
-    		String ivAsHex = getESAPIProperty(FIXED_IV, ""); // No default
-    		if ( ivAsHex == null || ivAsHex.trim().equals("") ) {
-    			throw new ConfigurationException("Fixed IV requires property " +
-    						FIXED_IV + " to be set, but it is not.");
-    		}
-    		return ivAsHex;		// We do no further checks here as we have no context.
+    		// Originally, this was planned for future implementation where setting
+    		//      Encryptor.ChooseIVMethod=specified
+            // would have allowed a dev to write their own static method to be
+            // invoked in a future TBD property, but that is a recipe for
+            // disaster. So, it's not going to happen. Ever.
+    		throw new ConfigurationException("Contrary to previous internal comments, '" + IV_TYPE + "=specified' is not going to be supported -- ever.");
     	} else {
-    		// DISCUSS: Should we just log a warning here and return null instead?
-    		//			If so, may cause NullPointException somewhere later.
-    		throw new ConfigurationException("IV type not 'fixed' [which is DEPRECATED!] (set to '" +
-    										 getIVType() + "'), so no fixed IV applicable.");
+    		logSpecial("WARNING: '" + value + "' is illegal value for " + IV_TYPE +
+    										 ". Using 'random' for the IV type.");
     	}
+        return "random";
     }
 
     /**

src/main/java/org/owasp/esapi/reference/crypto/JavaEncryptor.java

@@ -464,25 +464,10 @@ public CipherText encrypt(SecretKey key, PlainText plain)
                  IvParameterSpec ivSpec = null;
                  if ( ivType.equalsIgnoreCase("random") ) {
                      ivBytes = ESAPI.randomizer().getRandomBytes(encrypter.getBlockSize());
-                 } else if ( ivType.equalsIgnoreCase("fixed") ) {
-                     String fixedIVAsHex = ESAPI.securityConfiguration().getFixedIV();
-                     ivBytes = Hex.decode(fixedIVAsHex);
-                     /* FUTURE       } else if ( ivType.equalsIgnoreCase("specified")) {
-                            // FUTURE - TODO  - Create instance of specified class to use for IV generation and
-                            //                   use it to create the ivBytes. (The intent is to make sure that
-                            //                   1) IVs are never repeated for cipher modes like OFB and CFB, and
-                            //                   2) to screen for weak IVs for the particular cipher algorithm.
-                            //      In meantime, use 'random' for block cipher in feedback mode. Unlikely they will
-                            //      be repeated unless you are salting SecureRandom with same value each time. Anything
-                            //      monotonically increasing should be suitable, like a counter, but need to remember
-                            //      it across JVM restarts. Was thinking of using System.currentTimeMillis(). While
-                            //      it's not perfect it probably is good enough. Could even all (advanced) developers
-                            //      to define their own class to create a unique IV to allow them some choice, but
-                            //      definitely need to provide a safe, default implementation.
-                      */
                  } else {
-                     // TODO: Update to add 'specified' once that is supported and added above.
-                     throw new ConfigurationException("Property Encryptor.ChooseIVMethod must be set to 'random' or 'fixed'");
+                     // This really shouldn't happen here. Show catch it a few
+                     // lines above.
+                     throw new ConfigurationException("Property Encryptor.ChooseIVMethod must be set to 'random'.");
                  }
                  ivSpec = new IvParameterSpec(ivBytes);
                  cipherSpec.setIV(ivBytes);

src/test/java/org/owasp/esapi/SecurityConfigurationWrapper.java

@@ -291,14 +291,6 @@ public String getIVType()
 		return wrapped.getIVType();
 	}
 
-	/**
-	 * {@inheritDoc}
-	 */
-	// @Override
-	public String getFixedIV()
-	{
-		return wrapped.getFixedIV();
-	}
 
 	/**
 	 * {@inheritDoc}

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

@@ -29,10 +29,7 @@ public class CipherSpecTest extends TestCase {
 	private byte[] myIV = null;
 
 	@Before public void setUp() throws Exception {
-			// This will throw ConfigurationException if IV type is not set to
-			// 'fixed', which it's not. (We have it set to 'random'.)
-		// myIV = Hex.decode( ESAPI.securityConfiguration().getFixedIV() );
-		myIV = Hex.decode( "0x000102030405060708090a0b0c0d0e0f" );
+		myIV = Hex.decode( "0x000102030405060708090a0b0c0d0e0f" ); // Any IV to test w/ will do.
 
 		dfltAESCipher   = Cipher.getInstance("AES");
 		dfltECBCipher   = Cipher.getInstance("AES/ECB/NoPadding");