Merge pull request #506 from kwwall/issue-245

Not going to pretend I'm smart enough to understand the changes made in #245. Crypto isn't my top skill.  Overall all these changes appear to be pretty small overall.  Merging.

Author: xeno6696

documentation/esapi4java-core-2.0-ciphertext-serialization.pdf

@@ -697,9 +697,12 @@ public String toString() {
         int n = getRawCipherTextByteLength();
         String rawCipherText = (( n > 0 ) ? "present (" + n + " bytes)" : "absent");
         String mac = (( separate_mac_ != null ) ? "present" : "absent");
-        sb.append("Creation time: ").append(creationTime);
-        sb.append(", raw ciphertext is ").append(rawCipherText);
-        sb.append(", MAC is ").append(mac).append("; ");
+
+        sb.append("KDF Version: ").append( kdfVersion_ );
+        sb.append(", KDF PRF: ").append( kdfPRFAsInt() );
+        sb.append("; Creation time: ").append(creationTime);
+        sb.append("; raw ciphertext is ").append(rawCipherText);
+        sb.append("; MAC is ").append(mac).append("; ");
         sb.append( cipherSpec_.toString() );
         return sb.toString();
     }
@@ -792,6 +795,15 @@ protected boolean canEqual(Object other) {
      * <pre>
      *      HMAC-SHA1(nonce, IV + plaintext)
      * </pre>
+     * Note that <i>only</i> HMAC-SHA1 is used for the MAC calcuation. Unlike
+     * the PRF used for derived key generation in the {@code KeyDerivationFunction}
+     * class, the user cannot change the algorithm used to compute the MAC itself.
+     * One reason for that is that we don't want the MAC value to be excessively
+     * long; 128 bits is already quite long when only encrypting short strings.
+     * Also while the NSA reviewed this and were okay with it, Bellare, Canetti & Krawczyk
+     * proved in 1996 [see http://pssic.free.fr/Extra%20Reading/SEC+/SEC+/hmac-cb.pdf] that
+     * HMAC security doesn’t require that the underlying hash function be collision resistant,
+     * but only that it acts as a pseudo-random function, which SHA1 satisfies.
      * @param ciphertext    The ciphertext value for which the MAC is computed.
      * @return The value for the MAC.
      */ 

documentation/esapi4java-core-2.0-ciphertext-serialization.xls

@@ -126,18 +126,23 @@ public static SecretKey generateSecretKey(String alg, int keySize)
 	 * 						this is thrown with the original {@code UnsupportedEncodingException}
 	 * 						as the cause. (NOTE: This should never happen as "UTF-8" is supposed to
 	 * 						be a common encoding supported by all Java implementations. Support
-	 * 					    for it is usually in rt.jar.)
+	 * 					    for it is usually in rt.jar.) This exception is also thrown if the
+     * 					    requested {@code keySize} parameter exceeds the length of the number of
+     * 					    bytes provded in the {@code keyDerivationKey} parameter.
 	 * @throws InvalidKeyException 	Likely indicates a coding error. Should not happen.
 	 * @throws EncryptionException  Throw for some precondition violations.
-	 * @deprecated Use{@code KeyDerivationFunction} instead. This method will be removed as of
-	 * 			   ESAPI release 2.3 so if you are using this, please change your code.
+	 * @deprecated Use same method in {@code KeyDerivationFunction} instead. This method will be <b>removed</b> as of
+	 * 			   ESAPI release 2.3 so if you are using this, please CHANGE YOUR CODE. Note that the replacement
+     * 			   is not a static method, so create your own wrapper if you wish, but this will soon disappear.
 	 */
     @Deprecated
 	public static SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySize, String purpose)
 			throws NoSuchAlgorithmException, InvalidKeyException, EncryptionException
 	{
-        // These really should be turned into actual runtime checks and an
-        // IllegalArgumentException should be thrown if they are violated.
+        // Fingers cross; maybe this will help.
+        logger.warning(Logger.SECURITY_AUDIT,
+                "Your code is using the deprecated CryptoHelper.computeDerivedKey() method which will be removed next release");
+
 		if ( keyDerivationKey == null ) {
             throw new IllegalArgumentException("Key derivation key cannot be null.");
         }
@@ -159,6 +164,9 @@ public static SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySiz
 		// 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
 		//			previous ESAPI versions. JavaEncryptor doesn't use this any longer.
+        // ANSWER:  This is deprecated and will be removed in 2.3.0.0, so it really matter
+        //          that much. However, Since the property Encryptor.KDF.PRF is (and has
+        //          been) "HMacSHA256". changing this could unintentionally break code.
 		KeyDerivationFunction kdf = new KeyDerivationFunction(
 											KeyDerivationFunction.PRF_ALGORITHMS.HmacSHA1);
 		return kdf.computeDerivedKey(keyDerivationKey, keySize, purpose);
@@ -260,7 +268,8 @@ public static boolean isCipherTextMACvalid(SecretKey sk, CipherText ct)
     {
         if ( CryptoHelper.isMACRequired( ct ) ) {
             try {
-                SecretKey authKey = CryptoHelper.computeDerivedKey( sk, ct.getKeySize(), "authenticity");
+		        KeyDerivationFunction kdf = new KeyDerivationFunction( ct.getKDF_PRF() );
+                SecretKey authKey = kdf.computeDerivedKey(sk, ct.getKeySize(), "authenticity");
                 boolean validMAC = ct.validateMAC( authKey );
                 return validMAC;
             } catch (Exception ex) {

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

@@ -187,7 +187,7 @@ public int getVersion() {
 	}
 
 
-	// TODO: IMPORTANT NOTE: In a future release (hopefully starting in 2.1.1),
+	// TODO: IMPORTANT NOTE: In a future release (hopefully starting in 2.3),
 	// we will be using the 'context' to mix in some additional things. At a
 	// minimum, we will be using the KDF version (version_) so that downgrade version
 	// attacks are not possible. Other candidates are the cipher xform and
@@ -267,24 +267,26 @@ public String getContext() {
 	 * @param keySize		The cipher's key size (in bits) for the {@code keyDerivationKey}.
 	 * 						Must have a minimum size of 56 bits and be an integral multiple of 8-bits.
 	 * 						<b>Note:</b> The derived key will have the same size as this.
-	 * @param purpose		The purpose for the derived key. For the ESAPI reference implementation,
+	 * @param purpose		The purpose for the derived key. <b>IMPORTANT:</b> For the ESAPI reference implementation,
 	 * 						{@code JavaEncryptor}, this <i>must</i> be either the string "encryption" or
 	 * 						"authenticity", where "encryption" is used for creating a derived key to use
 	 * 						for confidentiality, and "authenticity" is used for creating a derived key to
 	 * 						use with a MAC to ensure message authenticity. However, since parameter serves
 	 * 						the same purpose as the "Label" in section 5.1 of NIST SP 800-108, it really can
 	 * 						be set to anything other than {@code null} or an empty string when called outside
-	 * 						of {@code JavaEncryptor}.
+	 * 						of ESAPI's {@code JavaEncryptor} reference implementation (but you must consistent).
 	 * @return				The derived {@code SecretKey} to be used according
 	 * 						to the specified purpose. 
 	 * @throws NoSuchAlgorithmException		The {@code keyDerivationKey} has an unsupported
-	 * 						encryption algorithm or no current JCE provider supports
-	 * 						"HmacSHA1".
+	 * 						encryption algorithm or no current JCE provider supports requested
+     * 						Hmac algorithrm used for the PRF for key generation.
 	 * @throws EncryptionException		If "UTF-8" is not supported as an encoding, then
 	 * 						this is thrown with the original {@code UnsupportedEncodingException}
 	 * 						as the cause. (NOTE: This should never happen as "UTF-8" is supposed to
 	 * 						be a common encoding supported by all Java implementations. Support
-	 * 					    for it is usually in rt.jar.)
+	 * 					    for it is usually in rt.jar.) This exception is also thrown if the
+     * 					    requested {@code keySize} parameter exceeds the length of the number of
+     * 					    bytes provded in the {@code keyDerivationKey} parameter.
 	 * @throws InvalidKeyException 	Likely indicates a coding error. Should not happen.
 	 * @throws EncryptionException  Throw for some precondition violations.
 	 */
@@ -322,7 +324,29 @@ public SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySize, Stri
 			throw new IllegalArgumentException("Purpose may not be null or empty.");
 		}
 
-		keySize = calcKeySize( keySize );	// Safely convert to whole # of bytes.
+        //
+        // No longer, since we no longer wish to restrict this to use only by JavaEncryptor, so
+        // we no longer test for this.  For details, see the javadoc for '@param purpose', above.
+        //
+        /*
+         *
+         *      if ( ! ( purpose.equals("encryption") || purpose.equals("authenticity") ) ) {
+         *          throw new IllegalArgumentException("Purpose must be \"encryption\" or \"authenticity\".");
+         *      }
+         *
+         */
+
+        int providedKeyLen = 8 * keyDerivationKey.getEncoded().length;
+        // assert providedKeyLen >= 56 : "Coding error? Length of keyDerivationKey < 56 bits!";    // Ugh. DES compatible.
+
+        if ( providedKeyLen < keySize ) {
+            throw new EncryptionException("KeyDerivationFunction.computeDerivedKey() not intended for key stretching: " +
+                                          "provided key too short (" + providedKeyLen + " bits) to provide " + keySize + " bits.",
+                        "Key stretching not supported: Provided key, keyDerivationKey, has insufficient entropy (" +
+                            providedKeyLen + " bits) to generate key of requested size of " + keySize + " bits.");
+        }
+
+		keySize = calcKeySize( keySize );	// Safely convert from bits to a whole # of bytes.
 		byte[] derivedKey = new byte[ keySize ];
 		byte[] label;				    	// Same purpose as NIST SP 800-108's "label" in section 5.1.
 		byte[] context;						// See setContext() for details.
@@ -344,20 +368,20 @@ public SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySize, Stri
             // under the hood to create 'sk' so that it is 20 bytes. I cannot vouch
             // for how secure this key-stretching is. Worse, it might not be specified
             // as to *how* it is done and left to each JCE provider.
-		SecretKey sk = new SecretKeySpec(keyDerivationKey.getEncoded(), "HmacSHA1");
+      	SecretKey sk = new SecretKeySpec(keyDerivationKey.getEncoded(), prfAlg_ );
 		Mac mac = null;
 
 		try {
-			mac = Mac.getInstance("HmacSHA1");
+    		mac = Mac.getInstance( prfAlg_ );
 			mac.init(sk);
 		} catch( InvalidKeyException ex ) {
 			logger.error(Logger.SECURITY_FAILURE,
-					"Created HmacSHA1 Mac but SecretKey sk has alg " +
+					"Created " + prfAlg_ + " Mac but SecretKey sk has alg " +
 					sk.getAlgorithm(), ex);
 			throw ex;
 		}
 
-		// Repeatedly call of HmacSHA1 hash until we've collected enough bits
+		// Repeatedly call of PRF Hmac until we've collected enough bits
 		// for the derived key. The first time through, we calculate the HmacSHA1
 		// on the "purpose" string, but subsequent calculations are performed
 		// on the previous result.
@@ -415,6 +439,15 @@ public SecretKey computeDerivedKey(SecretKey keyDerivationKey, int keySize, Stri
 
 		// Don't leave remnants of the partial key in memory. (Note: we could
 		// not do this if tmpKey were declared in the do-while loop.
+        // Of course, in reality, trying to stomp these bits out is probably not
+        // realistic because the JIT is likely toing to be smart enough to
+        // optimze this loop away. We probably could try to outsmart it, by
+        // (say) writing out the overwritten bits to /dev/null, but then even
+        // then we'd still probably have to overwrite with random bits rather
+        // than all null chars. How much is enough? Who knows? But it does point
+        // to a serious limitation in Java and many other languages that one
+        // cannot arbitrarily disable the optimizer either at compile time or
+        // run time because of security reasons. Sigh. At least we've tried.
 		for ( int i = 0; i < tmpKey.length; i++ ) {
 			tmpKey[i] = '\0';
 		}

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

@@ -191,9 +191,8 @@ public final void testMIC() {
 	        encryptor.init(Cipher.ENCRYPT_MODE, key, ivSpec);
 	        byte[] raw = encryptor.doFinal("This is my secret message!!!".getBytes("UTF8"));
 	        CipherText ciphertext = new CipherText(cipherSpec, raw);
-	        	// TODO: Replace this w/ call to KeyDerivationFunction as this is
-	        	//		 deprecated! Shame on me!
-	        SecretKey authKey = CryptoHelper.computeDerivedKey(key, key.getEncoded().length * 8, "authenticity");
+		    KeyDerivationFunction kdf = new KeyDerivationFunction( KeyDerivationFunction.PRF_ALGORITHMS.HmacSHA1 );
+	        SecretKey authKey = kdf.computeDerivedKey(key, key.getEncoded().length * 8, "authenticity");
 	        ciphertext.computeAndStoreMAC( authKey );
 //          System.err.println("Original ciphertext being serialized: " + ciphertext);
 	        byte[] serializedBytes = ciphertext.asPortableSerializedByteArray();