Skip to content

JSSE: fix SSLEngine.unwrap() data delivery and close_notify handling after closeOutbound() #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cconlon wants to merge 1 commit into
base: master
Choose a base branch
from
+235 −3
Open

JSSE: fix SSLEngine.unwrap() data delivery and close_notify handling after closeOutbound() #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/java/com/wolfssl/provider/jsse/WolfSSLEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1528,7 +1528,14 @@ else if (hs == SSLEngineResult.HandshakeStatus.NEED_WRAP &&
checkAndInitSSLEngine();
}

if (outBoundOpen == false) {
/* Only route unwrap() into ClosingConnection() if inbound side
* is already closed or the handshake not finished. If outbound
* has been closed but inbound is still open, fall through to
* normal receive path which delivers buffered/incoming app
* data to app and processes the peer's incoming close_notify
* alert via RecvAppData(). */
if (outBoundOpen == false &&
(inBoundOpen == false || !this.handshakeFinished)) {
try {
if (ClosingConnection() == WolfSSL.SSL_SUCCESS) {
/* Mark SSLEngine status as CLOSED */
Expand Down Expand Up @@ -1708,8 +1715,14 @@ else if (inRemaining > 0 &&
}
} /* end DoHandshake() / RecvAppData() */

if (outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) {
/* Mark status CLOSED when closing down, but do not
* overwrite BUFFER_OVERFLOW/BUFFER_UNDERFLOW. App needs
* those to enlarge the output buffer or provide more input
* data, otherwise pending app data may not be delivered. */
if ((outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) &&
(status != SSLEngineResult.Status.BUFFER_OVERFLOW) &&
(status != SSLEngineResult.Status.BUFFER_UNDERFLOW)) {
/* Mark SSLEngine status as CLOSED */
status = SSLEngineResult.Status.CLOSED;
/* Handshake has finished and SSLEngine is closed,
Expand Down
219 changes: 219 additions & 0 deletions src/test/com/wolfssl/provider/jsse/test/WolfSSLEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2792,6 +2792,225 @@ public void testCloseNotifyTLS13HandshakeStatus()

}

@Test
public void testCloseOutboundWithPendingAppData()
throws NoSuchProviderException, NoSuchAlgorithmException,
KeyManagementException, KeyStoreException, CertificateException,
IOException, UnrecoverableKeyException {

/* After an app calls closeOutbound() on a SSLEngine, unwrap() must
* continue to deliver incoming peer application data to the app and
* process the peer's close_notify alert so inbound closes. Native
* wolfSSL_shutdown() will not process incoming records while
* decrypted application data is still buffered internally, so unwrap()
* needs to drain application data itself instead of relying on
* wolfSSL_shutdown() to consume incoming records. Mirrors the close
* sequence of the SunJSSE interop test SSLEngine/NoAuthClientAuth.java,
* which spins forever on wrap()/unwrap() if the peer's close_notify is
* never processed. */

for (int i = 0; i < enabledProtocols.size(); i++) {

/* Skip DTLS, testing in-order TLS close sequence here */
if (enabledProtocols.get(i).startsWith("DTLS")) {
continue;
}

this.ctx = tf.createSSLContext(
enabledProtocols.get(i), engineProvider);

SSLEngine server = this.ctx.createSSLEngine();
SSLEngine client = this.ctx.createSSLEngine(
"wolfSSL close pending data test", 11111);

server.setUseClientMode(false);
server.setNeedClientAuth(false);
client.setUseClientMode(true);

int ret = tf.testConnection(server, client, null, null,
"close pending data test");
if (ret != 0) {
fail("failed to create connection: " +
enabledProtocols.get(i));
}

String msg = "client data sent after server closeOutbound";
ByteBuffer clientOut = ByteBuffer.wrap(msg.getBytes());
ByteBuffer empty = ByteBuffer.allocate(0);
ByteBuffer cliToSer = ByteBuffer.allocateDirect(
client.getSession().getPacketBufferSize());
ByteBuffer serToCli = ByteBuffer.allocateDirect(
server.getSession().getPacketBufferSize());
ByteBuffer serPlain = ByteBuffer.allocate(
server.getSession().getApplicationBufferSize());
ByteBuffer cliPlain = ByteBuffer.allocate(
client.getSession().getApplicationBufferSize());

/* Server closes outbound while client app data is still in flight.
* Drive both engines like an app would until both report closed,
* bounded by maxLoops to catch a close seq that never completes. */
server.closeOutbound();

int loops = 0;
int maxLoops = 20;
while ((!server.isInboundDone() || !server.isOutboundDone() ||
!client.isInboundDone() || !client.isOutboundDone()) &&
(loops++ < maxLoops)) {

client.wrap(clientOut, cliToSer);
server.wrap(empty, serToCli);

cliToSer.flip();
serToCli.flip();

if (!client.isInboundDone()) {
client.unwrap(serToCli, cliPlain);
}
if (!server.isInboundDone()) {
server.unwrap(cliToSer, serPlain);
}

cliToSer.compact();
serToCli.compact();
}

if (!server.isInboundDone() || !server.isOutboundDone() ||
!client.isInboundDone() || !client.isOutboundDone()) {
fail("SSLEngines failed to close within " + maxLoops +
" wrap/unwrap iterations after closeOutbound(), " +
"peer close_notify possibly not processed: " +
enabledProtocols.get(i));
}

/* Application data sent by peer after local closeOutbound()
* must be delivered by unwrap(), not discarded */
serPlain.flip();
byte[] received = new byte[serPlain.remaining()];
serPlain.get(received);
assertEquals("server did not receive application data sent " +
"by client during connection close: " +
enabledProtocols.get(i), msg, new String(received));
}
}

/**
* unwrap() called after closeOutbound() with an output buffer too
* small for pending peer app data must return BUFFER_OVERFLOW so app
* knows to enlarge its buffer. Data must then be delivered when
* unwrap() is called again with a large enough buffer.
*/
@Test
public void testCloseOutboundUnwrapBufferOverflow()
throws NoSuchProviderException, NoSuchAlgorithmException,
KeyManagementException, KeyStoreException, CertificateException,
IOException, UnrecoverableKeyException {

for (int i = 0; i < enabledProtocols.size(); i++) {

/* Skip DTLS, testing in-order TLS close sequence here */
if (enabledProtocols.get(i).startsWith("DTLS")) {
continue;
}

this.ctx = tf.createSSLContext(enabledProtocols.get(i),
engineProvider);

SSLEngine server = this.ctx.createSSLEngine();
SSLEngine client = this.ctx.createSSLEngine(
"wolfSSL close overflow test", 11111);

server.setUseClientMode(false);
server.setNeedClientAuth(false);
client.setUseClientMode(true);

int ret = tf.testConnection(server, client, null, null,
"close overflow test");
if (ret != 0) {
fail("failed to create connection: " + enabledProtocols.get(i));
}

String msg = "client data sent after server closeOutbound";
ByteBuffer clientOut = ByteBuffer.wrap(msg.getBytes());
ByteBuffer empty = ByteBuffer.allocate(0);
ByteBuffer cliToSer = ByteBuffer.allocateDirect(
client.getSession().getPacketBufferSize());
ByteBuffer serToCli = ByteBuffer.allocateDirect(
server.getSession().getPacketBufferSize());
ByteBuffer tooSmall = ByteBuffer.allocate(1);
ByteBuffer serPlain = ByteBuffer.allocate(
server.getSession().getApplicationBufferSize());
ByteBuffer cliPlain = ByteBuffer.allocate(
client.getSession().getApplicationBufferSize());

server.closeOutbound();

/* Client sends application data after server closed outbound */
SSLEngineResult result = client.wrap(clientOut, cliToSer);
if (result.bytesConsumed() != msg.getBytes().length) {
fail("client wrap() did not consume all application data: " +
enabledProtocols.get(i));
}
cliToSer.flip();

/* unwrap() with too small output buffer must return
* BUFFER_OVERFLOW, CLOSED would mask the need to enlarge the
* output buffer and pending data would never be delivered */
result = server.unwrap(cliToSer, tooSmall);
if (result.getStatus() != SSLEngineResult.Status.BUFFER_OVERFLOW) {
fail("unwrap() with too small output buffer should return " +
"BUFFER_OVERFLOW, got " + result.getStatus() + ": " +
enabledProtocols.get(i));
}
if (result.bytesProduced() != 0) {
fail("unwrap() returning BUFFER_OVERFLOW should not " +
"produce data: " + enabledProtocols.get(i));
}

/* unwrap() again with large enough output buffer must deliver
* the pending application data */
result = server.unwrap(cliToSer, serPlain);
serPlain.flip();
byte[] received = new byte[serPlain.remaining()];
serPlain.get(received);
assertEquals("server did not receive application data after " +
"BUFFER_OVERFLOW retry: " + enabledProtocols.get(i),
msg, new String(received));
serPlain.clear();
cliToSer.compact();

/* Finish close sequence, both engines should close cleanly */
int loops = 0;
int maxLoops = 20;
while ((!server.isInboundDone() || !server.isOutboundDone() ||
!client.isInboundDone() || !client.isOutboundDone()) &&
(loops++ < maxLoops)) {

client.wrap(clientOut, cliToSer);
server.wrap(empty, serToCli);

cliToSer.flip();
serToCli.flip();

if (!client.isInboundDone()) {
client.unwrap(serToCli, cliPlain);
}
if (!server.isInboundDone()) {
server.unwrap(cliToSer, serPlain);
}

cliToSer.compact();
serToCli.compact();
}

if (!server.isInboundDone() || !server.isOutboundDone() ||
!client.isInboundDone() || !client.isOutboundDone()) {
fail("SSLEngines failed to close within " + maxLoops +
" wrap/unwrap iterations after closeOutbound(): " +
enabledProtocols.get(i));
}
}
}

@Test
public void testBufferUnderflowPartialRecord()
throws NoSuchProviderException, NoSuchAlgorithmException,
Expand Down
Loading