rmaucher commented on PR #1085:
URL: https://github.com/apache/tomcat/pull/1085#issuecomment-6196341788
Was part of my code review, so I would have fixed it tomorrow maybe. You
should commit it.
### [Low] TLS 1.3 is omitted from the enabled-protocol bookkeeping in the
JNI OpenSSL implementation
- File:
/home/opencode/code-review/tomcat/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java:804-825
(getEnabledProtocols), 828-870 (setEnabledProtocols);
/home/opencode/code-review/tomcat/java/org/apache/tomcat/util/net/openssl/OpenSSLContext.java:476-497
(post-init "Reconfigure the enabled protocols/ciphers" block)
- Description: The JNI implementation never accounts for TLS 1.3 in protocol
list management, while it is advertised as implemented
(OpenSSLEngine.java:114-116) and is correctly handled at the SSL_CTX level
(OpenSSLContext.java:159-160). Three concrete gaps:
1. `OpenSSLEngine.getEnabledProtocols()` checks only the
SSL_OP_NO_SSLv3/NO_TLSv1/NO_TLSv1_1/NO_TLSv1_2 options and always returns a
list without "TLSv1.3", even when TLS 1.3 is negotiated/enabled. Because the
base `SSLEngine.getSSLParameters()` (verified against OpenJDK 25 bytecode) is
built from `getEnabledProtocols()`, and the engine is exposed to web
applications via the SSLEngine request attribute, applications/JMX see an
incomplete enabled-protocol list.
2. `OpenSSLEngine.setEnabledProtocols()` has no `SSL_PROTO_TLSv1_3` case
and never sets a NO_TLSv1_3 option (the JNI constants class
org.apache.tomcat.jni.SSL does not even define one), so if the method were
executed with a protocol list that excludes TLS 1.3, TLS 1.3 would be silently
left enabled.
3. `OpenSSLContext.init()` overwrites `sslHostConfig.enabledProtocols`
after applying the OpenSSLConf (OpenSSLContext.java:477-494) with a list
recomputed solely from SSL_OP_NO_* flags, which likewise never contains TLS
1.3. This overwrite runs in the common configuration because the constructor
auto-creates an (empty) OpenSSLConf whenever no truststore/custom trust manager
is configured (OpenSSLContext.java:118-128), so after startup
`sslHostConfig.getEnabledProtocols()` no longer matches the protocol mask
actually used to create the SSL_CTX (lines 147-168). The same block also
replaces `sslHostConfig.enabledCiphers` with raw OpenSSL-format names (line
496) although the field was populated with JSSE names by SSLUtilBase
(AbstractEndpoint.java:574).
The modern panama implementation handles TLS 1.3 in all three places
(panama/OpenSSLEngine.java:740-742, 766, 776, 795-797;
panama/OpenSSLContext.java:689-691), confirming the JNI code is stale.
- Verification: Traced all consumers of
`sslHostConfig.getEnabledProtocols()`: AbstractEndpoint.createSSLEngine
(AbstractEndpoint.java:719-720) feeds it to `engine.setEnabledProtocols()`,
which is a no-op for the OpenSSL engine because engines created by
`OpenSSLContext.createSSLEngine()` pass `initialized=true`
(OpenSSLContext.java:631-636, set at line 505; guard at
OpenSSLEngine.java:829-831), so no actual protocol change results;
SSLAuthenticator.java:218-232 reads it only to emit the TLS 1.3/HTTP/2 warning,
but that branch is unreachable for this implementation because
`OpenSSLUtil.isTls13RenegAuthAvailable()` returns true (OpenSSLUtil.java:75-78,
applied at SSLUtilBase.java:148); the remaining consumers are the Manager
UI/JMX (trusted per the security model). The real handshake behaviour is
determined by the SSL_CTX created with the correct protocol mask in the
constructor, so there is no direct security impact - this is an API/reporting
inaccuracy, plus a latent "requested p
rotocols silently not enforced" gap in the unreachable `initialized=false`
path. TestOpenSSLConf.testOpenSSLConfCmdProtocol
(test/.../TestOpenSSLConf.java:164-193) exercises the reconfigure path but only
asserts absence of disabled protocols, so the missing TLS 1.3 entry is not
caught.
- Proposed fix: mirror the panama implementation.
1. Add the missing constant to org.apache.tomcat.jni.SSL (e.g. `public
static final int SSL_OP_NO_TLSv1_3 = 0x02000000;`, value per the Tomcat
Native/OpenSSL header in use).
2. In `OpenSSLEngine.getEnabledProtocols()` add `if ((opts &
SSL.SSL_OP_NO_TLSv1_3) == 0) { enabled.add(Constants.SSL_PROTO_TLSv1_3); }` and
in `setEnabledProtocols()` add `case Constants.SSL_PROTO_TLSv1_3 -> tlsv1_3 =
true;` plus `if (!tlsv1_3) { SSL.setOptions(state.ssl, SSL.SSL_OP_NO_TLSv1_3);
}`.
3. In `OpenSSLContext.init()` extend the reconfigure block with the same
`SSL_OP_NO_TLSv1_3` check, and map `SSLContext.getCiphers(state.ctx)` through
`OpenSSLCipherConfigurationParser.openSSLToJsse()` before calling
`sslHostConfig.setEnabledCiphers(...)` so the field keeps its documented
JSSE-name format.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
