Skip to content

unwrap() Client-side pre-check in SSLEngine to fix BUFFER_UNDERFLOW #352

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
rlm2002 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

unwrap() Client-side pre-check in SSLEngine to fix BUFFER_UNDERFLOW #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8f22e5
move peek logic higher in unwrap, do not consume data if
rlm2002 Apr 6, 2026
34e923d
update test to catch expected statuses
rlm2002 Apr 6, 2026
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
266 changes: 156 additions & 110 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 @@ -1459,10 +1459,46 @@ else if (hs == SSLEngineResult.HandshakeStatus.NEED_WRAP &&
prevSessionTicketCount = this.sessionTicketCount;
}

boolean hsUnderflow = false;
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When hsUnderflow is detected, the method sets status = BUFFER_UNDERFLOW but (from the shown hunk) does not update the handshake status to reflect “need more inbound data”. If the returned SSLEngineResult ends up carrying the prior HandshakeStatus (notably NEED_WRAP in this surrounding branch), callers that prioritize HandshakeStatus may incorrectly try to wrap() instead of reading more network data. Consider explicitly ensuring the result’s HandshakeStatus is NEED_UNWRAP (or otherwise consistent with BUFFER_UNDERFLOW) in the hsUnderflow path.

Copilot uses AI. Check for mistakes.
if (this.handshakeFinished == false) {
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "starting or continuing handshake");
ret = DoHandshake(false);
/* Client mode: pre-check record header to return
* BUFFER_UNDERFLOW with bytesConsumed == 0 per
* the JSSE spec. Bogus headers fall through to
* wolfSSL. Server mode is excluded: wolfSSL buffers
* partial records via the I/O callback and returns
* WANT_READ, mapping to OK + NEED_UNWRAP. Returning
* BUFFER_UNDERFLOW server-side would break the
* SSLEngineExplorer pattern, whose BUFFER_UNDERFLOW
* handler reads from the socket, not the pre-read
* ClientHello buffer. */
if (this.getUseClientMode() &&
inRemaining > 0 &&
(this.ssl.dtls() == 0)) {
synchronized (netDataLock) {
int pos = in.position();
if (inRemaining < TLS_RECORD_HEADER_LEN) {
hsUnderflow = true;
} else {
int recLen =
((in.get(pos + TLS_RECORD_LEN_HI_OFF)
& 0xFF) << 8) |
(in.get(pos + TLS_RECORD_LEN_LO_OFF)
& 0xFF);
if (recLen <= WolfSSL.MAX_RECORD_SIZE
&& inRemaining <
TLS_RECORD_HEADER_LEN + recLen) {
hsUnderflow = true;
}
}
}
}
if (hsUnderflow) {
status = SSLEngineResult.Status.BUFFER_UNDERFLOW;
} else {
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "starting or continuing handshake");
ret = DoHandshake(false);
}
Comment on lines +1474 to +1501
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When hsUnderflow is detected, the method sets status = BUFFER_UNDERFLOW but (from the shown hunk) does not update the handshake status to reflect “need more inbound data”. If the returned SSLEngineResult ends up carrying the prior HandshakeStatus (notably NEED_WRAP in this surrounding branch), callers that prioritize HandshakeStatus may incorrectly try to wrap() instead of reading more network data. Consider explicitly ensuring the result’s HandshakeStatus is NEED_UNWRAP (or otherwise consistent with BUFFER_UNDERFLOW) in the hsUnderflow path.

Copilot uses AI. Check for mistakes.
}
else {
/* TLS-only: peek at the record header and
Expand All @@ -1488,8 +1524,9 @@ else if (hs == SSLEngineResult.HandshakeStatus.NEED_WRAP &&
& 0xFF) << 8) |
(in.get(pos + TLS_RECORD_LEN_LO_OFF)
& 0xFF);
if (inRemaining <
TLS_RECORD_HEADER_LEN + recLen) {
if (recLen <= WolfSSL.MAX_RECORD_SIZE
&& inRemaining <
TLS_RECORD_HEADER_LEN + recLen) {
bufferUnderflow = true;
}
Comment on lines +1527 to 1531
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “partial TLS record” detection logic (read header, compute recLen, compare against TLS_RECORD_HEADER_LEN + recLen, and cap with MAX_RECORD_SIZE) now appears in multiple places (hsUnderflow and bufferUnderflow). This duplication increases the risk of future drift (e.g., changing the cap/conditions in one place but not the other). Consider extracting a small private helper (e.g., isPartialTlsRecord(ByteBuffer in, int inRemaining)) and reusing it for both branches.

Copilot uses AI. Check for mistakes.
}
Expand Down Expand Up @@ -1527,11 +1564,13 @@ else if (hs == SSLEngineResult.HandshakeStatus.NEED_WRAP &&
}
else {
/* Still not enough output space */
status = SSLEngineResult.Status.BUFFER_OVERFLOW;
status =
SSLEngineResult.Status.BUFFER_OVERFLOW;
}
}
else if (bufferUnderflow) {
status = SSLEngineResult.Status.BUFFER_UNDERFLOW;
status =
SSLEngineResult.Status.BUFFER_UNDERFLOW;
}
/* If we have input data, make sure output buffer
* length is greater than zero, otherwise ask app to
Expand Down Expand Up @@ -1616,7 +1655,8 @@ else if (inRemaining > 0 &&
(ssl.getError(0) == 0) &&
!this.sessionStored) {
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "calling engineHelper.saveSession()");
() ->
"calling engineHelper.saveSession()");
int ret2 = this.engineHelper.saveSession();
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "return from saveSession(), ret = " +
Expand All @@ -1628,123 +1668,129 @@ else if (inRemaining > 0 &&
}
} /* end DoHandshake() / RecvAppData() */

if (outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) {
/* Mark SSLEngine status as CLOSED */
status = SSLEngineResult.Status.CLOSED;
/* Handshake has finished and SSLEngine is closed,
* release, global JNI verify callback pointer */
this.engineHelper.unsetVerifyCallback();
}
if (!hsUnderflow) {
if (outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) {
/* Mark SSLEngine status as CLOSED */
status = SSLEngineResult.Status.CLOSED;
/* Handshake has finished and SSLEngine is closed,
* release, global JNI verify callback pointer */
this.engineHelper.unsetVerifyCallback();
}

Comment on lines +1671 to 1680
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gating the CLOSED-state propagation behind if (!hsUnderflow) can return BUFFER_UNDERFLOW even when the engine is already logically closed (outBoundOpen == false, closeNotifySent, or closeNotifyReceived). CLOSED typically should take precedence over underflow to avoid prompting callers to read more data on a closed engine. Consider checking and applying the CLOSED state before the underflow early-return path (or applying it regardless of hsUnderflow).

Suggested change
if (!hsUnderflow) {
if (outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) {
/* Mark SSLEngine status as CLOSED */
status = SSLEngineResult.Status.CLOSED;
/* Handshake has finished and SSLEngine is closed,
* release, global JNI verify callback pointer */
this.engineHelper.unsetVerifyCallback();
}
if (outBoundOpen == false || this.closeNotifySent ||
this.closeNotifyReceived) {
/* Mark SSLEngine status as CLOSED */
status = SSLEngineResult.Status.CLOSED;
/* Handshake has finished and SSLEngine is closed,
* release, global JNI verify callback pointer */
this.engineHelper.unsetVerifyCallback();
}
if (!hsUnderflow) {

Copilot uses AI. Check for mistakes.
synchronized (ioLock) {
err = ssl.getError(ret);
}
if (ret < 0 && (this.ssl.dtls() == 1) &&
(err == WolfSSL.OUT_OF_ORDER_E)) {
/* Received out of order DTLS message. Ignore and set
* our status to NEED_UNWRAP again to wait for correct
* data */
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "out of order message, dropping and " +
"putting state back to NEED_UNWRAP");
}
else if (ret < 0 &&
(err != WolfSSL.SSL_ERROR_WANT_READ) &&
(err != WolfSSL.SSL_ERROR_WANT_WRITE)) {

if (err == WolfSSL.UNKNOWN_ALPN_PROTOCOL_NAME_E) {
/* Native wolfSSL could not negotiate a common ALPN
* protocol */
this.inBoundOpen = false;
throw new SSLHandshakeException(
"Unrecognized protocol name error, ret:err = " +
ret + " : " + err);
synchronized (ioLock) {
err = ssl.getError(ret);
}
else {
/* Native wolfSSL threw an exception when unwrapping
* data, close inbound since we can't receive more
* data */
this.inBoundOpen = false;
if (err == WolfSSL.FATAL_ERROR) {
/* If client side and we received fatal alert,
* close outbound since we won't be receiving
* any more data */
this.outBoundOpen = false;
if (ret < 0 && (this.ssl.dtls() == 1) &&
(err == WolfSSL.OUT_OF_ORDER_E)) {
/* Received out of order DTLS message. Ignore and
* set our status to NEED_UNWRAP again to wait for
* correct data */
WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
() -> "out of order message, dropping and " +
"putting state back to NEED_UNWRAP");
}
else if (ret < 0 &&
(err != WolfSSL.SSL_ERROR_WANT_READ) &&
(err != WolfSSL.SSL_ERROR_WANT_WRITE)) {

if (err == WolfSSL.UNKNOWN_ALPN_PROTOCOL_NAME_E) {
/* Native wolfSSL could not negotiate a common
* ALPN protocol */
this.inBoundOpen = false;
throw new SSLHandshakeException(
"Unrecognized protocol name error, " +
"ret:err = " + ret + " : " + err);
}
/* Throw SSLHandshakeException if handshake not
* finished, otherwise throw SSLException for
* post-handshake errors */
if (!this.handshakeFinished) {
SSLHandshakeException hse2 =
new SSLHandshakeException(
"SSL/TLS handshake error, ret:err = " +
ret + " : " + err);
if (this.engineHelper != null) {
Exception verifyEx2 = this.engineHelper
.getLastVerifyException();
if (verifyEx2 != null) {
hse2.initCause(verifyEx2);
else {
/* Native wolfSSL threw an exception when
* unwrapping data, close inbound since we
* can't receive more data */
this.inBoundOpen = false;
if (err == WolfSSL.FATAL_ERROR) {
/* If client side and we received fatal
* alert, close outbound since we won't be
* receiving any more data */
this.outBoundOpen = false;
}
/* Throw SSLHandshakeException if handshake not
* finished, otherwise throw SSLException for
* post-handshake errors */
if (!this.handshakeFinished) {
SSLHandshakeException hse2 =
new SSLHandshakeException(
"SSL/TLS handshake error, ret:err = " +
ret + " : " + err);
if (this.engineHelper != null) {
Exception verifyEx2 = this.engineHelper
.getLastVerifyException();
if (verifyEx2 != null) {
hse2.initCause(verifyEx2);
}
}
throw hse2;
}
throw hse2;
throw new SSLException(
"wolfSSL error, ret:err = " + ret + " : " +
err);
}
throw new SSLException(
"wolfSSL error, ret:err = " + ret + " : " +
err);
}
}
else if (!this.handshakeFinished && (ret == 0) &&
(err == 0 || err == WolfSSL.SSL_ERROR_ZERO_RETURN)) {
else if (!this.handshakeFinished && (ret == 0) &&
(err == 0 ||
err == WolfSSL.SSL_ERROR_ZERO_RETURN)) {

boolean gotCloseNotify = false;
synchronized (ioLock) {
gotCloseNotify = this.ssl.gotCloseNotify();
}
boolean gotCloseNotify = false;
synchronized (ioLock) {
gotCloseNotify = this.ssl.gotCloseNotify();
}

if (!gotCloseNotify && !this.closeNotifyReceived) {
this.inBoundOpen = false;
this.outBoundOpen = false;
throw new SSLHandshakeException(
"Peer closed connection during handshake");
if (!gotCloseNotify && !this.closeNotifyReceived) {
this.inBoundOpen = false;
this.outBoundOpen = false;
throw new SSLHandshakeException(
"Peer closed connection during handshake");
}
}
}

/* Get DTLS drop count after data has been processed. To be
* used below when setting BUFFER_UNDERFLOW status. */
synchronized (ioLock) {
dtlsCurrDropCount = ssl.getDtlsMacDropCount();
}
/* Get DTLS drop count after data has been processed.
* To be used below when setting BUFFER_UNDERFLOW
* status. */
synchronized (ioLock) {
dtlsCurrDropCount = ssl.getDtlsMacDropCount();
}

/* Detect if we need to set BUFFER_UNDERFLOW.
* If we consume data in unwrap() but it's just a session
* ticket, we don't set BUFFER_UNDERFLOW and just continue
* on to set status as OK. */
synchronized (toSendLock) {
synchronized (netDataLock) {
if (ret <= 0 &&
err == WolfSSL.SSL_ERROR_WANT_READ &&
in.remaining() == 0 &&
inRemaining == 0 &&
(this.internalIOSendBufOffset == 0) &&
(prevSessionTicketCount ==
this.sessionTicketCount)) {

if ((this.ssl.dtls() == 0) ||
(this.handshakeFinished &&
(dtlsPrevDropCount ==
dtlsCurrDropCount))) {
/* Need more data. For DTLS only set
* after handshake has completed, since
* apps expect to switch on NEED_UNWRAP
* HandshakeStatus at that point */
status =
SSLEngineResult.Status.BUFFER_UNDERFLOW;
/* Detect if we need to set BUFFER_UNDERFLOW.
* If we consume data in unwrap() but it's just a
* session ticket, we don't set BUFFER_UNDERFLOW and
* just continue on to set status as OK. */
synchronized (toSendLock) {
synchronized (netDataLock) {
if (ret <= 0 &&
err == WolfSSL.SSL_ERROR_WANT_READ &&
in.remaining() == 0 &&
inRemaining == 0 &&
(this.internalIOSendBufOffset == 0) &&
(prevSessionTicketCount ==
this.sessionTicketCount)) {

if ((this.ssl.dtls() == 0) ||
(this.handshakeFinished &&
(dtlsPrevDropCount ==
dtlsCurrDropCount))) {
/* Need more data. For DTLS only set
* after handshake has completed, since
* apps expect to switch on NEED_UNWRAP
* HandshakeStatus at that point */
status =
SSLEngineResult.Status
.BUFFER_UNDERFLOW;
}
}
}
}
}
}
} /* end if (!hsUnderflow) */

} /* end else (outBoundOpen == true) */

synchronized (netDataLock) {
consumed += in.position() - inPosition;
Expand Down
10 changes: 7 additions & 3 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 @@ -3147,9 +3147,12 @@ public void testHandshakeUnwrapConsumedNotBufferUnderflow()
result = client.unwrap(firstHalf, cliAppBuf);

/* BUFFER_UNDERFLOW must not be returned when input was
* consumed - regression for inRemaining == 0 guard fix */
* consumed - regression for inRemaining == 0 guard fix.
* If BUFFER_UNDERFLOW is returned, bytesConsumed() must be 0
* per the JSSE spec ("No data was consumed"). */
if (result.getStatus() ==
SSLEngineResult.Status.BUFFER_UNDERFLOW) {
SSLEngineResult.Status.BUFFER_UNDERFLOW &&
result.bytesConsumed() > 0) {
Comment on lines 3147 to +3155
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test now permits BUFFER_UNDERFLOW as long as bytesConsumed() == 0, but it doesn’t assert the newly introduced behavior that specifically motivated the change in WolfSSLEngine.unwrap() (client-mode underflow on a partial TLS record header/record before calling into wolfSSL). Consider adding a targeted test case that feeds (a) <5 bytes (partial header) and (b) a full header with an incomplete payload, and asserts Status.BUFFER_UNDERFLOW with bytesConsumed() == 0 for both.

Copilot uses AI. Check for mistakes.
error("\t... failed");
fail("unwrap() with consumed handshake data must not " +
"return BUFFER_UNDERFLOW (regression: inRemaining" +
Expand All @@ -3159,7 +3162,8 @@ public void testHandshakeUnwrapConsumedNotBufferUnderflow()

/* Input was non-empty so at least some bytes must have
* been consumed */
if (result.bytesConsumed() == 0) {
if (result.getStatus() != SSLEngineResult.Status.BUFFER_UNDERFLOW
&& result.bytesConsumed() == 0) {
Comment on lines +3165 to +3166
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test now permits BUFFER_UNDERFLOW as long as bytesConsumed() == 0, but it doesn’t assert the newly introduced behavior that specifically motivated the change in WolfSSLEngine.unwrap() (client-mode underflow on a partial TLS record header/record before calling into wolfSSL). Consider adding a targeted test case that feeds (a) <5 bytes (partial header) and (b) a full header with an incomplete payload, and asserts Status.BUFFER_UNDERFLOW with bytesConsumed() == 0 for both.

Copilot uses AI. Check for mistakes.
error("\t... failed");
fail("unwrap() consumed 0 bytes from a non-empty " +
"handshake buffer");
Expand Down
Loading