Skip to content

[Heartbeat] Add prctl dumpable flag reset after cap drop #38269

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 14, 2024
Merged

[Heartbeat] Add prctl dumpable flag reset after cap drop #38269

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
92dd998
Add prctl dumpable flag reset after cap drop
emilioalvap Mar 12, 2024
e8bf476
Add changelog
emilioalvap Mar 12, 2024
180bd6b
Address feedback
emilioalvap Mar 13, 2024
efef83a
Merge branch 'main' into fix-hb-proc-io
vigneshshanmugam Mar 14, 2024
6d2e844
Merge branch 'main' into fix-hb-proc-io
emilioalvap Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ fields added to events containing the Beats version. {pull}37553[37553]
- Fix panics when parsing dereferencing invalid parsed url. {pull}34702[34702]
- Fix setuid root when running under cgroups v2. {pull}37794[37794]
- Adjust State loader to only retry when response code status is 5xx {pull}37981[37981]
- Reset prctl dumpable flag after cap drop. {pull}38269[38269]

*Metricbeat*

Expand Down
14 changes: 14 additions & 0 deletions heartbeat/security/security.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strconv"
"syscall"

"golang.org/x/sys/unix"
"kernel.org/pub/linux/libs/security/libcap/cap"
)

Expand All @@ -46,6 +47,9 @@ func init() {
// The beat should use `getcap` at a later point to examine available capabilities
// rather than relying on errors from `setcap`
_ = setCapabilities()

// Make heartbeat dumpable so elastic-agent can access process metrics.
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is this applicable only when

  • We are not running as root, common in context of container (BEAT_SETUID_AS) and under elastic-agent.
  • We effectively switched the user id and group id which changed the dumpable flag to value other than 1 (restircting access).

With that said, Should we enforce this change only on containerized environment? I assume there is no harm in overriding the dumpable flag even if its already 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume there is no harm in overriding the dumpable flag even if its already 1?

There shouldn't be, there's no issue with overriding and in case of error, kernel returns an error code we can safely ignore since the agent is ready to handle those cases now.

We are not running as root, common in context of container (BEAT_SETUID_AS) and under elastic-agent.
We effectively switched the user id and group id which changed the dumpable flag to value other than 1 (restircting access).

Not exactly, elastic-agent image is already shipped with file capabilities enabled so it doesn't require BEAT_SETUID_AS mechanism to kick in, see this point in prctl:

 •  The process executes ([execve(2)](https://man7.org/linux/man-pages/man2/execve.2.html)) a program that has
    file capabilities (see [capabilities(7)](https://man7.org/linux/man-pages/man7/capabilities.7.html)), but only if
    the permitted capabilities gained exceed those already
    permitted for the process.

Although changing effective uid/gid also triggers disabling dumpable flag.

Should we enforce this change only on containerized environment?

Container detection can be unreliable in some cases. Also, I can't think of a situation where this would broaden the permission scope for a non-containerized instance (running users already have access to config files). But I'm happy to discuss alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, Thanks for the extra context. I agree with the argument here on container detection and permission scope. I think the workaround looks good as it is.

_ = setDumpable()
}

func setNodeProcAttr(localUserName string) error {
Expand Down Expand Up @@ -99,3 +103,13 @@ func setCapabilities() error {

return nil
}

// Enforce PR_SET_DUMPABLE=true to allow user-level access to /proc/<pid>/io.
func setDumpable() error {
_, err := cap.Prctl(unix.PR_SET_DUMPABLE, 1)
if err != nil {
return fmt.Errorf("error setting dumpable flag via prctl: %w", err)
}

return nil
}