SSL Identity Check Bypass

[mikeburgh]

I was debugging the driver to resolve an SNI issue (trying to pass servername as an SSL option) and discovered what I think is an issue in the createSecureContext method:

mariadb-connector-nodejs/lib/connection.js

Line 1337 in d8fc60a

createSecureContext(info, callback) {

If you supply a checkServerIdentity function in the ssl config, then it wont be called, and no SSL identity check will be performed, due to this line:

 info.requireIdentifyCheck = this.opts.ssl === true || this.opts.ssl.checkServerIdentity === undefined;

I think the last check should be !== undefined, since if it's defined then it should be used and an identity check performed.

However, that causes another issue in this line:

const sslOption = this.opts.ssl === true ? baseConf : Object.assign({}, this.opts.ssl, baseConf);

baseConf is applied over the supplied ssl options, rather than the other way around. This leads to the no op function in baseConf being used, and since it does not return (same as returning undefined) the check still passes.

If both of those are fixed, then this line in handshakeResult

const identityError = tls.checkServerIdentity(opts.host, info.tlsCert);

should probably change to something like:

const identityError = tls.checkServerIdentity(typeof opts.ssl === 'object' && opts.ssl.servername ? opts.ssl.servername : opts.host, info.tlsCert);

I can put all this in a PR if that's preferred ? Just wanted to confirm this is right, or am I missing something.

[rusher]

Hi @mikeburgh,
You're absolutely right - there are two errors that need fixing:

The condition this.opts.ssl.checkServerIdentity === undefined should actually be !== (not equal to undefined)
We need to allow name validation using the provided servername parameter as you pointed out

Regarding the SSL configuration, it's intentionally set to "trust" mode by default, which establishes encrypted communication without enforcing chain validation or server name validation. This approach is explained in detail in this blog post: https://mariadb.org/mission-impossible-zero-configuration-ssl/
(Since version 11.4, the server sends an encrypted hash combining the password, seed, and certificate fingerprint, which the client validates. This eliminates the need for clients to have certificates when dealing with self-signed certificates. While this is beneficial, it requires programmatic server identity verification to work across all servers)

so forcing to "trust" mode is the intended reason of setting sslOption.

To summarize: you correctly identified 2 issues. I'd welcome a pull request from you, or I'll implement the fixes before the next release.
Thanks for your clear and detailed bug report!

[mikeburgh]

No worries, just submitted the PR.

I figured there was more going on, that's a neat solution to the problem.

Although is that why checkServerIdentity in the baseConfig is there ? if so my PR will need to change as I removed it.. In my testing between self signed, signed and no SSL, it did not seem to make a difference, but while it was present, and not replaced in the ssl options, as I understand it, all certs would pass the identity check, even if the names didn't match (self signed or otherwise).

[rusher]

ah, checkServerIdentity: () => {} is still required :

const sslOption = this.opts.ssl === true ? baseConf : Object.assign({}, this.opts.ssl, baseConf);

will default to have ssl.checkServerIdentity validation value

the problem will only concerns MariaDB 11.4+ servers: server identity is validated by another means (the hash i was indicated above), and when ssl.checkServerIdentity was set, it will fails

[mikeburgh]

Yeah the PR switched the assign order:

const sslOption = this.opts.ssl === true ? baseConf : Object.assign({}, baseConf, this.opts.ssl);

Which was why I was thinking the checkServerIdentity no op needed to go.. as in the above assignment order, when you provide any ssl object keys, but not the checkServerIdentity, you will end up with the no op in sslOptions, which TLS will call and then since it returns undefined, all certs will be treated as passing the identity check ?

If I don't switch the assign order, then you cannot set servername (since it's set to the host in baseConf), and you can never set your own checkServerIdentity method.

I have ran connections against 10.6, and 11.4 and cannot find any combinations of ssl = true, ssl = {...} where the no op checkServerIdentity method is getting used, but I think that's cause this logic is still wrong:

The PR set's it to this

info.requireIdentifyCheck = this.opts.ssl === true || this.opts.ssl.checkServerIdentity !== undefined;

But I think that's still not right in the case of setting ssl = {..} as if you are setting any SSL options, but not checkServerIdentity then that is going to still skip the identityCheck, and that's not right.

Not sure how to resolve that, short of being explicit about the identity check, and an option to disable it ?