Skip to content

Fix based on Coverity and Sonar audits (part 12) - #22340

Merged
stelfrag merged 4 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part12
May 4, 2026
Merged

Fix based on Coverity and Sonar audits (part 12)#22340
stelfrag merged 4 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part12

Conversation

@stelfrag

Copy link
Copy Markdown
Collaborator

ktsaou added 4 commits April 30, 2026 10:27
Sonar c:S5527 (CRITICAL vulnerability): mqtt_wss_connect() configured
the peer certificate chain check and sent SNI via
SSL_set_tlsext_host_name(), but it never asked OpenSSL to verify that
the certificate's CN/SAN matched the requested hostname. A certificate
issued by a trusted CA for any other host would pass the existing
checks, allowing a man-in-the-middle to impersonate the target.

Add SSL_set1_host(client->ssl, client->target_host) before SSL_connect()
so OpenSSL's certificate verification also matches the peer's
identity against the requested hostname. The call is guarded by the
existing MQTT_WSS_SSL_DONT_CHECK_CERTS opt-out so callers that
explicitly want to skip cert checks are unchanged. Failure to
configure the hostname is reported and returns the same TLS-setup
error code path as the SNI failure above; silently leaving the
verification disabled would re-introduce the vulnerability.
…P mismatch

The previous commit added SSL_set1_host() to enable TLS hostname
verification. This catches a real MITM gap on production Cloud
connections, but it tightens the rules for on-prem deployments where
the cert's CN/SAN may legitimately not match the hostname the agent
connects to:

- the cert was issued for a different DNS name and reached via an
  alias / CNAME / load-balancer entry,
- the cert was issued for a hostname but the agent is configured
  with the IP literal,
- the cert is self-signed without proper SAN entries (CN-only).

These were already the kind of setups MQTT_WSS_SSL_ALLOW_SELF_SIGNED
was meant for. Extend cert_verify_callback so this flag also accepts
X509_V_ERR_HOSTNAME_MISMATCH and X509_V_ERR_IP_ADDRESS_MISMATCH (in
addition to the existing self-signed-leaf override). Default deployments
(no flag) keep strict hostname verification; on-prem operators who set
MQTT_WSS_SSL_ALLOW_SELF_SIGNED keep working without having to fall back
to the much blunter MQTT_WSS_SSL_DONT_CHECK_CERTS.
Use X509_VERIFY_PARAM_set1_host through SSL_get0_param for OpenSSL 1.0.2 compatibility while preserving hostname verification behavior.
X509_VERIFY_PARAM_set1_host() only matches against the certificate's
dNSName SAN. When the agent connects to an IP literal (e.g. an on-prem
deployment configured with 10.0.0.5 instead of a hostname), the
verification fails even if the certificate has a valid iPAddress SAN
because the IP literal is interpreted as a DNS name.

Try X509_VERIFY_PARAM_set1_ip_asc() first -- it parses the input as an
IP and matches against the iPAddress SAN, returning 0 when the input
is not a valid IP. If that fails (the typical DNS hostname case), fall
back to X509_VERIFY_PARAM_set1_host(). Both code paths are guarded by
the existing MQTT_WSS_SSL_DONT_CHECK_CERTS opt-out, and the
MQTT_WSS_SSL_ALLOW_SELF_SIGNED override already covers
X509_V_ERR_IP_ADDRESS_MISMATCH for setups whose certs do not match.
@sonarqubecloud

Copy link
Copy Markdown

@stelfrag

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@stelfrag I have started the AI code review. It will take a few minutes to complete.

@stelfrag
stelfrag marked this pull request as ready for review May 1, 2026 07:26
Copilot AI review requested due to automatic review settings May 1, 2026 07:26
@stelfrag
stelfrag marked this pull request as draft May 1, 2026 07:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the MQTT-over-WebSockets (WSS) client TLS verification behavior to better align with audit findings and common on-prem deployment needs.

Changes:

  • Extends the certificate verify callback to optionally accept additional TLS verification failures (hostname/IP mismatch) when MQTT_WSS_SSL_ALLOW_SELF_SIGNED is enabled.
  • Adds explicit TLS peer identity verification configuration for client->target_host, supporting both DNS hostnames and IP literals via OpenSSL verify parameters.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/aclk/mqtt_websockets/mqtt_wss_client.c
Comment thread src/aclk/mqtt_websockets/mqtt_wss_client.c
Comment thread src/aclk/mqtt_websockets/mqtt_wss_client.c
@stelfrag
stelfrag requested a review from thiagoftsm May 4, 2026 07:11
@stelfrag

stelfrag commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@stelfrag
stelfrag marked this pull request as ready for review May 4, 2026 07:14
@cubic-dev-ai

cubic-dev-ai Bot commented May 4, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@stelfrag I have started the AI code review. It will take a few minutes to complete.

@thiagoftsm thiagoftsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connection with cloud and the chart visualization are working as expected. LGTM!

@stelfrag
stelfrag merged commit c655511 into netdata:master May 4, 2026
162 checks passed
@stelfrag
stelfrag deleted the cov_fix_202604_part12 branch May 4, 2026 13:02
@stelfrag stelfrag mentioned this pull request Jun 22, 2026
Ferroin pushed a commit that referenced this pull request Jul 15, 2026
* mqtt_wss: enable TLS hostname verification via SSL_set1_host

Sonar c:S5527 (CRITICAL vulnerability): mqtt_wss_connect() configured
the peer certificate chain check and sent SNI via
SSL_set_tlsext_host_name(), but it never asked OpenSSL to verify that
the certificate's CN/SAN matched the requested hostname. A certificate
issued by a trusted CA for any other host would pass the existing
checks, allowing a man-in-the-middle to impersonate the target.

Add SSL_set1_host(client->ssl, client->target_host) before SSL_connect()
so OpenSSL's certificate verification also matches the peer's
identity against the requested hostname. The call is guarded by the
existing MQTT_WSS_SSL_DONT_CHECK_CERTS opt-out so callers that
explicitly want to skip cert checks are unchanged. Failure to
configure the hostname is reported and returns the same TLS-setup
error code path as the SNI failure above; silently leaving the
verification disabled would re-introduce the vulnerability.

* mqtt_wss: extend MQTT_WSS_SSL_ALLOW_SELF_SIGNED to cover hostname / IP mismatch

The previous commit added SSL_set1_host() to enable TLS hostname
verification. This catches a real MITM gap on production Cloud
connections, but it tightens the rules for on-prem deployments where
the cert's CN/SAN may legitimately not match the hostname the agent
connects to:

- the cert was issued for a different DNS name and reached via an
  alias / CNAME / load-balancer entry,
- the cert was issued for a hostname but the agent is configured
  with the IP literal,
- the cert is self-signed without proper SAN entries (CN-only).

These were already the kind of setups MQTT_WSS_SSL_ALLOW_SELF_SIGNED
was meant for. Extend cert_verify_callback so this flag also accepts
X509_V_ERR_HOSTNAME_MISMATCH and X509_V_ERR_IP_ADDRESS_MISMATCH (in
addition to the existing self-signed-leaf override). Default deployments
(no flag) keep strict hostname verification; on-prem operators who set
MQTT_WSS_SSL_ALLOW_SELF_SIGNED keep working without having to fall back
to the much blunter MQTT_WSS_SSL_DONT_CHECK_CERTS.

* mqtt_wss: address PR-review findings

Use X509_VERIFY_PARAM_set1_host through SSL_get0_param for OpenSSL 1.0.2 compatibility while preserving hostname verification behavior.

* mqtt_wss: support IP literals in TLS hostname verification

X509_VERIFY_PARAM_set1_host() only matches against the certificate's
dNSName SAN. When the agent connects to an IP literal (e.g. an on-prem
deployment configured with 10.0.0.5 instead of a hostname), the
verification fails even if the certificate has a valid iPAddress SAN
because the IP literal is interpreted as a DNS name.

Try X509_VERIFY_PARAM_set1_ip_asc() first -- it parses the input as an
IP and matches against the iPAddress SAN, returning 0 when the input
is not a valid IP. If that fails (the typical DNS hostname case), fall
back to X509_VERIFY_PARAM_set1_host(). Both code paths are guarded by
the existing MQTT_WSS_SSL_DONT_CHECK_CERTS opt-out, and the
MQTT_WSS_SSL_ALLOW_SELF_SIGNED override already covers
X509_V_ERR_IP_ADDRESS_MISMATCH for setups whose certs do not match.

---------

Co-authored-by: Costa Tsaousis <costa@netdata.cloud>
(cherry picked from commit c655511)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants