Skip to content

Fix based on Coverity and Sonar audits (part 3) - #22331

Merged
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part3
May 6, 2026
Merged

Fix based on Coverity and Sonar audits (part 3)#22331
stelfrag merged 3 commits into
netdata:masterfrom
stelfrag:cov_fix_202604_part3

Conversation

@stelfrag

Copy link
Copy Markdown
Collaborator

ktsaou added 3 commits April 30, 2026 09:49
Sonar c:S2612 (MAJOR vulnerability): the management API key file was
created with `open(..., O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 444)`. The
literal `444` is decimal, not octal, which equals octal 0674 = rw-rwxr--.
Group gets read+write+execute and others get read on a file that stores
the management API key (a UUID granting admin endpoint access). Anyone
with local read on the host can lift the key.

Even if the original intent was octal `0444` (world-readable), a
secret-key file should not be readable by group or others. Use `0600`
(owner read+write only), the standard mode for secrets.
Sonar c:S2612: file_lock_get() created the advisory lock file with
mode 0666 (rw-rw-rw-). Netdata runs as a single dedicated user, so
group/other access is unnecessary and adds latent risk if any local
user can interfere with the lock file.

flock(2) is purely advisory and does not enforce by mode, but the open()
permission still controls who can create/access the file. Owner-only
0600 keeps the locking behavior intact for the netdata user while
preventing unrelated local users from creating or opening the file.

The function is currently unused (its caller in src/daemon/main.c is
commented out), but it is exposed in the public header and may be
revived for single-instance enforcement; tightening the default now
avoids carrying permissive bits forward.
… -> 0666)

Sonar c:S2612: create_listen_socket_unix() chmod'd the bound UNIX socket
file to 0777. For UNIX domain socket files only the read/write
permissions affect client connect() access -- the execute bit is unused.
0777 and 0666 are functionally identical for socket connect.

Both callers (web API and statsd) intentionally allow arbitrary local
clients to connect, so the broad rw permission is preserved with 0666.
The execute bit was misleading and unnecessary; remove it and update
the explanatory comment.
@sonarqubecloud

Copy link
Copy Markdown

@stelfrag
stelfrag marked this pull request as ready for review April 30, 2026 14:23
Copilot AI review requested due to automatic review settings April 30, 2026 14:23
@stelfrag
stelfrag marked this pull request as draft April 30, 2026 14:23
@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.

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Comment thread src/libnetdata/socket/listen-sockets.c Dismissed

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


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

Comment thread src/libnetdata/os/file_lock.c
Comment thread src/web/api/v1/api_v1_manage.c
@stelfrag
stelfrag marked this pull request as ready for review May 2, 2026 19:58

@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.

PR is working as expected on Linux. LGTM!

@stelfrag
stelfrag merged commit d6029b8 into netdata:master May 6, 2026
165 of 166 checks passed
@stelfrag
stelfrag deleted the cov_fix_202604_part3 branch May 6, 2026 21:42
@stelfrag stelfrag mentioned this pull request Jun 22, 2026
Ferroin pushed a commit that referenced this pull request Jul 15, 2026
* api: tighten management API key file permissions to 0600

Sonar c:S2612 (MAJOR vulnerability): the management API key file was
created with `open(..., O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 444)`. The
literal `444` is decimal, not octal, which equals octal 0674 = rw-rwxr--.
Group gets read+write+execute and others get read on a file that stores
the management API key (a UUID granting admin endpoint access). Anyone
with local read on the host can lift the key.

Even if the original intent was octal `0444` (world-readable), a
secret-key file should not be readable by group or others. Use `0600`
(owner read+write only), the standard mode for secrets.

* file_lock: tighten lock file creation mode from 0666 to 0600

Sonar c:S2612: file_lock_get() created the advisory lock file with
mode 0666 (rw-rw-rw-). Netdata runs as a single dedicated user, so
group/other access is unnecessary and adds latent risk if any local
user can interfere with the lock file.

flock(2) is purely advisory and does not enforce by mode, but the open()
permission still controls who can create/access the file. Owner-only
0600 keeps the locking behavior intact for the netdata user while
preventing unrelated local users from creating or opening the file.

The function is currently unused (its caller in src/daemon/main.c is
commented out), but it is exposed in the public header and may be
revived for single-instance enforcement; tightening the default now
avoids carrying permissive bits forward.

* listen-sockets: drop misleading exec bit from UNIX socket chmod (0777 -> 0666)

Sonar c:S2612: create_listen_socket_unix() chmod'd the bound UNIX socket
file to 0777. For UNIX domain socket files only the read/write
permissions affect client connect() access -- the execute bit is unused.
0777 and 0666 are functionally identical for socket connect.

Both callers (web API and statsd) intentionally allow arbitrary local
clients to connect, so the broad rw permission is preserved with 0666.
The execute bit was misleading and unnecessary; remove it and update
the explanatory comment.

---------

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants