Adds TLS SNI to ghidra client connections #8
Conversation
|
Had you encountered issues with the canonical hostname resolution issue NationalSecurityAgency#4924 in conjunction with the use of a proxy server? It seems like it would cause an issue, even with your change, if connectioning to multiple separate Ghidra Servers handled by the proxy agent due to the manner that server connections are cached. |
|
No, but it could be used to work around the issue in a cloud environment. AWS allows you to configure reverse DNS for Network Load Balancers with EIPs assigned. This PR was originally created to allow for a single AWS SNI-based proxy to be in front of multiple Ghidra Server instances using the same RMI ports. SNI aware proxies can route IP traffic to different backends solely based on FQDN and subdomain used to connect to the server. |
|
@dtshepherd I think this PR suffers from the same problem that the canonical hostname issue raises. The key point is that unless a different IP is used for each connection, you cannot distinguish between connections. You cannot use a proxy because regardless of the servername the user entered when creating the project, Ghidra will always connect with (and send as the SNI) the canonical hostname for the IP. So this PR in itself doesn't really help at all, and cannot be used to work around the canonical hostname issue. |
|
This PR is really dependent on correcting the canonical hostname transformation issue raised by NationalSecurityAgency#4924. |
|
Is this change still neccessary given that the canonical hostname transformation issue raised by NationalSecurityAgency#4924 has been resolved? |
WalkthroughThe changes introduce explicit Server Name Indication (SNI) support for SSL/TLS client connections within Ghidra's server and RMI infrastructure. A new class, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GhidraSSLClientSocket
participant Server
Client->>GhidraSSLClientSocket: createSocket(host, port)
GhidraSSLClientSocket->>Server: Establish SSL connection with SNI(host)
Server-->>GhidraSSLClientSocket: SSL handshake (with SNI)
GhidraSSLClientSocket-->>Client: Return configured SSLSocket
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Ghidra/Features/GhidraServer/src/main/java/ghidra/server/stream/RemoteBlockStreamHandle.java(2 hunks)Ghidra/Framework/FileSystem/src/main/java/ghidra/framework/client/GhidraSSLClientSocket.java(1 hunks)Ghidra/Framework/FileSystem/src/main/java/ghidra/framework/client/ServerConnectTask.java(2 hunks)
🔇 Additional comments (6)
Ghidra/Framework/FileSystem/src/main/java/ghidra/framework/client/ServerConnectTask.java (2)
164-165: SNI support added for RMI registry connections.The switch from the standard
SslRMIClientSocketFactoryto the customGhidraSSLClientSocketenables Server Name Indication (SNI) when connecting to the RMI registry. This is necessary to support domain-based proxy routing as mentioned in the PR objectives.
322-323: SNI support added for SSL connection testing.Consistent with the change in the RMI registry connection, the SSL handshake test now uses the custom
GhidraSSLClientSocketimplementation to enable SNI support when testing connectivity.Ghidra/Framework/FileSystem/src/main/java/ghidra/framework/client/GhidraSSLClientSocket.java (2)
18-23: Clean and appropriate imports for SNI functionality.The imports include necessary classes for SNI implementation including
java.net.*,java.util.*, and SSL-related classes.
29-43: Well-implemented SNI extension for SSL sockets.The class correctly extends
SslRMIClientSocketFactoryand overrides thecreateSocketmethod to add SNI support. The implementation:
- Creates the base SSL socket using the parent implementation
- Creates an SNI host name entry using the connection's host parameter
- Sets this SNI information on the socket's SSL parameters
- Returns the properly configured socket
This implementation allows the socket to indicate the intended hostname to upstream proxies, which is essential for the PR's goal of enabling proxy routing by domain name.
Ghidra/Features/GhidraServer/src/main/java/ghidra/server/stream/RemoteBlockStreamHandle.java (2)
21-24: Updated imports to support SNI functionality.The imports have been expanded to include
java.util.*and updated to import all classes fromjavax.net.*andjavax.net.ssl.*, which are needed for the SNI implementation added to the connect method.
282-287: Added SNI support to block stream socket connections.Similarly to the changes in
GhidraSSLClientSocket, this code now explicitly sets up SNI for the SSL socket used in block streaming. The implementation:
- Creates an SSL socket from the default factory
- Sets SNI server names to the stream server IP address
- Applies the updated parameters to the socket
This ensures consistent SNI behavior across all types of connections (RMI and block streaming).
| Socket socket = socketFactory.createSocket(streamServerIPAddress, streamServerPort); | ||
| SSLSocket socket = (SSLSocket)socketFactory.createSocket(streamServerIPAddress, streamServerPort); | ||
|
|
||
| List<SNIServerName> serverNames = Arrays.asList(new SNIHostName(streamServerIPAddress)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using the hostname rather than IP address for SNI.
SNI is designed to indicate the hostname being accessed, not the IP address. While using the IP address will work technically, it may not provide the expected behavior with certain proxy configurations that rely on hostname-based routing.
- List<SNIServerName> serverNames = Arrays.asList(new SNIHostName(streamServerIPAddress));
+ // Use the original hostname if available, falling back to IP address if necessary
+ String sniHostname = streamServerIPAddress;
+ List<SNIServerName> serverNames = Arrays.asList(new SNIHostName(sniHostname));Note: This implementation would require tracking and passing the original hostname through the API. The current implementation using IP address will work for basic SNI functionality, but might not provide the optimal behavior with hostname-based proxy routing.
🌐 Web query:
Is using an IP address with SNI (Server Name Indication) valid?
💡 Result:
Using an IP address with SNI (Server Name Indication) is not valid according to the SNI specification and its intended purpose.
SNI is an extension to the TLS protocol that allows a client to specify the hostname (domain name) it wants to connect to during the TLS handshake, enabling servers to present the correct certificate for that hostname even when multiple domains are hosted on the same IP address[2][3][7]. The key point is that SNI is designed for hostnames, not IP addresses.
If a client attempts to use an IP address in the SNI field:
- Most servers will ignore the SNI field or be unable to present the correct certificate, because the certificate is typically issued for a domain name, not an IP address[3][9].
- The SNI specification requires a "hostname" (per RFC 3546 and RFC 6066) in the SNI extension, not an IP address. Using an IP address does not comply with the standard and may result in errors or a fallback to a default certificate, leading to browser security warnings about certificate mismatches[2][3][9].
In summary:
SNI is designed to be used with domain names, not IP addresses. Using an IP address in the SNI field is not valid, and is not supported by the protocol or by major web servers. If you use an IP address in SNI, you will likely encounter connection errors or certificate warnings.[2][3][9]
Citations:
- 1: https://docs.venafi.com/Docs/current/TopNav/Content/Validation/r-validation-SNI-support.php
- 2: https://www.cloudflare.com/learning/ssl/what-is-sni/
- 3: https://en.wikipedia.org/wiki/Server_Name_Indication
- 4: https://www.globalsign.com/en/blog/what-is-server-name-indication
- 5: https://www.thesslstore.com/blog/what-is-sni/
- 6: https://https.cio.gov/sni/
- 7: https://knowledge.digicert.com/quovadis/ssl-certificates/ssl-general-topics/what-is-sni-server-name-indication
- 8: https://www.cheapsslshop.com/blog/what-is-server-name-indication-sni-how-it-works-on-single-ip
- 9: https://www.ibm.com/docs/en/i/7.3?topic=concepts-server-name-indicationsni
- 10: https://www.ionos.com/digitalguide/server/know-how/server-name-indication/
Use DNS hostname for SNI instead of IP address
SNI (per RFC 6066) must carry the server’s DNS name. Supplying an IP address will be ignored by many servers or cause a default‐certificate fallback, leading to TLS failures or certificate mismatches.
Please update the code at:
- Ghidra/Features/GhidraServer/src/main/java/ghidra/server/stream/RemoteBlockStreamHandle.java: line 284
Suggested changes:
- Propagate the original hostname (e.g.
streamServerHostname) through your API. - Construct the SNI value from the hostname, falling back to the IP only if the hostname is unavailable.
Example diff:
- List<SNIServerName> serverNames = Arrays.asList(new SNIHostName(streamServerIPAddress));
+ // Use DNS hostname for SNI, fallback to IP if necessary
+ String sniHostname = (streamServerHostname != null && !streamServerHostname.isEmpty())
+ ? streamServerHostname
+ : streamServerIPAddress;
+ List<SNIServerName> serverNames = Arrays.asList(new SNIHostName(sniHostname));You’ll need to update the surrounding API to accept and pass along streamServerHostname accordingly.
Committable suggestion skipped: line range outside the PR's diff.
This PR adds the SNI extension to the TLS requests, which enables proxying by domain. It should be ignored/unused for direct connections, since the Ghidra server-side doesn't look at SNI.
Summary by CodeRabbit