Skip to content
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.

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

ZnClient allows untrusted HTTPS certificates #146

Open
Rinzwind opened this issue Jun 30, 2024 · 12 comments
Open

ZnClient allows untrusted HTTPS certificates #146

Rinzwind opened this issue Jun 30, 2024 · 12 comments

Comments

@Rinzwind
Copy link

When using HTTPS, ZnClient seems to not require the server to have a trusted certificate. In Pharo VM pull request #816, I gave a snippet for setting up a ZnSecureServer on macOS, which can be continued as follows:

"Remove trust in the certificate authority’s certificate (needs confirmation):"
result7 := LibC resultOfCommand: 'cd /tmp && /usr/bin/security ' ,
	'remove-trusted-cert ca-cert 2>&1'.

"Get ‘/’ from the server again:"
response2 := ZnEasy get: 'https://localhost:1443'.

The last send of #get: unexpectedly still answers a ZnResponse, I would have expected it to signal an error instead as the server’s certificate is no longer trusted.

There’s a method #certificateVerificationState on ZdcPluginSSLSession. Adding connection sslSession certificateVerificationState inspect at the end of #setupTLSTo: on ZnClient shows that it does distinguish between the certificate being trusted (value 0) or not (value 1), at least for the example of this snippet. But the method is not otherwise used.

While it would be good to, when the certificates do get verified, still have an option to disable that, I would expect it to be enabled by default, as described in the last paragraph of section 4.3.4 in RFC 9110.

@svenvc
Copy link
Owner

svenvc commented Jul 1, 2024

You are absolutely right, the default should be to verify trust in server certificates as a client.

The reason this is not done is historical, though I fear nothing much has changed.

To enable this, it must be supported on all 3 major platforms. The SSL plugin is native code that is totally different between those platforms. As far as I know, it is incomplete, especially in this area.

So this is an area for improvement (but my opinion is that the plugin needs a serious rewrite, not my area of expertise).

@Rinzwind
Copy link
Author

Rinzwind commented Jul 2, 2024

Would having a check on #certificateVerificationState with the current plugin implementations work, or are they known to have false negatives, meaning cases in which a certificate that is valid according to say Chrome or curl is not valid according to the plugin? False positives are also undesirable but not worse I guess than the current situation of not having a check at all.

@Rinzwind
Copy link
Author

Rinzwind commented Sep 2, 2024

I tested for false negatives by modifying #setupTLSTo: to signal a ZnCertificateVerificationFailed when #certificateVerificationState is not 0, and executing the snippet below, which sends a HEAD request to 100 hosts selected at random from Chromium’s HSTS preload list and then sends another request using curl to the hosts for which the error was signaled. While there’s some inherent bias in selecting from that list, it does contain various hosts for which a ZnCertificateVerificationFailed is signaled. Having executed this several times on macOS and, through virtual machines, on Windows and Ubuntu, I have not yet had a case in which curl didn’t also report a certificate validation error.

url := 'https://github.com/chromium/chromium/raw/main/net/http/transport_security_state_static.json'.
contents := (ZnEasy get: url) contents.
nonCommentLines := contents lines reject: [ :line | line matchesRegex: '[[:space:]]*//.*' ].
entries := (STONJSON fromString: (String lf join: nonCommentLines)) at: 'entries'.
hosts := entries select: [ :entry |
		(#('test' 'public-suffix' 'public-suffix-requested') includes: (entry at: 'policy')) not ]
	thenCollect: [ :entry | entry at: 'name' ].
randomHosts := (1 to: 100) collect: [ :i | hosts atRandom ].
results := Dictionary newFromKeys: randomHosts andValues: (randomHosts collect: [ :host |
	([ ZnConnectionTimeout value: 2 during: [ ZnEasy head: 'https://' , host ] ]
		on: Error do: [ :error | error ]) ]).
failedHosts := (results select: [ :result | result isKindOf: ZnCertificateVerificationFailed ]) keys.
curlResults := Dictionary newFromKeys: failedHosts andValues: (failedHosts collect: [ :host |
	LibC resultOfCommand: 'curl --silent --show-error --head https://' , host , ' 2>&1' ]).

On Ubuntu I had to execute the following first because, as far as I understand, the default directory path is compiled into the OpenSSL library and the path in the library included with the Pharo VM is not the path compiled into Ubuntu’s library:

OSEnvironment current at: 'SSL_CERT_DIR' put: '/usr/lib/ssl/certs'

I would suggest adding the check on #certificateVerificationState and an option for controlling what that should do in case of failure: ignore, warn or signal, with the second as the default for now which I guess should just log the failure on the transcript.

@Rinzwind
Copy link
Author

@svenvc: I was wondering whether you took a look at my previous comment? In particular: the suggestion to add a check on #certificateVerificationState and an option to control what that should do in case of failure, with ‘warn’ as the default for now.

@svenvc
Copy link
Owner

svenvc commented Oct 25, 2024

Yes I already had a look and yes I want to try this, at least the first step of it.

@svenvc
Copy link
Owner

svenvc commented Jan 7, 2025

Hi Kris,

I finally took some time to try something. This is the simplest thing that I could think of following your suggestion, we can improve when necessary.

The code is in the feenk fork for now, but I will sync with the original upstream soon.

feenkcom@40933f3

The following fails and can be proceeded:

ZnClient new get: 'https://expired-rsa-ev.ssl.com/'

From https://www.ssl.com/sample-valid-revoked-and-expired-ssl-tls-certificates/

@svenvc
Copy link
Owner

svenvc commented Jan 7, 2025

Hmm, the linux CI jobs are not happy while these tests passed for me on macOS 15, GT/Pharo 11, M2.

https://github.com/feenkcom/zinc/actions/runs/12654653342/job/35264030074

That must have something to do with certificate validation installed by default.

@svenvc
Copy link
Owner

svenvc commented Jan 7, 2025

I made the check optional

feenkcom@f1d99a1

@Rinzwind
Copy link
Author

Rinzwind commented Jan 7, 2025

Thanks! The CI uses Ubuntu per line 7 in ‘CI.yml’, so I assume my comment about needing to set SSL_CERT_DIR applies, try changing the ‘run’ command on line 17 to:

SSL_CERT_DIR=/usr/lib/ssl/certs smalltalkci -s ${{ matrix.smalltalk }}

@svenvc
Copy link
Owner

svenvc commented Jan 7, 2025

I will study that tomorrow, thanks.

@svenvc
Copy link
Owner

svenvc commented Jan 9, 2025

I experimented with SSL_CERT_DIR in my Ubuntu VM in an interactive development environment and I can confirm that it works as expected. I.e. without it, many websites fail to validate, with it, they do validate as on Mac.

Why is this not set by default then ?

There are thus 3 ways to set it:

  • as a prefix in the run command
  • as an exported env var before doing the run
  • setting the env var from within the image

I am not sure what would be the best way.
I do not want things to fail for other users.

@Rinzwind
Copy link
Author

I’m not sure how best to deal with the problem of needing to configure the directory for OpenSSL either, but as things won’t break for anyone as long as the default for the option #verifyCertificates is false, I would suggest to proceed and merge commit f1d99a1 in the upstream repository. I can handle opening a pull request to update the version of Zinc included in Pharo and posing the question there of how to deal with the problem so that the option’s default can be changed to true later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants