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

SSL connection HostnameVerifier result must be checked #506

Closed
minii-dev opened this issue Mar 24, 2018 · 4 comments
Closed

SSL connection HostnameVerifier result must be checked #506

minii-dev opened this issue Mar 24, 2018 · 4 comments
Assignees
Milestone

Comments

@minii-dev
Copy link

@minii-dev minii-dev commented Mar 24, 2018

  • [+] Bug exists Release Version 1.2.0 ( Master Branch)
  • [?] Bug exists in Snapshot Version 1.2.1-SNAPSHOT (Develop Branch)
  • [?] Bug exists in MQTTv5 Version (mqttv5-new Branch)

HostnameVerifier set via MqttConnectOptions.setSSLHostnameVerifier(..)

  1. when connecting using ssl://... URL:
  • the verifier public boolean verify(String targetHost, SSLSession sslSession) is called
  • its return value (false) is NOT checked, and as a result, the verifier is useless. I think, on the false value an exception must be thrown indicating the SSL certificate does not contain expected host name
  1. when connecting using wss://... URL:
  • the verifier is NOT called. I am not sure, but I think it must be called and an exception must be thrown on failed check
jpwsutton added a commit that referenced this issue Mar 26, 2018
Signed-off-by: James Sutton <james.sutton@uk.ibm.com>
@jpwsutton
Copy link
Member

@jpwsutton jpwsutton commented Mar 26, 2018

Hi @minii-dev, Thanks for raising this issue! You're totally right, we were calling the custom HostnameVerifier without actually doing anything.

I've updated our v3 and v5 clients to throw an SSLPeerUnverifiedException if the HostnameVerifier returns false, resulting in a message like the one below:
(I set mine to always return false to test this)

Connecting to broker: ssl://iot.eclipse.org:8883
[MyHostnameVerifier] - Retuning False
reason 0
msg MqttException
loc MqttException
cause javax.net.ssl.SSLPeerUnverifiedException: Host: iot.eclipse.org, Peer Host: iot.eclipse.org
excep MqttException (0) - javax.net.ssl.SSLPeerUnverifiedException: Host: iot.eclipse.org, Peer Host: iot.eclipse.org
MqttException (0) - javax.net.ssl.SSLPeerUnverifiedException: Host: iot.eclipse.org, Peer Host: iot.eclipse.org
	at org.eclipse.paho.client.mqttv3.internal.ExceptionHelper.createMqttException(ExceptionHelper.java:38)
	at org.eclipse.paho.client.mqttv3.internal.ClientComms$ConnectBG.run(ClientComms.java:727)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180)
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: javax.net.ssl.SSLPeerUnverifiedException: Host: iot.eclipse.org, Peer Host: iot.eclipse.org
	at org.eclipse.paho.client.mqttv3.internal.SSLNetworkModule.start(SSLNetworkModule.java:144)
	at org.eclipse.paho.client.mqttv3.internal.ClientComms$ConnectBG.run(ClientComms.java:713)
	... 7 more

The changes are now in the develop branch and ready to test. Just so you know, there is also a default Hostname Verification implementation now available that can be enabled by setting setHttpsHostnameVerificationEnabled(true); on your Mqtt Connection Options, this will use the Default HTTPS hostname verification method.

@jpwsutton jpwsutton self-assigned this Mar 26, 2018
@minii-dev
Copy link
Author

@minii-dev minii-dev commented Mar 26, 2018

Thank you for very fast fix.
I wonder, what is the correct behaviour for wss:// connection.

@jpwsutton
Copy link
Member

@jpwsutton jpwsutton commented Mar 27, 2018

The wss network module uses the ssl network module under the covers, so the behaviour should be the same.

@minii-dev
Copy link
Author

@minii-dev minii-dev commented Mar 27, 2018

In my tests, on wss connection the verifier WAS NOT called at all, but WAS on ssl (yes, without the result check, but was called).

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

No branches or pull requests

3 participants