From 8c4a771b6f09869e8b7c9e5b739119dee76788ac Mon Sep 17 00:00:00 2001 From: xuelei Date: Mon, 7 Dec 2009 21:16:41 -0800 Subject: [PATCH] 6898739: TLS renegotiation issue Summary: the interim fix disables TLS/SSL renegotiation Reviewed-by: mullan, chegar, wetmore --- .../sun/security/ssl/ClientHandshaker.java | 45 ++++++++++++++-- .../classes/sun/security/ssl/Handshaker.java | 19 +++++-- .../sun/security/ssl/SSLEngineImpl.java | 23 +++++--- .../sun/security/ssl/SSLSocketImpl.java | 22 ++++++-- .../sun/security/ssl/ServerHandshaker.java | 54 ++++++++++++++++++- .../InvalidateServerSessionRenegotiate.java | 4 +- .../net/ssl/NewAPIs/JSSERenegotiate.java | 4 +- .../ssl/NewAPIs/SSLEngine/CheckStatus.java | 4 +- .../ssl/NewAPIs/SSLEngine/ConnectionTest.java | 4 +- .../NewAPIs/SSLEngine/NoAuthClientAuth.java | 4 +- 10 files changed, 159 insertions(+), 24 deletions(-) diff --git a/src/share/classes/sun/security/ssl/ClientHandshaker.java b/src/share/classes/sun/security/ssl/ClientHandshaker.java index 11f2da32c..24db26e80 100644 --- a/src/share/classes/sun/security/ssl/ClientHandshaker.java +++ b/src/share/classes/sun/security/ssl/ClientHandshaker.java @@ -93,13 +93,17 @@ final class ClientHandshaker extends Handshaker { * Constructors */ ClientHandshaker(SSLSocketImpl socket, SSLContextImpl context, - ProtocolList enabledProtocols) { + ProtocolList enabledProtocols, + ProtocolVersion activeProtocolVersion) { super(socket, context, enabledProtocols, true, true); + this.activeProtocolVersion = activeProtocolVersion; } ClientHandshaker(SSLEngineImpl engine, SSLContextImpl context, - ProtocolList enabledProtocols) { + ProtocolList enabledProtocols, + ProtocolVersion activeProtocolVersion) { super(engine, context, enabledProtocols, true, true); + this.activeProtocolVersion = activeProtocolVersion; } /* @@ -275,7 +279,42 @@ final class ClientHandshaker extends Handshaker { // sent the "client hello" but the server's not seen it. // if (state < HandshakeMessage.ht_client_hello) { - kickstart(); + if (!renegotiable) { // renegotiation is not allowed. + if (activeProtocolVersion.v >= ProtocolVersion.TLS10.v) { + // response with a no_negotiation warning, + warningSE(Alerts.alert_no_negotiation); + + // invalidate the handshake so that the caller can + // dispose this object. + invalidated = true; + + // If there is still unread block in the handshake + // input stream, it would be truncated with the disposal + // and the next handshake message will become incomplete. + // + // However, according to SSL/TLS specifications, no more + // handshake message could immediately follow ClientHello + // or HelloRequest. But in case of any improper messages, + // we'd better check to ensure there is no remaining bytes + // in the handshake input stream. + if (input.available() > 0) { + fatalSE(Alerts.alert_unexpected_message, + "HelloRequest followed by an unexpected " + + "handshake message"); + } + + } else { + // For SSLv3, send the handshake_failure fatal error. + // Note that SSLv3 does not define a no_negotiation alert + // like TLSv1. However we cannot ignore the message + // simply, otherwise the other side was waiting for a + // response that would never come. + fatalSE(Alerts.alert_handshake_failure, + "renegotiation is not allowed"); + } + } else { + kickstart(); + } } } diff --git a/src/share/classes/sun/security/ssl/Handshaker.java b/src/share/classes/sun/security/ssl/Handshaker.java index a556d4e99..7d5ead0d0 100644 --- a/src/share/classes/sun/security/ssl/Handshaker.java +++ b/src/share/classes/sun/security/ssl/Handshaker.java @@ -1,5 +1,5 @@ /* - * Copyright 1996-2008 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 1996-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -60,9 +60,12 @@ import sun.security.ssl.CipherSuite.*; */ abstract class Handshaker { - // current protocol version + // protocol version being established using this Handshaker ProtocolVersion protocolVersion; + // the currently active protocol version during a renegotiation + ProtocolVersion activeProtocolVersion; + // list of enabled protocols ProtocolList enabledProtocols; @@ -124,6 +127,13 @@ abstract class Handshaker { /* Class and subclass dynamic debugging support */ static final Debug debug = Debug.getInstance("ssl"); + // By default, disable the unsafe legacy session renegotiation + static final boolean renegotiable = Debug.getBooleanProperty( + "sun.security.ssl.allowUnsafeRenegotiation", false); + + // need to dispose the object when it is invalidated + boolean invalidated; + Handshaker(SSLSocketImpl c, SSLContextImpl context, ProtocolList enabledProtocols, boolean needCertVerify, boolean isClient) { @@ -144,6 +154,7 @@ abstract class Handshaker { this.sslContext = context; this.isClient = isClient; enableNewSession = true; + invalidated = false; setCipherSuite(CipherSuite.C_NULL); @@ -489,7 +500,9 @@ abstract class Handshaker { */ void processLoop() throws IOException { - while (input.available() > 0) { + // need to read off 4 bytes at least to get the handshake + // message type and length. + while (input.available() >= 4) { byte messageType; int messageLen; diff --git a/src/share/classes/sun/security/ssl/SSLEngineImpl.java b/src/share/classes/sun/security/ssl/SSLEngineImpl.java index 72291d54b..98afa6c26 100644 --- a/src/share/classes/sun/security/ssl/SSLEngineImpl.java +++ b/src/share/classes/sun/security/ssl/SSLEngineImpl.java @@ -1,5 +1,5 @@ /* - * Copyright 2003-2008 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2003-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -433,11 +433,12 @@ final public class SSLEngineImpl extends SSLEngine { connectionState = cs_RENEGOTIATE; } if (roleIsServer) { - handshaker = new ServerHandshaker - (this, sslContext, enabledProtocols, doClientAuth); + handshaker = new ServerHandshaker(this, sslContext, + enabledProtocols, doClientAuth, + connectionState == cs_RENEGOTIATE, protocolVersion); } else { - handshaker = new ClientHandshaker - (this, sslContext, enabledProtocols); + handshaker = new ClientHandshaker(this, sslContext, + enabledProtocols, protocolVersion); } handshaker.enabledCipherSuites = enabledCipherSuites; handshaker.setEnableSessionCreation(enableSessionCreation); @@ -639,6 +640,10 @@ final public class SSLEngineImpl extends SSLEngine { break; case cs_DATA: + if (!Handshaker.renegotiable) { + throw new SSLHandshakeException("renegotiation is not allowed"); + } + // initialize the handshaker, move to cs_RENEGOTIATE initHandshaker(); break; @@ -966,7 +971,13 @@ final public class SSLEngineImpl extends SSLEngine { handshaker.process_record(inputRecord, expectingFinished); expectingFinished = false; - if (handshaker.isDone()) { + if (handshaker.invalidated) { + handshaker = null; + // if state is cs_RENEGOTIATE, revert it to cs_DATA + if (connectionState == cs_RENEGOTIATE) { + connectionState = cs_DATA; + } + } else if (handshaker.isDone()) { sess = handshaker.getSession(); if (!writer.hasOutboundData()) { hsStatus = HandshakeStatus.FINISHED; diff --git a/src/share/classes/sun/security/ssl/SSLSocketImpl.java b/src/share/classes/sun/security/ssl/SSLSocketImpl.java index 820954f96..0f1b683ce 100644 --- a/src/share/classes/sun/security/ssl/SSLSocketImpl.java +++ b/src/share/classes/sun/security/ssl/SSLSocketImpl.java @@ -907,7 +907,13 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { handshaker.process_record(r, expectingFinished); expectingFinished = false; - if (handshaker.isDone()) { + if (handshaker.invalidated) { + handshaker = null; + // if state is cs_RENEGOTIATE, revert it to cs_DATA + if (connectionState == cs_RENEGOTIATE) { + connectionState = cs_DATA; + } + } else if (handshaker.isDone()) { sess = handshaker.getSession(); handshaker = null; connectionState = cs_DATA; @@ -925,6 +931,7 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { t.start(); } } + if (needAppData || connectionState != cs_DATA) { continue; } else { @@ -1083,11 +1090,12 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { connectionState = cs_RENEGOTIATE; } if (roleIsServer) { - handshaker = new ServerHandshaker - (this, sslContext, enabledProtocols, doClientAuth); + handshaker = new ServerHandshaker(this, sslContext, + enabledProtocols, doClientAuth, + connectionState == cs_RENEGOTIATE, protocolVersion); } else { - handshaker = new ClientHandshaker - (this, sslContext, enabledProtocols); + handshaker = new ClientHandshaker(this, sslContext, + enabledProtocols, protocolVersion); } handshaker.enabledCipherSuites = enabledCipherSuites; handshaker.setEnableSessionCreation(enableSessionCreation); @@ -1192,6 +1200,10 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { break; case cs_DATA: + if (!Handshaker.renegotiable) { + throw new SSLHandshakeException("renegotiation is not allowed"); + } + // initialize the handshaker, move to cs_RENEGOTIATE initHandshaker(); break; diff --git a/src/share/classes/sun/security/ssl/ServerHandshaker.java b/src/share/classes/sun/security/ssl/ServerHandshaker.java index 594db0e74..94ef02451 100644 --- a/src/share/classes/sun/security/ssl/ServerHandshaker.java +++ b/src/share/classes/sun/security/ssl/ServerHandshaker.java @@ -69,6 +69,9 @@ final class ServerHandshaker extends Handshaker { // flag to check for clientCertificateVerify message private boolean needClientVerify = false; + // indicate a renegotiation handshaking + private boolean isRenegotiation = false; + /* * For exportable ciphersuites using non-exportable key sizes, we use * ephemeral RSA keys. We could also do anonymous RSA in the same way @@ -96,20 +99,28 @@ final class ServerHandshaker extends Handshaker { * Constructor ... use the keys found in the auth context. */ ServerHandshaker(SSLSocketImpl socket, SSLContextImpl context, - ProtocolList enabledProtocols, byte clientAuth) { + ProtocolList enabledProtocols, byte clientAuth, + boolean isRenegotiation, ProtocolVersion activeProtocolVersion) { + super(socket, context, enabledProtocols, (clientAuth != SSLEngineImpl.clauth_none), false); doClientAuth = clientAuth; + this.isRenegotiation = isRenegotiation; + this.activeProtocolVersion = activeProtocolVersion; } /* * Constructor ... use the keys found in the auth context. */ ServerHandshaker(SSLEngineImpl engine, SSLContextImpl context, - ProtocolList enabledProtocols, byte clientAuth) { + ProtocolList enabledProtocols, byte clientAuth, + boolean isRenegotiation, ProtocolVersion activeProtocolVersion) { + super(engine, context, enabledProtocols, (clientAuth != SSLEngineImpl.clauth_none), false); doClientAuth = clientAuth; + this.isRenegotiation = isRenegotiation; + this.activeProtocolVersion = activeProtocolVersion; } /* @@ -257,6 +268,45 @@ final class ServerHandshaker extends Handshaker { if (debug != null && Debug.isOn("handshake")) { mesg.print(System.out); } + + // if it is a renegotiation request and renegotiation is not allowed + if (isRenegotiation && !renegotiable) { + if (activeProtocolVersion.v >= ProtocolVersion.TLS10.v) { + // response with a no_negotiation warning, + warningSE(Alerts.alert_no_negotiation); + + // invalidate the handshake so that the caller can + // dispose this object. + invalidated = true; + + // If there is still unread block in the handshake + // input stream, it would be truncated with the disposal + // and the next handshake message will become incomplete. + // + // However, according to SSL/TLS specifications, no more + // handshake message could immediately follow ClientHello + // or HelloRequest. But in case of any improper messages, + // we'd better check to ensure there is no remaining bytes + // in the handshake input stream. + if (input.available() > 0) { + fatalSE(Alerts.alert_unexpected_message, + "ClientHello followed by an unexpected " + + "handshake message"); + + } + + return; + } else { + // For SSLv3, send the handshake_failure fatal error. + // Note that SSLv3 does not define a no_negotiation alert + // like TLSv1. However we cannot ignore the message + // simply, otherwise the other side was waiting for a + // response that would never come. + fatalSE(Alerts.alert_handshake_failure, + "renegotiation is not allowed"); + } + } + /* * Always make sure this entire record has been digested before we * start emitting output, to ensure correct digesting order. diff --git a/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/InvalidateServerSessionRenegotiate.java b/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/InvalidateServerSessionRenegotiate.java index ab8ac46e4..1320c8b59 100644 --- a/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/InvalidateServerSessionRenegotiate.java +++ b/test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/InvalidateServerSessionRenegotiate.java @@ -1,5 +1,5 @@ /* - * Copyright 2001-2003 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2001-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -25,6 +25,8 @@ * @test * @bug 4403428 * @summary Invalidating JSSE session on server causes SSLProtocolException + * @ignore incompatible with disabled unsafe renegotiation (6898739), please + * reenable when safe renegotiation is implemented. * @author Brad Wetmore */ diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/JSSERenegotiate.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/JSSERenegotiate.java index 442c897b0..e735ecc94 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/JSSERenegotiate.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/JSSERenegotiate.java @@ -1,5 +1,5 @@ /* - * Copyright 2001-2007 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2001-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -26,6 +26,8 @@ * @bug 4280338 * @summary "Unsupported SSL message version" SSLProtocolException * w/SSL_RSA_WITH_NULL_MD5 + * @ignore incompatible with disabled unsafe renegotiation (6898739), please + * reenable when safe renegotiation is implemented. * * @author Ram Marti * @author Brad Wetmore diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/CheckStatus.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/CheckStatus.java index 76c5a0d2f..6c923ab48 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/CheckStatus.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/CheckStatus.java @@ -1,5 +1,5 @@ /* - * Copyright 2003-2004 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2003-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -25,6 +25,8 @@ * @test * @bug 4948079 * @summary SSLEngineResult needs updating [none yet] + * @ignore incompatible with disabled unsafe renegotiation (6898739), please + * reenable when safe renegotiation is implemented. * * This is a simple hack to test a bunch of conditions and check * their return codes. diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/ConnectionTest.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/ConnectionTest.java index a1229d403..cca2865f8 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/ConnectionTest.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/ConnectionTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2003-2004 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2003-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -26,6 +26,8 @@ * @bug 4495742 * @summary Add non-blocking SSL/TLS functionality, usable with any * I/O abstraction + * @ignore incompatible with disabled unsafe renegotiation (6898739), please + * reenable when safe renegotiation is implemented. * * This is a bit hacky, meant to test various conditions. The main * thing I wanted to do with this was to do buffer reads/writes diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/NoAuthClientAuth.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/NoAuthClientAuth.java index 6ea457ba9..c49a2cb7a 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/NoAuthClientAuth.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/NoAuthClientAuth.java @@ -1,5 +1,5 @@ /* - * Copyright 2006 Sun Microsystems, Inc. All Rights Reserved. + * Copyright 2003-2009 Sun Microsystems, Inc. All Rights Reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -25,6 +25,8 @@ * @test * @bug 4495742 * @summary Demonstrate SSLEngine switch from no client auth to client auth. + * @ignore incompatible with disabled unsafe renegotiation (6898739), please + * reenable when safe renegotiation is implemented. * * @author Brad R. Wetmore */ -- GitLab