Conversation
|
Making this a command-line option indicates to me that the server is normally going to be the one calling the shots. IMHO this is not a useful security control, because it relies on the server to enforce it's own trust which, in the case of this PR's attack model, is where the attack originates: the "server" (impersonated by a MitM). It would be better to make this a GUI-based setup where the client always asks the user to approve a certificate it hasn't seen before, and then (optionally) adds it to a local trust store. |
|
If this PR is merged, the You also need to be able to trust a CA certificate (root or intermediate, not the leaf), which isn't available in the options listed. I don't understand the |
The deference to the command line is because the client is not the launcher. I agree with the GUI prompt being required for untrusted certificates, but that has to happen in the launcher. Prompting a second time in the client itself adds no security value. In the JNLP model, trust has to already been established before executing the first line of downloaded code (otherwise there would be an RCE vulnerability). The only thing the client can do is continue the prior trust established by the launcher. |
The current implementation permits any combination of options, separated by commas (including multiple fingerprints).
The current implementation trusts any fingerprint provided as long as it is present in the chain provided by the server (root, intermediate, or leaf cert).
The "webserver" option is converted by the server into a specific fingerprint based the actual webserver certificate when generating the JNLP. "webserver" is not a valid option for the client. |
I agree completely: the launcher needs to have this capability. Adding it to the application, as directed by the server, serves no purpose.
Again, I agree: the launcher needs to first validate the server before downloading anything. Thus, the client cannot be trusted to make these decisions on its own. What you have built here (unless I'm very much misunderstanding) is that the server tells the client (not the launcher) what to trust. If the server has been compromised via MitM, then the client is still vulnerable. It's just vulnerable to a much different kind of attack. This kind of verification belongs in the launcher and not the client. |
This PR serves one very specific purpose: to allow the launcher to tell the client which servers it should trust. It allows a passing of the baton from the launcher to the client. With a secure launcher, but without this PR, an attacker can MITM the server API calls from the client. That results a loss of confidentiality (password disclosure by client to an attacker), and loss of authenticity of server responses (which can probably be used for client RCE via deserialization vulnerability). The launcher cannot validate API calls to the server; the client must do the validation.
The JNLP is a communication to the launcher, not the client. The JNLP must be a trusted document since it specifies what code to download, what class to execute. The launcher root of trust must be in authentication of the JNLP (the server TLS cert, or some signature or hash of the JNLP itself). Anything else allows an attacker to RCE the client by design, by modifying the JNLP. The server includes the pinned trust list in the JNLP, which is authenticated by the launcher. The client is the ultimate receiver of the trust list, and interprets it. |
|
I see only one useful setting: "use this specific trust store" with a proper certificate in it, having been fetched and approved by the launcher before the client runs. "Trust any localhost" isn't worth implementing. "Trust anybody" isn't worth implementing. Thumbprints require more code to implement your own trust store. Just use the existing SSLSocketFactory tie-ins that allow you to specify the trust managers. Writing your own trust manager that looks at the fingerprint is very non-Java. I would drop all the server-specific parts of this PR and all the client stuff with the exception of anything that points it at the correct trust store. |
Trust any localhost is intended to be used without a launcher. localhost is by nature not vulnerable to MITM attack, nor loss of confidentiality. This option primarily exists for developers, and is not present in the default configuration.
Trust anybody is intended for use in allowing backwards compatibility with the historical behavior. It is fundamentally insecure if someone modifies their server configuration to use that option, but I believe it is neccessary for practical purposes in a brown-field codebase. That's also why it has the name
The "pki" trust option is the "java defaults" trust store. For anyone with the ability to execute in that mode, they are welcome to. There is no mandate to use the trust managers you disagree with.
How do you imagine the launcher should know what servers to trust without a list provided in the JNLP? Are you assuming that the JNLP is served from the singular valid cert? Or some OOB configuration I'm very open to other designs, but this is the only one I see that is backwards compatible with current launchers, while moving us to better security. If you have an alternate model, send PR's or document a design compatible with the ecosystem of launchers. |
|
Related kayyagari/ballista#53 |
|
@mgaffigan at least with Launcher which prompts about accepting the server cert (but no longer verfies jars), is this now addressed? |
No. This is still required to address the issue. Both launcher and client are vulnerable. |
3cc59c7 to
aac9277
Compare
pacmano1
left a comment
pacmano1
left a comment
There was a problem hiding this comment.
Requesting changes, for two reasons.
The pin can be bypassed. CertificateThumbprintMatcher accepts a match anywhere in the chain the server sends. The server's certificate is public, so a MITM sends its own certificate followed by a copy of the real one and passes both the trust check and the hostname check.
Reproduced against this branch: a client configured with pki,<real thumbprint> connected to a fake server presenting [attackerCert, realServerCert], and the login request, password included, arrived at the fake server. The attacker certificate on its own is rejected. Compare only chain[0].
The CLI can't reach remote engines. CommandLineInterface calls new Client(server), which now defaults to pki,localhost, and the CLI has no way to pass a trust setting. Against an engine using the default self-signed certificate, mirth-cli-launcher.jar can only connect via localhost. From any other address it fails with PKIX path building failed.
java -jar mirth-cli-launcher.jar -a https://<engine-ip>:8443 -u admin -p <password> -v 0.0.0 # fails
java -jar mirth-cli-launcher.jar -a https://localhost:8443 -u admin -p <password> -v 0.0.0 # works
The CLI needs a trust option in the same format as -trust.
87cbcf0 to
e29517e
Compare
The client's connection monitor is a non-daemon thread, and runShell only closed the client on success. A refused connection, rejected certificate, or failed login printed the error and then hung forever. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Nothing covered command end to end. The harness image now carries the CLI and runs it as a child process, so the distribution layout and launcher manifest are covered too. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
The harness connects to the stack's server at https://oie:8443, whose certificate is self-signed and generated on first boot under a hostname no certificate could match. Certificate pinning changed the client default from "trust any self-signed certificate, never check the hostname" to "pki,localhost", so every integration test started failing the TLS handshake with a PKIX error. Give the harness its own trust setting, defaulted to insecure_trust_all_certs. It lives in HarnessConfig rather than in ci/ so the harness behaves the same in CI, from Gradle, and in an IDE; -Doie.pinnedClientTrust overrides it for a run against a real certificate. No product default changes: HarnessConfig is in the smoke test's own source set and is only ever packaged into the local CI harness image. Signed-off-by: Mitch Gaffigan <mitch@intouchpharma.com>
Covers the trust managers end to end against the certificate the server actually presents, which unit tests cannot do: the certificate is self-signed, generated on first boot, and served under a hostname it does not match. ServerCertificate discovers the thumbprint over a throwaway handshake and recomputes it from the DER encoding, so it is an independent oracle rather than a second call into the matcher under test. Three cases: pinning the discovered thumbprint logs in, where the pin is the only reason the connection can succeed; pinning a thumbprint one character off is refused, so the comparison is exact rather than merely rejecting garbage; and pki alone is refused, guarding the regression this suite tripped over. Failures are asserted as an SSLException anywhere in the cause chain. A rejected pin currently surfaces as the JDK's "trustAnchors parameter must be non-empty", since a thumbprint-only configuration defers to an empty trust store, so matching on a message would be fragile. Gated to the two derby configurations: the harness container is the same image everywhere, and only the server image that generated the certificate varies. Signed-off-by: Mitch Gaffigan <mitch@intouchpharma.com>
Pinning left command on the pki,localhost default, which cannot reach a remote server with a self-signed certificate. Adds the same -trust option the administrator takes, readable from mirth-cli-config.properties. Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
e29517e to
8e5eda8
Compare
|
@pacmano1, thank you for the review. I've pushed patches to both of those. |
4 participants
Addresses MITM vulnerability by adding
-trust {config}option to client. Addsadministrator.pinnedclienttrustoption to mirth.properties to control the option.pkiwebserver<thumbprint>insecure_trust_all_certslocalhostIf argument is not passed to the client (e.g.: for development without a launcher),
pki,localhostis used. If the option is not specified in mirth.properties or for new installs,pki,webserveris used. Multiple options can be specified by separating with commas.Typical scenarios:
✅ No edits needed, secure by default
✅ No edits needed, secure by default
insecure_trust_all_certs