Netdata fixes part 6 - #22906
Merged
Merged
Conversation
security_test_certificate() obtains the peer certificate with SSL_get_peer_certificate(). On OpenSSL 3.x this is an alias of SSL_get1_peer_certificate(), which returns an owned reference that must be released with X509_free(). The current SSL verification path checks the certificate and returns without releasing that reference, leaking one X509 object for each verified SSL connection that presents a peer certificate. This is a current issue because nd_sock_open_ssl() calls security_test_certificate() when certificate verification is enabled. Free the owned certificate reference after SSL_get_verify_result() has been captured and before returning the existing result. The NULL-certificate path is unchanged and still returns immediately. Performance impact is negligible: one X509_free() refcount decrement per checked SSL connection. Functional impact is limited to releasing the local OpenSSL reference; return values, logging, verification state, and connection behavior are unchanged. The required review gate passed with all reviewers voting production grade and absolutely necessary. (cherry picked from commit 64ad6a8)
MEM-socket-003: nd_sock_connect_to_this() could retain an allocated SSL object after a TCP connect succeeded but TLS setup failed, leaving the object bound to a file descriptor that was then closed. Classification: current bounded error-path lifetime bug. Functional impact: failure return values, socket errors, retry behavior, certificate policy, and successful connections are unchanged. Performance impact: successful paths are unchanged; failed TLS setup now performs immediate SSL/SNI cleanup instead of deferring it to later socket cleanup. Clean the SSL sub-structure before closing the fd and clear the SNI hostname allocated for the failed TLS attempt. (cherry picked from commit 8f4d5cc)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant App as Application
participant ND_Sock as nd_sock
participant OpenSSL as OpenSSL Library
participant Security as security.c
Note over App,Security: TLS Connection Lifecycle
App->>ND_Sock: nd_sock_connect_to_this()
ND_Sock->>ND_Sock: Create socket & set SNI hostname
alt TLS enabled (ssl && s->ctx)
ND_Sock->>ND_Sock: nd_sock_open_ssl()
alt TLS setup succeeds
ND_Sock->>Security: security_test_certificate(ssl)
Security->>OpenSSL: SSL_get_peer_certificate()
OpenSSL-->>Security: X509 cert (owned reference)
Security->>OpenSSL: X509_verify_cert() etc.
Security->>OpenSSL: X509_free(cert)
Security-->>ND_Sock: ret (0 or 1)
ND_Sock-->>App: success
else TLS setup fails
ND_Sock->>ND_Sock: netdata_ssl_close(&s->ssl)
ND_Sock->>ND_Sock: close(s->fd)
ND_Sock->>ND_Sock: freez(s->sni_hostname)
ND_Sock->>ND_Sock: s->sni_hostname = NULL
ND_Sock-->>App: false (connection failed)
end
else No TLS
ND_Sock-->>App: plain socket connection
end
Note over Security: Verification completion (all paths)
Security->>OpenSSL: X509_free(cert) — ensures cert freed<br/>even on failed verification
stelfrag
marked this pull request as ready for review
June 29, 2026 18:26
thiagoftsm
approved these changes
Jun 30, 2026
thiagoftsm
left a comment
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
No issues found between runtime. LGTM!
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by cubic
Fixes SSL/TLS resource leaks and cleanup paths: frees the peer certificate on OpenSSL 3.x and clears SSL/SNI state when TLS setup fails. Behavior is unchanged; reduces memory leaks on failed and verified connections.
Written for commit 5418c02. Summary will update on new commits.