From 166451ee16414d413fb0f552ed16f0c5ea897a20 Mon Sep 17 00:00:00 2001 From: xuelei Date: Thu, 29 Sep 2011 17:31:30 -0700 Subject: [PATCH] 7064341: jsse/runtime security problem Reviewed-by: wetmore --- .../classes/javax/net/ssl/SSLEngine.java | 4 +- .../sun/security/ssl/AppOutputStream.java | 30 +++++++++++- .../classes/sun/security/ssl/CipherBox.java | 17 ++++++- .../classes/sun/security/ssl/CipherSuite.java | 13 ++++- .../sun/security/ssl/EngineOutputRecord.java | 47 +++++++++++++++++-- .../classes/sun/security/ssl/Record.java | 19 +++++++- .../sun/security/ssl/SSLEngineImpl.java | 34 ++++++++++++++ .../sun/security/ssl/SSLSocketImpl.java | 35 ++++++++++++++ .../ssl/NewAPIs/SSLEngine/CheckStatus.java | 4 +- .../net/ssl/NewAPIs/SSLEngine/LargeBufs.java | 4 +- .../ssl/NewAPIs/SSLEngine/LargePacket.java | 4 +- 11 files changed, 197 insertions(+), 14 deletions(-) diff --git a/src/share/classes/javax/net/ssl/SSLEngine.java b/src/share/classes/javax/net/ssl/SSLEngine.java index 2eea55121..411626cd7 100644 --- a/src/share/classes/javax/net/ssl/SSLEngine.java +++ b/src/share/classes/javax/net/ssl/SSLEngine.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2011, Oracle and/or its affiliates. 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 @@ -538,7 +538,7 @@ public abstract class SSLEngine { * If this SSLEngine has not yet started its initial * handshake, this method will automatically start the handshake. *

- * This method will attempt to produce one SSL/TLS packet, and will + * This method will attempt to produce SSL/TLS records, and will * consume as much source data as possible, but will never consume * more than the sum of the bytes remaining in each buffer. Each * ByteBuffer's position is updated to reflect the diff --git a/src/share/classes/sun/security/ssl/AppOutputStream.java b/src/share/classes/sun/security/ssl/AppOutputStream.java index 34f3be6d6..6be00b8e2 100644 --- a/src/share/classes/sun/security/ssl/AppOutputStream.java +++ b/src/share/classes/sun/security/ssl/AppOutputStream.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1996, 2009, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1996, 2011, Oracle and/or its affiliates. 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 @@ -69,12 +69,38 @@ class AppOutputStream extends OutputStream { // check if the Socket is invalid (error or closed) c.checkWrite(); + /* + * By default, we counter chosen plaintext issues on CBC mode + * ciphersuites in SSLv3/TLS1.0 by sending one byte of application + * data in the first record of every payload, and the rest in + * subsequent record(s). Note that the issues have been solved in + * TLS 1.1 or later. + * + * It is not necessary to split the very first application record of + * a freshly negotiated TLS session, as there is no previous + * application data to guess. To improve compatibility, we will not + * split such records. + * + * This avoids issues in the outbound direction. For a full fix, + * the peer must have similar protections. + */ + boolean isFirstRecordOfThePayload = true; + // Always flush at the end of each application level record. // This lets application synchronize read and write streams // however they like; if we buffered here, they couldn't. try { do { - int howmuch = Math.min(len, r.availableDataBytes()); + int howmuch; + if (isFirstRecordOfThePayload && c.needToSplitPayload()) { + howmuch = Math.min(0x01, r.availableDataBytes()); + } else { + howmuch = Math.min(len, r.availableDataBytes()); + } + + if (isFirstRecordOfThePayload && howmuch != 0) { + isFirstRecordOfThePayload = false; + } // NOTE: *must* call c.writeRecord() even for howmuch == 0 if (howmuch > 0) { diff --git a/src/share/classes/sun/security/ssl/CipherBox.java b/src/share/classes/sun/security/ssl/CipherBox.java index 91f9f3619..22ef091ee 100644 --- a/src/share/classes/sun/security/ssl/CipherBox.java +++ b/src/share/classes/sun/security/ssl/CipherBox.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1996, 2011, Oracle and/or its affiliates. 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 @@ -112,6 +112,11 @@ final class CipherBox { */ private SecureRandom random; + /** + * Is the cipher of CBC mode? + */ + private final boolean isCBCMode; + /** * Fixed masks of various block size, as the initial decryption IVs * for TLS 1.1 or later. @@ -128,6 +133,7 @@ final class CipherBox { private CipherBox() { this.protocolVersion = ProtocolVersion.DEFAULT; this.cipher = null; + this.isCBCMode = false; } /** @@ -148,6 +154,7 @@ final class CipherBox { random = JsseJce.getSecureRandom(); } this.random = random; + this.isCBCMode = bulkCipher.isCBCMode; /* * RFC 4346 recommends two algorithms used to generated the @@ -691,4 +698,12 @@ final class CipherBox { } } + /* + * Does the cipher use CBC mode? + * + * @return true if the cipher use CBC mode, false otherwise. + */ + boolean isCBCMode() { + return isCBCMode; + } } diff --git a/src/share/classes/sun/security/ssl/CipherSuite.java b/src/share/classes/sun/security/ssl/CipherSuite.java index 38734b75b..b87ddb173 100644 --- a/src/share/classes/sun/security/ssl/CipherSuite.java +++ b/src/share/classes/sun/security/ssl/CipherSuite.java @@ -420,10 +420,16 @@ final class CipherSuite implements Comparable { // exportable under 512/40 bit rules final boolean exportable; + // Is the cipher algorithm of Cipher Block Chaining (CBC) mode? + final boolean isCBCMode; + BulkCipher(String transformation, int keySize, int expandedKeySize, int ivSize, boolean allowed) { this.transformation = transformation; - this.algorithm = transformation.split("/")[0]; + String[] splits = transformation.split("/"); + this.algorithm = splits[0]; + this.isCBCMode = + splits.length <= 1 ? false : "CBC".equalsIgnoreCase(splits[1]); this.description = this.algorithm + "/" + (keySize << 3); this.keySize = keySize; this.ivSize = ivSize; @@ -436,7 +442,10 @@ final class CipherSuite implements Comparable { BulkCipher(String transformation, int keySize, int ivSize, boolean allowed) { this.transformation = transformation; - this.algorithm = transformation.split("/")[0]; + String[] splits = transformation.split("/"); + this.algorithm = splits[0]; + this.isCBCMode = + splits.length <= 1 ? false : "CBC".equalsIgnoreCase(splits[1]); this.description = this.algorithm + "/" + (keySize << 3); this.keySize = keySize; this.ivSize = ivSize; diff --git a/src/share/classes/sun/security/ssl/EngineOutputRecord.java b/src/share/classes/sun/security/ssl/EngineOutputRecord.java index 60a428396..707fd04f8 100644 --- a/src/share/classes/sun/security/ssl/EngineOutputRecord.java +++ b/src/share/classes/sun/security/ssl/EngineOutputRecord.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2007, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2011, Oracle and/or its affiliates. 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 @@ -46,6 +46,7 @@ import sun.misc.HexDumpEncoder; */ final class EngineOutputRecord extends OutputRecord { + private SSLEngineImpl engine; private EngineWriter writer; private boolean finishedMsg = false; @@ -62,6 +63,7 @@ final class EngineOutputRecord extends OutputRecord { */ EngineOutputRecord(byte type, SSLEngineImpl engine) { super(type, recordSize(type)); + this.engine = engine; writer = engine.writer; } @@ -227,11 +229,50 @@ final class EngineOutputRecord extends OutputRecord { * implementations are fragile and don't like to see empty * records, so this increases robustness. */ - int length = Math.min(ea.getAppRemaining(), maxDataSize); - if (length == 0) { + if (ea.getAppRemaining() == 0) { return; } + /* + * By default, we counter chosen plaintext issues on CBC mode + * ciphersuites in SSLv3/TLS1.0 by sending one byte of application + * data in the first record of every payload, and the rest in + * subsequent record(s). Note that the issues have been solved in + * TLS 1.1 or later. + * + * It is not necessary to split the very first application record of + * a freshly negotiated TLS session, as there is no previous + * application data to guess. To improve compatibility, we will not + * split such records. + * + * Because of the compatibility, we'd better produce no more than + * SSLSession.getPacketBufferSize() net data for each wrap. As we + * need a one-byte record at first, the 2nd record size should be + * equal to or less than Record.maxDataSizeMinusOneByteRecord. + * + * This avoids issues in the outbound direction. For a full fix, + * the peer must have similar protections. + */ + int length; + if (engine.needToSplitPayload(writeCipher, protocolVersion)) { + write(ea, writeMAC, writeCipher, 0x01); + ea.resetLim(); // reset application data buffer limit + length = Math.min(ea.getAppRemaining(), + maxDataSizeMinusOneByteRecord); + } else { + length = Math.min(ea.getAppRemaining(), maxDataSize); + } + + // Don't bother to really write empty records. + if (length > 0) { + write(ea, writeMAC, writeCipher, length); + } + + return; + } + + void write(EngineArgs ea, MAC writeMAC, CipherBox writeCipher, + int length) throws IOException { /* * Copy out existing buffer values. */ diff --git a/src/share/classes/sun/security/ssl/Record.java b/src/share/classes/sun/security/ssl/Record.java index 1378e107a..92c8a3ebb 100644 --- a/src/share/classes/sun/security/ssl/Record.java +++ b/src/share/classes/sun/security/ssl/Record.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1996, 2011, Oracle and/or its affiliates. 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 @@ -67,6 +67,23 @@ interface Record { + maxPadding // padding + trailerSize; // MAC + static final boolean enableCBCProtection = + Debug.getBooleanProperty("jsse.enableCBCProtection", true); + + /* + * For CBC protection in SSL3/TLS1, we break some plaintext into two + * packets. Max application data size for the second packet. + */ + static final int maxDataSizeMinusOneByteRecord = + maxDataSize // max data size + - ( // max one byte record size + headerSize // header + + maxIVLength // iv + + 1 // one byte data + + maxPadding // padding + + trailerSize // MAC + ); + /* * The maximum large record size. * diff --git a/src/share/classes/sun/security/ssl/SSLEngineImpl.java b/src/share/classes/sun/security/ssl/SSLEngineImpl.java index ce78c10cc..c9df474a4 100644 --- a/src/share/classes/sun/security/ssl/SSLEngineImpl.java +++ b/src/share/classes/sun/security/ssl/SSLEngineImpl.java @@ -311,6 +311,11 @@ final public class SSLEngineImpl extends SSLEngine { private Object unwrapLock; Object writeLock; + /* + * Is it the first application record to write? + */ + private boolean isFirstAppOutputRecord = true; + /* * Class and subclass dynamic debugging support */ @@ -617,6 +622,9 @@ final public class SSLEngineImpl extends SSLEngine { // See comment above. oldCipher.dispose(); + + // reset the flag of the first application record + isFirstAppOutputRecord = true; } /* @@ -1295,9 +1303,35 @@ final public class SSLEngineImpl extends SSLEngine { } } + /* + * turn off the flag of the first application record if we really + * consumed at least byte. + */ + if (isFirstAppOutputRecord && ea.deltaApp() > 0) { + isFirstAppOutputRecord = false; + } + return hsStatus; } + /* + * Need to split the payload except the following cases: + * + * 1. protocol version is TLS 1.1 or later; + * 2. bulk cipher does not use CBC mode, including null bulk cipher suites. + * 3. the payload is the first application record of a freshly + * negotiated TLS session. + * 4. the CBC protection is disabled; + * + * More details, please refer to + * EngineOutputRecord.write(EngineArgs, MAC, CipherBox). + */ + boolean needToSplitPayload(CipherBox cipher, ProtocolVersion protocol) { + return (protocol.v <= ProtocolVersion.TLS10.v) && + cipher.isCBCMode() && !isFirstAppOutputRecord && + Record.enableCBCProtection; + } + /* * Non-application OutputRecords go through here. */ diff --git a/src/share/classes/sun/security/ssl/SSLSocketImpl.java b/src/share/classes/sun/security/ssl/SSLSocketImpl.java index d12eaf1c0..193a45fc3 100644 --- a/src/share/classes/sun/security/ssl/SSLSocketImpl.java +++ b/src/share/classes/sun/security/ssl/SSLSocketImpl.java @@ -371,6 +371,11 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { /* Class and subclass dynamic debugging support */ private static final Debug debug = Debug.getInstance("ssl"); + /* + * Is it the first application record to write? + */ + private boolean isFirstAppOutputRecord = true; + // // CONSTRUCTORS AND INITIALIZATION CODE // @@ -804,8 +809,35 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { if (connectionState < cs_ERROR) { checkSequenceNumber(writeMAC, r.contentType()); } + + // turn off the flag of the first application record + if (isFirstAppOutputRecord && + r.contentType() == Record.ct_application_data) { + isFirstAppOutputRecord = false; + } } + /* + * Need to split the payload except the following cases: + * + * 1. protocol version is TLS 1.1 or later; + * 2. bulk cipher does not use CBC mode, including null bulk cipher suites. + * 3. the payload is the first application record of a freshly + * negotiated TLS session. + * 4. the CBC protection is disabled; + * + * More details, please refer to AppOutputStream.write(byte[], int, int). + */ + boolean needToSplitPayload() { + writeLock.lock(); + try { + return (protocolVersion.v <= ProtocolVersion.TLS10.v) && + writeCipher.isCBCMode() && !isFirstAppOutputRecord && + Record.enableCBCProtection; + } finally { + writeLock.unlock(); + } + } /* * Read an application data record. Alerts and handshake @@ -2034,6 +2066,9 @@ final public class SSLSocketImpl extends BaseSSLSocketImpl { // See comment above. oldCipher.dispose(); + + // reset the flag of the first application record + isFirstAppOutputRecord = true; } /* 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 c29f55a04..dfdefa9f1 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 (c) 2003, 2010, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2011, Oracle and/or its affiliates. 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 @@ -29,6 +29,8 @@ * This is a simple hack to test a bunch of conditions and check * their return codes. * + * @run main/othervm -Djsse.enableCBCProtection=false CheckStatus + * * @author Brad Wetmore */ diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargeBufs.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargeBufs.java index 849e739c8..5960ea639 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargeBufs.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargeBufs.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004, 2006, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2004, 2011, Oracle and/or its affiliates. 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 @@ -30,6 +30,8 @@ * This is to test larger buffer arrays, and make sure the maximum * is being passed. * + * @run main/othervm -Djsse.enableCBCProtection=false LargeBufs + * * @author Brad R. Wetmore */ diff --git a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargePacket.java b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargePacket.java index 4699958dc..a58e19a1a 100644 --- a/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargePacket.java +++ b/test/sun/security/ssl/javax/net/ssl/NewAPIs/SSLEngine/LargePacket.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2006, 2011, Oracle and/or its affiliates. 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 @@ -28,6 +28,8 @@ * @summary Need adjustable TLS max record size for interoperability * with non-compliant * + * @run main/othervm -Djsse.enableCBCProtection=false LargePacket + * * @author Xuelei Fan */ -- GitLab