feat: tedge diag collect security enhancement#3698
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
9954a42 to
152d74d
Compare
152d74d to
8b12358
Compare
| } | ||
|
|
||
| // extra metadata check when the process is running by the root user for the security | ||
| if get_current_uid() == 0 { |
There was a problem hiding this comment.
Do we really need the additional uzers crate to fetch the uid? Could have fetched it from MetadataExt, right?
There was a problem hiding this comment.
Do we really need the additional
uzerscrate to fetch the uid? Could have fetched it fromMetadataExt, right?
The point is to get the id of the current user, not the id of the file owner.
As two kinds of user id are used here, I would be explicit:
| if get_current_uid() == 0 { | |
| if uzers.get_current_uid() == 0 { |
There was a problem hiding this comment.
The point is to get the id of the current user, not the id of the file owner.
This is correct.
Changed as suggested in 8b7b787
| logger.warning(&format!("Skipping file: {path} (not owned by root)")); | ||
| return false; | ||
| } | ||
| if metadata.permissions().mode() & 0o022 != 0 { |
There was a problem hiding this comment.
I didn't really understand the reasoning behind this restriction. Even the one above. What if the normal user on the device created the script? Why reject that? Do we have similar restrictions for the software management plugins as well?
There was a problem hiding this comment.
The goal is to prevent a user from misleading the root user, i.e. to write a malware script that would be executed with admin privileges by sudo tedge diag collect.
Software management plugins are not executed directly by the agent but delegated to sudo.
| } | ||
|
|
||
| // extra metadata check when the process is running by the root user for the security | ||
| if get_current_uid() == 0 { |
There was a problem hiding this comment.
Do we really need the additional
uzerscrate to fetch the uid? Could have fetched it fromMetadataExt, right?
The point is to get the id of the current user, not the id of the file owner.
As two kinds of user id are used here, I would be explicit:
| if get_current_uid() == 0 { | |
| if uzers.get_current_uid() == 0 { |
If the process is run by root, the plugin must meet those conditions: * owned by root * not writable by other users Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>
8b7b787 to
72e4e95
Compare
Proposed changes
To enhance the security, if the process is run by root, the plugin must meet those conditions:
Otherwise, these files will be skipped.
(P.S. This is the last follow-up task for
tedge diag collect!)Types of changes
Paste Link to the issue
#3646
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments