# HG changeset patch # User ascarpino # Date 1528576707 25200 # Node ID b152d06ed6a9423aa147d716df08b9c003b0cf59 # Parent 2d7e08d730b6cf78698f87025a3d60a1c715f7f2 code review nits and TrasnportContext constructor changes diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/HandshakeContext.java --- a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java Sat Jun 09 13:38:27 2018 -0700 @@ -118,9 +118,6 @@ int clientHelloVersion; String applicationProtocol; - // the preferable signature algorithm used by ServerKeyExchange message - SignatureScheme preferableSignatureAlgorithm; - RandomCookie clientHelloRandom; RandomCookie serverHelloRandom; byte[] certRequestContext; @@ -228,7 +225,7 @@ } // Initialize the non-final class variables. - private void initialize() throws IOException { + private void initialize() { ProtocolVersion inputHelloVersion; ProtocolVersion outputHelloVersion; if (conContext.isNegotiated) { @@ -498,17 +495,6 @@ return activeProtocols.contains(protocolVersion); } - boolean isNegotiable(byte majorVersion, byte minorVersion) { - ProtocolVersion pv = - ProtocolVersion.valueOf(majorVersion, minorVersion); - if (pv == null) { - // unsupported protocol version - return false; - } - - return activeProtocols.contains(pv); - } - /** * Set the active protocol version and propagate it to the SSLSocket * and our handshake streams. Called from ClientHandshaker diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/SSLCipher.java --- a/src/java.base/share/classes/sun/security/ssl/SSLCipher.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/SSLCipher.java Sat Jun 09 13:38:27 2018 -0700 @@ -468,8 +468,7 @@ // shipped with the SunJCE provider. However, AES/256 is unavailable // when the default JCE policy jurisdiction files are installed because // of key length restrictions. - this.isAvailable = - allowed ? isUnlimited(keySize, transformation) : false; + this.isAvailable = allowed && isUnlimited(keySize, transformation); this.readCipherGenerators = readCipherGenerators; this.writeCipherGenerators = writeCipherGenerators; @@ -525,17 +524,6 @@ return null; } - public static final String getDefaultType() { - String prop = AccessController.doPrivileged( - new PrivilegedAction() { - @Override - public String run() { - return Security.getProperty("jdk.tls.KeyLimits"); - } - }); - return prop; - } - /** * Test if this bulk cipher is available. For use by CipherSuite. */ @@ -838,7 +826,7 @@ } if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -931,7 +919,7 @@ } if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -1024,7 +1012,7 @@ if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -1180,7 +1168,7 @@ if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -1296,7 +1284,7 @@ if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -1474,7 +1462,7 @@ if (bb.position() != dup.position()) { throw new RuntimeException( - "Unexpected Bytebuffer position"); + "Unexpected ByteBuffer position"); } } catch (ShortBufferException sbe) { // catch BouncyCastle buffering error @@ -1602,8 +1590,7 @@ // DON'T decrypt the nonce_explicit for AEAD mode. The buffer // position has moved out of the nonce_explicit range. - int len = bb.remaining(); - int pos = bb.position(); + int len, pos = bb.position(); ByteBuffer dup = bb.duplicate(); try { len = cipher.doFinal(dup, bb); @@ -1617,7 +1604,7 @@ throw new RuntimeException("Cipher buffering error in " + "JCE provider " + cipher.getProvider().getName(), sbe); } - // reset the limit to the end of the decryted data + // reset the limit to the end of the decrypted data bb.position(pos); bb.limit(pos + len); @@ -1718,8 +1705,7 @@ bb.put(nonce); // DON'T encrypt the nonce for AEAD mode. - int len = bb.remaining(); - int pos = bb.position(); + int len, pos = bb.position(); if (SSLLogger.isOn && SSLLogger.isOn("plaintext")) { SSLLogger.fine( "Plaintext before ENCRYPTION", @@ -1868,8 +1854,7 @@ contentType, bb.remaining(), sn); cipher.updateAAD(aad); - int len = bb.remaining(); - int pos = bb.position(); + int len, pos = bb.position(); ByteBuffer dup = bb.duplicate(); try { len = cipher.doFinal(dup, bb); @@ -1883,7 +1868,7 @@ throw new RuntimeException("Cipher buffering error in " + "JCE provider " + cipher.getProvider().getName(), sbe); } - // reset the limit to the end of the decryted data + // reset the limit to the end of the decrypted data bb.position(pos); bb.limit(pos + len); @@ -2000,8 +1985,7 @@ contentType, outputSize, sn); cipher.updateAAD(aad); - int len = bb.remaining(); - int pos = bb.position(); + int len, pos = bb.position(); if (SSLLogger.isOn && SSLLogger.isOn("plaintext")) { SSLLogger.fine( "Plaintext before ENCRYPTION", @@ -2205,8 +2189,8 @@ // The caller ensures there are enough bytes available in the buffer. // So we won't need to check the remaining of the buffer. - for (int i = 0; i < tag.length; i++) { - if (bb.get() != tag[i]) { + for (byte t : tag) { + if (bb.get() != t) { results[0]++; // mismatched bytes } else { results[1]++; // matched bytes diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/SSLHandshake.java --- a/src/java.base/share/classes/sun/security/ssl/SSLHandshake.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/SSLHandshake.java Sat Jun 09 13:38:27 2018 -0700 @@ -466,18 +466,6 @@ return name; } - /* - static SSLHandshake valueOf(byte id) { - for (SSLHandshake hs : SSLHandshake.values()) { - if (hs.id == id) { - return hs; - } - } - - return null; - } - */ - static String nameOf(byte id) { // If two handshake message share the same handshake type, returns // the first handshake message name. diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java --- a/src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/ServerHandshakeContext.java Sat Jun 09 13:38:27 2018 -0700 @@ -29,16 +29,15 @@ import java.security.AlgorithmConstraints; import java.security.AccessController; import sun.security.util.LegacyAlgorithmConstraints; -import sun.security.action.GetPropertyAction; import sun.security.action.GetLongAction; class ServerHandshakeContext extends HandshakeContext { // To prevent the TLS renegotiation issues, by setting system property // "jdk.tls.rejectClientInitiatedRenegotiation" to true, applications in - // server side can disable all client initiated SSL renegotiations + // server side can disable all client initiated SSL renegotiation // regardless of the support of TLS protocols. // - // By default, allow client initiated renegotiations. + // By default, allow client initiated renegotiation. static final boolean rejectClientInitiatedRenego = Utilities.getBooleanProperty( "jdk.tls.rejectClientInitiatedRenegotiation", false); diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/ServerHello.java --- a/src/java.base/share/classes/sun/security/ssl/ServerHello.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/ServerHello.java Sat Jun 09 13:38:27 2018 -0700 @@ -48,7 +48,7 @@ import sun.security.ssl.SupportedVersionsExtension.SHSupportedVersionsSpec; /** - * Pack of the ServertHello/HelloRetryRequest handshake message. + * Pack of the ServerHello/HelloRetryRequest handshake message. */ final class ServerHello { static final SSLConsumer handshakeConsumer = @@ -79,7 +79,7 @@ new T13HelloRetryRequestConsumer(); /** - * The ServertHello handshake message. + * The ServerHello handshake message. */ static final class ServerHelloMessage extends HandshakeMessage { final ProtocolVersion serverVersion; // TLS 1.3 legacy @@ -115,7 +115,7 @@ this.clientHello = clientHello; // The handshakeRecord field is used for HelloRetryRequest consumer - // only. It's fine to set it to null for gnerating side of the + // only. It's fine to set it to null for generating side of the // ServerHello/HelloRetryRequest message. this.handshakeRecord = null; } @@ -131,7 +131,7 @@ byte minor = m.get(); this.serverVersion = ProtocolVersion.valueOf(major, minor); if (this.serverVersion == null) { - // The client should only request for known protovol versions. + // The client should only request for known protocol versions. context.conContext.fatal(Alert.PROTOCOL_VERSION, "Unsupported protocol version: " + ProtocolVersion.nameOf(major, minor)); @@ -390,18 +390,18 @@ private static KeyExchangeProperties chooseCipherSuite( ServerHandshakeContext shc, ClientHelloMessage clientHello) throws IOException { - List prefered; + List preferred; List proposed; if (shc.sslConfig.preferLocalCipherSuites) { - prefered = shc.activeCipherSuites; + preferred = shc.activeCipherSuites; proposed = clientHello.cipherSuites; } else { - prefered = clientHello.cipherSuites; + preferred = clientHello.cipherSuites; proposed = shc.activeCipherSuites; } List legacySuites = new LinkedList<>(); - for (CipherSuite cs : prefered) { + for (CipherSuite cs : preferred) { if (!HandshakeContext.isNegotiable( proposed, shc.negotiatedProtocol, cs)) { continue; @@ -675,20 +675,20 @@ private static CipherSuite chooseCipherSuite( ServerHandshakeContext shc, ClientHelloMessage clientHello) throws IOException { - List prefered; + List preferred; List proposed; if (shc.sslConfig.preferLocalCipherSuites) { - prefered = shc.activeCipherSuites; + preferred = shc.activeCipherSuites; proposed = clientHello.cipherSuites; } else { - prefered = clientHello.cipherSuites; + preferred = clientHello.cipherSuites; proposed = shc.activeCipherSuites; } CipherSuite legacySuite = null; AlgorithmConstraints legacyConstraints = ServerHandshakeContext.legacyAlgorithmConstraints; - for (CipherSuite cs : prefered) { + for (CipherSuite cs : preferred) { if (!HandshakeContext.isNegotiable( proposed, shc.negotiatedProtocol, cs)) { continue; @@ -855,7 +855,6 @@ "No more message expected before ServerHello is processed"); } - int startPos = message.position(); ServerHelloMessage shm = new ServerHelloMessage(chc, message); if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) { SSLLogger.fine("Consuming ServerHello handshake message", shm); @@ -960,7 +959,7 @@ if (serverHello.serverRandom.isVersionDowngrade(chc)) { chc.conContext.fatal(Alert.ILLEGAL_PARAMETER, - "A potential protocol versoin downgrade attack"); + "A potential protocol version downgrade attack"); } // Consume the handshake message for the specific protocol version. diff -r 2d7e08d730b6 -r b152d06ed6a9 src/java.base/share/classes/sun/security/ssl/TransportContext.java --- a/src/java.base/share/classes/sun/security/ssl/TransportContext.java Sat Jun 09 08:08:12 2018 -0700 +++ b/src/java.base/share/classes/sun/security/ssl/TransportContext.java Sat Jun 09 13:38:27 2018 -0700 @@ -35,7 +35,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import javax.crypto.SecretKey; import javax.net.ssl.HandshakeCompletedEvent; import javax.net.ssl.HandshakeCompletedListener; import javax.net.ssl.SSLEngineResult.HandshakeStatus; @@ -60,109 +59,82 @@ // connection status boolean isUnsureMode; - boolean isNegotiated; - boolean isBroken; - boolean isInputCloseNotified; - boolean isOutputCloseNotified; - Exception closeReason; + boolean isNegotiated = false; + boolean isBroken = false; + boolean isInputCloseNotified = false; + boolean isOutputCloseNotified = false; + Exception closeReason = null; // negotiated security parameters SSLSessionImpl conSession; ProtocolVersion protocolVersion; - String applicationProtocol; + String applicationProtocol= null; // handshake context - HandshakeContext handshakeContext; + HandshakeContext handshakeContext = null; // connection reserved status for handshake. - boolean secureRenegotiation; + boolean secureRenegotiation = false; byte[] clientVerifyData; byte[] serverVerifyData; // connection sensitive configuration List serverRequestedNamedGroups; - SecretKey baseWriteSecret, baseReadSecret; CipherSuite cipherSuite; + private static final byte[] emptyByteArray = new byte[0]; // Please never use the transport parameter other than storing a // reference to this object. + // Called by SSLEngineImpl TransportContext(SSLContextImpl sslContext, SSLTransport transport, InputRecord inputRecord, OutputRecord outputRecord) { - this.transport = transport; - this.sslContext = sslContext; - this.inputRecord = inputRecord; - this.outputRecord = outputRecord; - this.sslConfig = new SSLConfiguration(sslContext, true); - this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize(); - this.isUnsureMode = true; - - initialize(); - - this.acc = AccessController.getContext(); - this.consumers = new HashMap<>(); + this(sslContext, transport, new SSLConfiguration(sslContext, true), + inputRecord, outputRecord, true); } // Please never use the transport parameter other than storing a // reference to this object. + // Called by SSLSocketImpl TransportContext(SSLContextImpl sslContext, SSLTransport transport, InputRecord inputRecord, OutputRecord outputRecord, boolean isClientMode) { + this(sslContext, transport, + new SSLConfiguration(sslContext, isClientMode), + inputRecord, outputRecord,false); + } + + // Please never use the transport parameter other than storing a + // reference to this object. + // Called by SSLSocketImpl with an existing SSLConfig + TransportContext(SSLContextImpl sslContext, SSLTransport transport, + SSLConfiguration sslConfig, + InputRecord inputRecord, OutputRecord outputRecord) { + this(sslContext, transport, (SSLConfiguration)sslConfig.clone(), + inputRecord, outputRecord, false); + } + + private TransportContext(SSLContextImpl sslContext, SSLTransport transport, + SSLConfiguration sslConfig, InputRecord inputRecord, + OutputRecord outputRecord, boolean isUnsureMode) { this.transport = transport; this.sslContext = sslContext; this.inputRecord = inputRecord; this.outputRecord = outputRecord; - this.sslConfig = new SSLConfiguration(sslContext, isClientMode); - this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize(); - this.isUnsureMode = false; - - initialize(); - - this.acc = AccessController.getContext(); - this.consumers = new HashMap<>(); - } - - // Please never use the transport parameter other than storing a - // reference to this object. - TransportContext(SSLContextImpl sslContext, SSLTransport transport, - SSLConfiguration sslConfig, - InputRecord inputRecord, OutputRecord outputRecord) { - this.transport = transport; - this.sslContext = sslContext; - this.inputRecord = inputRecord; - this.outputRecord = outputRecord; - this.sslConfig = (SSLConfiguration)sslConfig.clone(); + this.sslConfig = sslConfig; if (this.sslConfig.maximumPacketSize == 0) { this.sslConfig.maximumPacketSize = outputRecord.getMaxPacketSize(); } - this.isUnsureMode = false; - - initialize(); + this.isUnsureMode = isUnsureMode; - this.acc = AccessController.getContext(); - this.consumers = new HashMap<>(); - } - - // Initialize the non-final class variables. - private void initialize() { // initial security parameters this.conSession = SSLSessionImpl.nullSession; this.protocolVersion = this.sslConfig.maximumProtocolVersion; - this.applicationProtocol = null; - - // initial handshake context - this.handshakeContext = null; + this.clientVerifyData = emptyByteArray; + this.serverVerifyData = emptyByteArray; - // initial security parameters for secure renegotiation - this.secureRenegotiation = false; - this.clientVerifyData = new byte[0]; - this.serverVerifyData = new byte[0]; - - this.isNegotiated = false; - this.isBroken = false; - this.isInputCloseNotified = false; - this.isOutputCloseNotified = false; - this.closeReason = null; + this.acc = AccessController.getContext(); + this.consumers = new HashMap<>(); } // Dispatch plaintext to a specific consumer. @@ -317,7 +289,7 @@ } else { // unlikely, but just in case. if (SSLLogger.isOn && SSLLogger.isOn("ssl")) { SSLLogger.warning( - "Closed transport, rethrowing (unexpected)", cause); + "Closed transport, unexpected rethrowing", cause); } throw alert.createSSLException("Unexpected rethrowing", cause); } @@ -390,7 +362,7 @@ outputRecord.close(); } catch (IOException ioe) { if (SSLLogger.isOn && SSLLogger.isOn("ssl")) { - SSLLogger.warning("Fatal: ouput record closure failed", ioe); + SSLLogger.warning("Fatal: output record closure failed", ioe); } } @@ -473,7 +445,7 @@ } } - void closeInbound() throws SSLException { + void closeInbound() { if (isInboundDone()) { return; } @@ -512,11 +484,6 @@ inputRecord.close(); } - // For TLS 1.3, output closure is independent from input closure. -// if (isNegotiated && protocolVersion.useTLS13PlusSpec()) { -// return; -// } - // For TLS 1.2 and prior version, it is required to respond with // a close_notify alert of its own and close down the connection // immediately, discarding any pending writes. @@ -579,13 +546,6 @@ } } - // For TLS 1.3, output closure is independent from input closure. -// -// if (isNegotiated && protocolVersion.useTLS13PlusSpec()) { -// return; -// } -// - // It is not required for the initiator of the close to wait for the // responding close_notify alert before closing the read side of the // connection. However, if the application protocol using TLS @@ -641,8 +601,6 @@ } handshakeContext = null; - // inputRecord and outputRecord shares the same handshakeHash - // inputRecord.handshakeHash.finish(); outputRecord.handshakeHash.finish(); inputRecord.finishHandshake(); outputRecord.finishHandshake(); @@ -670,7 +628,7 @@ handshakeContext = null; // Note: May need trigger handshake completion even for post-handshake - // authenticiation in the future. + // authentication in the future. return HandshakeStatus.FINISHED; }