Copilot commented on code in PR #11503:
URL: https://github.com/apache/cloudstack/pull/11503#discussion_r2667188221
##########
utils/src/main/java/org/apache/cloudstack/utils/security/SSLUtils.java:
##########
@@ -70,6 +70,10 @@ public static SSLContext getSSLContext() throws
NoSuchAlgorithmException {
return SSLContext.getInstance("TLSv1.2");
}
+ public static SSLContext getSSLContextWithLatestProtocolVersion() throws
NoSuchAlgorithmException {
+ return SSLContext.getInstance("TLSv1.3");
+ }
Review Comment:
The method name in the PR description says
'getSSLContextWithLatestVersion()' but the actual implementation uses
'getSSLContextWithLatestProtocolVersion()'. The description should be updated
to match the implementation, or vice versa.
##########
utils/src/main/java/com/cloud/utils/nio/Link.java:
##########
@@ -174,116 +197,88 @@ public static void write(SocketChannel ch, ByteBuffer[]
buffers, SSLEngine sslEn
}
}
- /* SSL has limitation of 16k, we may need to split packets. 18000 is 16k +
some extra SSL informations */
- protected static final int MAX_SIZE_PER_PACKET = 18000;
- protected static final int HEADER_FLAG_FOLLOWING = 0x10000;
-
public byte[] read(SocketChannel ch) throws IOException {
- if (_readHeader) { // Start of a packet
- if (_readBuffer.position() == 0) {
- _readBuffer.limit(4);
- }
-
- if (ch.read(_readBuffer) == -1) {
- throw new IOException("Connection closed with -1 on reading
size.");
- }
-
- if (_readBuffer.hasRemaining()) {
- LOGGER.trace("Need to read the rest of the packet length");
- return null;
- }
- _readBuffer.flip();
- int header = _readBuffer.getInt();
- int readSize = (short)header;
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Packet length is " + readSize);
- }
-
- if (readSize > MAX_SIZE_PER_PACKET) {
- throw new IOException("Wrong packet size: " + readSize);
- }
-
- if (!_gotFollowingPacket) {
- _plaintextBuffer = ByteBuffer.allocate(2000);
- }
-
- if ((header & HEADER_FLAG_FOLLOWING) != 0) {
- _gotFollowingPacket = true;
- } else {
- _gotFollowingPacket = false;
- }
-
- _readBuffer.clear();
- _readHeader = false;
-
- if (_readBuffer.capacity() < readSize) {
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Resizing the byte buffer from " +
_readBuffer.capacity());
- }
- _readBuffer = ByteBuffer.allocate(readSize);
- }
- _readBuffer.limit(readSize);
+ if (_sslEngine == null) {
+ throw new IOException("SSLEngine not set");
}
-
- if (ch.read(_readBuffer) == -1) {
+ if (ch.read(netBuffer) == -1) {
throw new IOException("Connection closed with -1 on read.");
}
-
- if (_readBuffer.hasRemaining()) { // We're not done yet.
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Still has " + _readBuffer.remaining());
- }
- return null;
- }
-
- _readBuffer.flip();
-
- ByteBuffer appBuf;
-
- SSLSession sslSession = _sslEngine.getSession();
- SSLEngineResult engResult;
- int remaining = 0;
-
- while (_readBuffer.hasRemaining()) {
- remaining = _readBuffer.remaining();
- appBuf = ByteBuffer.allocate(sslSession.getApplicationBufferSize()
+ 40);
- engResult = _sslEngine.unwrap(_readBuffer, appBuf);
- if (engResult.getHandshakeStatus() != HandshakeStatus.FINISHED &&
engResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING &&
- engResult.getStatus() != SSLEngineResult.Status.OK) {
- throw new IOException("SSL: SSLEngine return bad result! " +
engResult);
- }
- if (remaining == _readBuffer.remaining()) {
- throw new IOException("SSL: Unable to unwrap received data!
still remaining " + remaining + "bytes!");
- }
-
- appBuf.flip();
- if (_plaintextBuffer.remaining() < appBuf.limit()) {
- // We need to expand _plaintextBuffer for more data
- ByteBuffer newBuffer =
ByteBuffer.allocate(_plaintextBuffer.capacity() + appBuf.limit() * 5);
- _plaintextBuffer.flip();
- newBuffer.put(_plaintextBuffer);
- _plaintextBuffer = newBuffer;
+ netBuffer.flip();
+ while (netBuffer.hasRemaining()) {
+ SSLEngineResult res;
+ try {
+ res = _sslEngine.unwrap(netBuffer, appBuffer);
+ } catch (SSLException e) {
+ throw new IOException("SSL unwrap failed: " + e.getMessage(),
e);
}
- _plaintextBuffer.put(appBuf);
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Done with packet: " + appBuf.limit());
+ switch (res.getStatus()) {
+ case OK:
+ appBuffer.flip();
+ while (appBuffer.hasRemaining()) {
+ if (frameRemaining < 0) {
+ int need = 4 - headerBuffer.position();
+ int take = Math.min(need, appBuffer.remaining());
+ int oldLimit = appBuffer.limit();
+ appBuffer.limit(appBuffer.position() + take);
+ headerBuffer.put(appBuffer);
+ appBuffer.limit(oldLimit);
+ if (headerBuffer.position() < 4) break;
+ headerBuffer.flip();
+ frameRemaining = headerBuffer.getInt();
+ headerBuffer.clear();
+ if (frameRemaining < 0) {
+ throw new IOException("Negative frame length");
+ }
+ if (plainTextBuffer == null ||
plainTextBuffer.capacity() < frameRemaining) {
+ plainTextBuffer =
ByteBuffer.allocate(Math.max(frameRemaining, 2048));
+ }
+ plainTextBuffer.clear();
+ } else {
+ int toCopy = Math.min(frameRemaining,
appBuffer.remaining());
+ if (plainTextBuffer.remaining() < toCopy) {
+ ByteBuffer newBuffer =
ByteBuffer.allocate(plainTextBuffer.capacity() + Math.max(toCopy, 4096));
+ plainTextBuffer.flip();
+ newBuffer.put(plainTextBuffer);
+ plainTextBuffer = newBuffer;
+ }
+ int oldLimit = appBuffer.limit();
+ appBuffer.limit(appBuffer.position() + toCopy);
+ plainTextBuffer.put(appBuffer);
+ appBuffer.limit(oldLimit);
+ frameRemaining -= toCopy;
+ if (frameRemaining == 0) {
+ plainTextBuffer.flip();
+ byte[] result = new
byte[plainTextBuffer.remaining()];
+ plainTextBuffer.get(result);
+ appBuffer.compact();
+ netBuffer.compact();
+ frameRemaining = -1;
+ return result;
+ }
+ }
+ }
+ appBuffer.compact();
+ break;
+ case BUFFER_OVERFLOW:
+ appBuffer = enlargeBuffer(appBuffer,
_sslEngine.getSession().getApplicationBufferSize());
Review Comment:
The BUFFER_OVERFLOW case allocates a new buffer without preserving data from
appBuffer. When overflow occurs during unwrap, appBuffer may contain unconsumed
application data from a previous compact operation (line 261). The
enlargeBuffer call discards this data, corrupting the frame assembly. The data
should be preserved by flipping appBuffer, copying to the new buffer, and then
compacting.
```suggestion
// Preserve existing data in appBuffer while enlarging
it for the next unwrap
appBuffer.flip();
int requiredCapacity =
_sslEngine.getSession().getApplicationBufferSize() + appBuffer.remaining();
ByteBuffer newAppBuffer =
ByteBuffer.allocate(requiredCapacity);
newAppBuffer.put(appBuffer);
appBuffer = newAppBuffer;
appBuffer.compact();
```
##########
utils/src/main/java/org/apache/cloudstack/utils/security/SSLUtils.java:
##########
@@ -70,6 +70,10 @@ public static SSLContext getSSLContext() throws
NoSuchAlgorithmException {
return SSLContext.getInstance("TLSv1.2");
}
Review Comment:
The new method getSSLContextWithLatestProtocolVersion lacks documentation.
Add a Javadoc comment explaining that it returns an SSLContext configured for
TLS 1.3, and clarify when this should be used versus the TLS 1.2 method to help
maintainers understand the distinction.
```suggestion
/**
* Returns an {@link SSLContext} configured for the latest supported TLS
protocol version (currently TLSv1.3).
* <p>
* Use this method when both client and server support TLSv1.3 and you
want to take advantage of the
* latest protocol improvements. For environments that require or are
limited to TLSv1.2, use
* {@link #getSSLContext()} instead.
*
* @return an {@code SSLContext} instance using the {@code TLSv1.3}
protocol
* @throws NoSuchAlgorithmException if the TLSv1.3 protocol is not
available in the current environment
*/
```
##########
utils/src/main/java/com/cloud/utils/nio/Link.java:
##########
@@ -103,58 +102,82 @@ public void setKey(SelectionKey key) {
public void setSSLEngine(SSLEngine sslEngine) {
_sslEngine = sslEngine;
+ if (_sslEngine == null) {
+ netBuffer = null;
+ appBuffer = null;
+ headerBuffer.clear();
+ frameRemaining = -1;
+ plainTextBuffer = null;
+ return;
+ }
+ final SSLSession s = _sslEngine.getSession();
+ netBuffer = ByteBuffer.allocate(Math.max(s.getPacketBufferSize(), 16 *
1024));
+ appBuffer = ByteBuffer.allocate(Math.max(s.getApplicationBufferSize(),
16 * 1024));
+ headerBuffer.clear();
+ frameRemaining = -1;
+ plainTextBuffer = null;
}
private static void doWrite(SocketChannel ch, ByteBuffer[] buffers,
SSLEngine sslEngine) throws IOException {
- SSLSession sslSession = sslEngine.getSession();
- ByteBuffer pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
- SSLEngineResult engResult;
-
- ByteBuffer headBuf = ByteBuffer.allocate(4);
+ if (sslEngine == null) {
+ throw new IOException("SSLEngine not set");
+ }
+ final SSLSession session = sslEngine.getSession();
+ ByteBuffer netBuf = ByteBuffer.allocate(session.getPacketBufferSize());
+ // Build app sequence: 4-byte length header + payload buffers
int totalLen = 0;
- for (ByteBuffer buffer : buffers) {
- totalLen += buffer.limit();
- }
-
- int processedLen = 0;
- while (processedLen < totalLen) {
- headBuf.clear();
- pkgBuf.clear();
- engResult = sslEngine.wrap(buffers, pkgBuf);
- if (engResult.getHandshakeStatus() != HandshakeStatus.FINISHED &&
engResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING &&
- engResult.getStatus() != SSLEngineResult.Status.OK) {
- throw new IOException("SSL: SSLEngine return bad result! " +
engResult);
+ for (ByteBuffer b : buffers) totalLen += b.remaining();
Review Comment:
The totalLen calculation could overflow if the sum of buffer.remaining()
across all buffers exceeds Integer.MAX_VALUE. While unlikely in practice (would
require >2GB of buffered data), an overflow would result in a corrupted
negative frame length being written to the header. Consider validating totalLen
is positive after the loop, or using long arithmetic with a range check.
##########
utils/src/main/java/com/cloud/utils/nio/Link.java:
##########
@@ -103,58 +102,82 @@ public void setKey(SelectionKey key) {
public void setSSLEngine(SSLEngine sslEngine) {
_sslEngine = sslEngine;
+ if (_sslEngine == null) {
+ netBuffer = null;
+ appBuffer = null;
+ headerBuffer.clear();
+ frameRemaining = -1;
+ plainTextBuffer = null;
+ return;
+ }
+ final SSLSession s = _sslEngine.getSession();
+ netBuffer = ByteBuffer.allocate(Math.max(s.getPacketBufferSize(), 16 *
1024));
+ appBuffer = ByteBuffer.allocate(Math.max(s.getApplicationBufferSize(),
16 * 1024));
+ headerBuffer.clear();
+ frameRemaining = -1;
+ plainTextBuffer = null;
}
private static void doWrite(SocketChannel ch, ByteBuffer[] buffers,
SSLEngine sslEngine) throws IOException {
- SSLSession sslSession = sslEngine.getSession();
- ByteBuffer pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
- SSLEngineResult engResult;
-
- ByteBuffer headBuf = ByteBuffer.allocate(4);
+ if (sslEngine == null) {
+ throw new IOException("SSLEngine not set");
+ }
+ final SSLSession session = sslEngine.getSession();
+ ByteBuffer netBuf = ByteBuffer.allocate(session.getPacketBufferSize());
+ // Build app sequence: 4-byte length header + payload buffers
int totalLen = 0;
- for (ByteBuffer buffer : buffers) {
- totalLen += buffer.limit();
- }
-
- int processedLen = 0;
- while (processedLen < totalLen) {
- headBuf.clear();
- pkgBuf.clear();
- engResult = sslEngine.wrap(buffers, pkgBuf);
- if (engResult.getHandshakeStatus() != HandshakeStatus.FINISHED &&
engResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING &&
- engResult.getStatus() != SSLEngineResult.Status.OK) {
- throw new IOException("SSL: SSLEngine return bad result! " +
engResult);
+ for (ByteBuffer b : buffers) totalLen += b.remaining();
+ ByteBuffer header = ByteBuffer.allocate(4);
+ header.putInt(totalLen).flip();
+
+ ByteBuffer[] appSeq = new ByteBuffer[buffers.length + 1];
+ appSeq[0] = header;
+ for (int i = 0; i < buffers.length; i++) {
+ appSeq[i + 1] = buffers[i].duplicate();
+ }
+
+ while (true) {
+ // Check if all app buffers are fully consumed
+ boolean allDone = true;
+ for (ByteBuffer b : appSeq) {
+ if (b.hasRemaining()) {
+ allDone = false;
+ break;
+ }
}
+ if (allDone) break;
- processedLen = 0;
- for (ByteBuffer buffer : buffers) {
- processedLen += buffer.position();
+ netBuf.clear();
+ SSLEngineResult res;
+ try {
+ res = sslEngine.wrap(appSeq, netBuf);
+ } catch (SSLException e) {
+ throw new IOException("SSL wrap failed: " + e.getMessage(), e);
}
-
- int dataRemaining = pkgBuf.position();
- int header = dataRemaining;
- int headRemaining = 4;
- pkgBuf.flip();
- if (processedLen < totalLen) {
- header = header | HEADER_FLAG_FOLLOWING;
+ switch (res.getStatus()) {
+ case OK:
+ netBuf.flip();
+ while (netBuf.hasRemaining()) {
+ ch.write(netBuf); // may be partial, loop until drained
+ }
+ break;
+ case BUFFER_OVERFLOW:
+ netBuf = enlargeBuffer(netBuf,
session.getPacketBufferSize());
+ break;
+ case CLOSED:
+ throw new IOException("SSLEngine is CLOSED during write");
+ default:
+ throw new IOException("Unexpected SSLEngineResult status
on wrap: " + res.getStatus());
}
- headBuf.putInt(header);
- headBuf.flip();
-
- while (headRemaining > 0) {
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Writing Header " + headRemaining);
- }
- long count = ch.write(headBuf);
- headRemaining -= count;
+ // Drain delegated tasks if any
+ if (res.getHandshakeStatus() == HandshakeStatus.NEED_TASK) {
+ Runnable task;
+ while ((task = sslEngine.getDelegatedTask()) != null)
task.run();
}
- while (dataRemaining > 0) {
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Writing Data " + dataRemaining);
- }
- long count = ch.write(pkgBuf);
- dataRemaining -= count;
+ if (res.getHandshakeStatus() == HandshakeStatus.NEED_UNWRAP) {
+ // Unusual during application writes; upper layer should drive
handshake
+ break;
Review Comment:
When NEED_UNWRAP occurs during write, the method breaks out of the loop
without completing the write operation. This leaves some data in appSeq unsent,
but the caller assumes the write succeeded. During renegotiation, NEED_UNWRAP
can occur during wrap, and the handshake must be driven to completion before
continuing the application data write. Consider throwing an exception or
implementing handshake handling to ensure data is not silently dropped.
```suggestion
throw new IOException("SSLEngine reported NEED_UNWRAP during
write; renegotiation during write is not supported and application data may
remain unsent");
```
##########
utils/src/main/java/com/cloud/utils/nio/Link.java:
##########
@@ -174,116 +197,88 @@ public static void write(SocketChannel ch, ByteBuffer[]
buffers, SSLEngine sslEn
}
}
- /* SSL has limitation of 16k, we may need to split packets. 18000 is 16k +
some extra SSL informations */
- protected static final int MAX_SIZE_PER_PACKET = 18000;
- protected static final int HEADER_FLAG_FOLLOWING = 0x10000;
-
public byte[] read(SocketChannel ch) throws IOException {
- if (_readHeader) { // Start of a packet
- if (_readBuffer.position() == 0) {
- _readBuffer.limit(4);
- }
-
- if (ch.read(_readBuffer) == -1) {
- throw new IOException("Connection closed with -1 on reading
size.");
- }
-
- if (_readBuffer.hasRemaining()) {
- LOGGER.trace("Need to read the rest of the packet length");
- return null;
- }
- _readBuffer.flip();
- int header = _readBuffer.getInt();
- int readSize = (short)header;
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Packet length is " + readSize);
- }
-
- if (readSize > MAX_SIZE_PER_PACKET) {
- throw new IOException("Wrong packet size: " + readSize);
- }
-
- if (!_gotFollowingPacket) {
- _plaintextBuffer = ByteBuffer.allocate(2000);
- }
-
- if ((header & HEADER_FLAG_FOLLOWING) != 0) {
- _gotFollowingPacket = true;
- } else {
- _gotFollowingPacket = false;
- }
-
- _readBuffer.clear();
- _readHeader = false;
-
- if (_readBuffer.capacity() < readSize) {
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Resizing the byte buffer from " +
_readBuffer.capacity());
- }
- _readBuffer = ByteBuffer.allocate(readSize);
- }
- _readBuffer.limit(readSize);
+ if (_sslEngine == null) {
+ throw new IOException("SSLEngine not set");
}
-
- if (ch.read(_readBuffer) == -1) {
+ if (ch.read(netBuffer) == -1) {
throw new IOException("Connection closed with -1 on read.");
}
-
- if (_readBuffer.hasRemaining()) { // We're not done yet.
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Still has " + _readBuffer.remaining());
- }
- return null;
- }
-
- _readBuffer.flip();
-
- ByteBuffer appBuf;
-
- SSLSession sslSession = _sslEngine.getSession();
- SSLEngineResult engResult;
- int remaining = 0;
-
- while (_readBuffer.hasRemaining()) {
- remaining = _readBuffer.remaining();
- appBuf = ByteBuffer.allocate(sslSession.getApplicationBufferSize()
+ 40);
- engResult = _sslEngine.unwrap(_readBuffer, appBuf);
- if (engResult.getHandshakeStatus() != HandshakeStatus.FINISHED &&
engResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING &&
- engResult.getStatus() != SSLEngineResult.Status.OK) {
- throw new IOException("SSL: SSLEngine return bad result! " +
engResult);
- }
- if (remaining == _readBuffer.remaining()) {
- throw new IOException("SSL: Unable to unwrap received data!
still remaining " + remaining + "bytes!");
- }
-
- appBuf.flip();
- if (_plaintextBuffer.remaining() < appBuf.limit()) {
- // We need to expand _plaintextBuffer for more data
- ByteBuffer newBuffer =
ByteBuffer.allocate(_plaintextBuffer.capacity() + appBuf.limit() * 5);
- _plaintextBuffer.flip();
- newBuffer.put(_plaintextBuffer);
- _plaintextBuffer = newBuffer;
+ netBuffer.flip();
+ while (netBuffer.hasRemaining()) {
+ SSLEngineResult res;
+ try {
+ res = _sslEngine.unwrap(netBuffer, appBuffer);
+ } catch (SSLException e) {
+ throw new IOException("SSL unwrap failed: " + e.getMessage(),
e);
}
- _plaintextBuffer.put(appBuf);
- if (LOGGER.isTraceEnabled()) {
- LOGGER.trace("Done with packet: " + appBuf.limit());
+ switch (res.getStatus()) {
+ case OK:
+ appBuffer.flip();
+ while (appBuffer.hasRemaining()) {
+ if (frameRemaining < 0) {
+ int need = 4 - headerBuffer.position();
+ int take = Math.min(need, appBuffer.remaining());
+ int oldLimit = appBuffer.limit();
+ appBuffer.limit(appBuffer.position() + take);
+ headerBuffer.put(appBuffer);
+ appBuffer.limit(oldLimit);
+ if (headerBuffer.position() < 4) break;
+ headerBuffer.flip();
+ frameRemaining = headerBuffer.getInt();
+ headerBuffer.clear();
+ if (frameRemaining < 0) {
+ throw new IOException("Negative frame length");
+ }
Review Comment:
The frame length is read as a signed integer without an upper bound check. A
malicious or corrupted header could specify a very large frame length (e.g.,
Integer.MAX_VALUE = ~2GB), causing an OutOfMemoryError at line 234 when
allocating plainTextBuffer. Add a maximum frame size check (e.g., 16MB) to
prevent resource exhaustion attacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
