On 11/27/2012 03:50 AM, Xuelei Fan wrote:
The fix looks fine to me. Would you like also to cleanup the comments (and code for get method) related to both setCertificateVerifyAlg()/getCertificateVerifyHash() in HandshakeHash.java?
Good point. restrictCertificateVerifyAlgs() was effectively a NOP as well, so I removed that, too.
New patch attached. -- Florian Weimer / Red Hat Product Security Team
# HG changeset patch # User Florian Weimer <[email protected]> # Date 1353688419 -3600 # Node ID ae640de33bb65ff9cb608605db209ab1ab4a32fa # Parent 9b9a751461efb879fcc08f5d0e09274c72409b07 Remove dead code related to signature_algorithms extension in sun.security.ssl diff --git a/src/share/classes/sun/security/ssl/ClientHandshaker.java b/src/share/classes/sun/security/ssl/ClientHandshaker.java --- a/src/share/classes/sun/security/ssl/ClientHandshaker.java +++ b/src/share/classes/sun/security/ssl/ClientHandshaker.java @@ -557,10 +557,6 @@ } if (resumingSession && session != null) { - if (protocolVersion.v >= ProtocolVersion.TLS12.v) { - handshakeHash.setCertificateVerifyAlg(null); - } - setHandshakeSessionSE(session); return; } @@ -975,8 +971,6 @@ throw new SSLHandshakeException( "No supported hash algorithm"); } - - handshakeHash.setCertificateVerifyAlg(hashAlg); } m3 = new CertificateVerify(protocolVersion, handshakeHash, @@ -994,10 +988,6 @@ } m3.write(output); output.doHashes(); - } else { - if (protocolVersion.v >= ProtocolVersion.TLS12.v) { - handshakeHash.setCertificateVerifyAlg(null); - } } /* diff --git a/src/share/classes/sun/security/ssl/HandshakeHash.java b/src/share/classes/sun/security/ssl/HandshakeHash.java --- a/src/share/classes/sun/security/ssl/HandshakeHash.java +++ b/src/share/classes/sun/security/ssl/HandshakeHash.java @@ -29,7 +29,6 @@ import java.io.ByteArrayOutputStream; import java.security.*; import java.util.Locale; -import java.util.Set; /** * Abstraction for the SSL/TLS hash of all handshake messages that is @@ -49,28 +48,23 @@ * 1. protocolDetermined(version) should be called when the negotiated * protocol version is determined. * - * 2. Before protocolDetermined() is called, only update(), reset(), - * restrictCertificateVerifyAlgs(), setFinishedAlg(), and - * setCertificateVerifyAlg() can be called. + * 2. Before protocolDetermined() is called, only update(), and reset() + * can be called. * * 3. After protocolDetermined() is called, reset() cannot be called. * * 4. After protocolDetermined() is called, if the version is pre-TLS 1.2, - * getFinishedHash() and getCertificateVerifyHash() cannot be called. Otherwise, + * getFinishedHash() cannot be called. Otherwise, * getMD5Clone() and getSHAClone() cannot be called. * * 5. getMD5Clone() and getSHAClone() can only be called after * protocolDetermined() is called and version is pre-TLS 1.2. * - * 6. getFinishedHash() and getCertificateVerifyHash() can only be called after - * all protocolDetermined(), setCertificateVerifyAlg() and setFinishedAlg() - * have been called and the version is TLS 1.2. If a CertificateVerify message - * is to be used, call setCertificateVerifyAlg() with the hash algorithm as the - * argument. Otherwise, you still must call setCertificateVerifyAlg(null) before - * calculating any hash value. + * 6. getFinishedHash() can only be called after protocolDetermined() + * and setFinishedAlg() have been called and the version is TLS 1.2. * - * Suggestions: Call protocolDetermined(), restrictCertificateVerifyAlgs(), - * setFinishedAlg(), and setCertificateVerifyAlg() as early as possible. + * Suggestion: Call protocolDetermined() and setFinishedAlg() + * as early as possible. * * Example: * <pre> @@ -80,21 +74,13 @@ * hh.setFinishedAlg("SHA-256"); * hh.update(serverHelloBytes); * ... - * hh.setCertificateVerifyAlg("SHA-384"); * hh.update(CertificateVerifyBytes); - * byte[] cvDigest = hh.getCertificateVerifyHash(); * ... * hh.update(finished1); * byte[] finDigest1 = hh.getFinishedHash(); * hh.update(finished2); * byte[] finDigest2 = hh.getFinishedHash(); * </pre> - * If no CertificateVerify message is to be used, call - * <pre> - * hh.setCertificateVerifyAlg(null); - * </pre> - * This call can be made once you are certain that this message - * will never be used. */ final class HandshakeHash { @@ -105,28 +91,19 @@ // 2: TLS 1.2 private int version = -1; private ByteArrayOutputStream data = new ByteArrayOutputStream(); - private final boolean isServer; // For TLS 1.1 private MessageDigest md5, sha; private final int clonesNeeded; // needs to be saved for later use // For TLS 1.2 - // cvAlgDetermined == true means setCertificateVerifyAlg() is called - private boolean cvAlgDetermined = false; - private String cvAlg; private MessageDigest finMD; /** * Create a new HandshakeHash. needCertificateVerify indicates whether - * a hash for the certificate verify message is required. The argument - * algs is a set of all possible hash algorithms that might be used in - * TLS 1.2. If the caller is sure that TLS 1.2 won't be used or no - * CertificateVerify message will be used, leave it null or empty. + * a hash for the certificate verify message is required. */ - HandshakeHash(boolean isServer, boolean needCertificateVerify, - Set<String> algs) { - this.isServer = isServer; + HandshakeHash(boolean needCertificateVerify) { clonesNeeded = needCertificateVerify ? 3 : 2; } @@ -256,47 +233,11 @@ finMD.update(data.toByteArray()); } - /** - * Restricts the possible algorithms for the CertificateVerify. Called by - * the server based on info in CertRequest. The argument must be a subset - * of the argument with the same name in the constructor. The method can be - * called multiple times. If the caller is sure that no CertificateVerify - * message will be used, leave this argument null or empty. - */ - void restrictCertificateVerifyAlgs(Set<String> algs) { - if (version == 1) { - throw new RuntimeException( - "setCertificateVerifyAlg() cannot be called for TLS 1.1"); - } - // Not used yet - } - - /** - * Specifies the hash algorithm used in CertificateVerify. - * Can be called multiple times. - */ - void setCertificateVerifyAlg(String s) { - - // Can be called multiple times, but only set once - if (cvAlgDetermined) return; - - cvAlg = s == null ? null : normalizeAlgName(s); - cvAlgDetermined = true; - } - byte[] getAllHandshakeMessages() { return data.toByteArray(); } /** - * Calculates the hash in the CertificateVerify. Must be called right - * after setCertificateVerifyAlg() - */ - /*byte[] getCertificateVerifyHash() { - throw new Error("Do not call getCertificateVerifyHash()"); - }*/ - - /** * Calculates the hash in Finished. Must be called after setFinishedAlg(). * This method can be called twice, for Finished messages of the server * side and client side respectively. diff --git a/src/share/classes/sun/security/ssl/Handshaker.java b/src/share/classes/sun/security/ssl/Handshaker.java --- a/src/share/classes/sun/security/ssl/Handshaker.java +++ b/src/share/classes/sun/security/ssl/Handshaker.java @@ -489,11 +489,7 @@ // We accumulate digests of the handshake messages so that // we can read/write CertificateVerify and Finished messages, // getting assurance against some particular active attacks. - Set<String> localSupportedHashAlgorithms = - SignatureAndHashAlgorithm.getHashAlgorithmNames( - getLocalSupportedSignAlgs()); - handshakeHash = new HandshakeHash(!isClient, needCertVerify, - localSupportedHashAlgorithms); + handshakeHash = new HandshakeHash(needCertVerify); // Generate handshake input/output stream. input = new HandshakeInStream(handshakeHash); diff --git a/src/share/classes/sun/security/ssl/MAC.java b/src/share/classes/sun/security/ssl/MAC.java --- a/src/share/classes/sun/security/ssl/MAC.java +++ b/src/share/classes/sun/security/ssl/MAC.java @@ -58,9 +58,6 @@ // Value of the null MAC is fixed private static final byte nullMAC[] = new byte[0]; - // internal identifier for the MAC algorithm - private final MacAlg macAlg; - // stuff defined by the kind of MAC algorithm private final int macSize; @@ -85,7 +82,6 @@ private MAC() { macSize = 0; - macAlg = M_NULL; mac = null; block = null; } @@ -95,7 +91,6 @@ */ MAC(MacAlg macAlg, ProtocolVersion protocolVersion, SecretKey key) throws NoSuchAlgorithmException, InvalidKeyException { - this.macAlg = macAlg; this.macSize = macAlg.size; String algorithm; diff --git a/src/share/classes/sun/security/ssl/ServerHandshaker.java b/src/share/classes/sun/security/ssl/ServerHandshaker.java --- a/src/share/classes/sun/security/ssl/ServerHandshaker.java +++ b/src/share/classes/sun/security/ssl/ServerHandshaker.java @@ -670,9 +670,6 @@ } if (protocolVersion.v >= ProtocolVersion.TLS12.v) { - if (resumingSession) { - handshakeHash.setCertificateVerifyAlg(null); - } handshakeHash.setFinishedAlg(cipherSuite.prfAlg.getPRFHashAlg()); } @@ -882,7 +879,6 @@ throw new SSLHandshakeException( "No supported signature algorithm"); } - handshakeHash.restrictCertificateVerifyAlgs(localHashAlgs); } caCerts = sslContext.getX509TrustManager().getAcceptedIssuers(); @@ -893,10 +889,6 @@ m4.print(System.out); } m4.write(output); - } else { - if (protocolVersion.v >= ProtocolVersion.TLS12.v) { - handshakeHash.setCertificateVerifyAlg(null); - } } /* @@ -1456,8 +1448,6 @@ throw new SSLHandshakeException( "No supported hash algorithm"); } - - handshakeHash.setCertificateVerifyAlg(hashAlg); } try { @@ -1672,11 +1662,6 @@ * not *REQUIRED*, this is an acceptable condition.) */ if (doClientAuth == SSLEngineImpl.clauth_requested) { - // Smart (aka stupid) to forecast that no CertificateVerify - // message will be received. - if (protocolVersion.v >= ProtocolVersion.TLS12.v) { - handshakeHash.setCertificateVerifyAlg(null); - } return; } else { fatalSE(Alerts.alert_bad_certificate, diff --git a/src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java b/src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java --- a/src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java +++ b/src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java @@ -77,9 +77,6 @@ // the hash algorithm private HashAlgorithm hash; - // the signature algorithm - private SignatureAlgorithm signature; - // id in 16 bit MSB format, i.e. 0x0603 for SHA512withECDSA private int id; @@ -96,7 +93,6 @@ private SignatureAndHashAlgorithm(HashAlgorithm hash, SignatureAlgorithm signature, String algorithm, int priority) { this.hash = hash; - this.signature = signature; this.algorithm = algorithm; this.id = ((hash.value & 0xFF) << 8) | (signature.value & 0xFF); this.priority = priority; @@ -105,11 +101,10 @@ // constructor for unsupported algorithm private SignatureAndHashAlgorithm(String algorithm, int id, int sequence) { this.hash = HashAlgorithm.valueOf((id >> 8) & 0xFF); - this.signature = SignatureAlgorithm.valueOf(id & 0xFF); this.algorithm = algorithm; this.id = id; - // add one more to the sequece number, in case that the number is zero + // add one more to the sequence number, in case that the number is zero this.priority = SUPPORTED_ALG_PRIORITY_MAX_NUM + sequence + 1; }
