8000 src/nbd: set groups of child process by omrisarig13 · Pull Request #1695 · rauc/rauc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

src/nbd: set groups of child process #1695

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.

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 2 commits into from
Apr 16, 2025

Conversation

omrisarig13
Copy link

When rauc is streaming a bundle during an installation operation, it creates a sub-process, and switches the user of this subprocess, to create e sandbox environment where this user have access only to the required resources in the system.

Until now, when this subprocess received the UID and GID of the streaming user, however, the groups of this user were not set accordingly.
This can cause problems when resources needed for the installation (e.g., installation keys) are available only through group permissions of the sandbox user.

This commit adds relevant logic to the child preparation and setup functions, where the code finds out which groups the user is a part of and sets them accordingly.

Setting the groups of the process must happen before changing the UID of the process, otherwise, the group setting operation will fail with permissions errors.

@omrisarig13
Copy link
Author
omrisarig13 commented Apr 11, 2025

In contrast to the uid and gid values of the child-process, the groups require allocation of dynamic memory (to avoid allocating the maximum value, which is unreasonably high). This means that the groups values of child_setup_args need to be freed. Currently, this is not done very elegantly.
The code is freeing the pointer in the parent process between the forking and the error validation, and is not being freed in the child (which we expect to be okay, as the child continue to "exec" shortly after).
This maybe can be solved in a better way, but I'm not familiar enough with the code-base to understand what the proper way to do it in rauc is.

@jluebbe
Copy link
Member
jluebbe commented Apr 16, 2025

In contrast to the uid and gid values of the child-process, the groups require allocation of dynamic memory (to avoid allocating the maximum value, which is unreasonably high). This means that the groups values of child_setup_args need to be freed. Currently, this is not done very elegantly. The code is freeing the pointer in the parent process between the forking and the error validation, and is not being freed in the child (which we expect to be okay, as the child continue to "exec" shortly after). This maybe can be solved in a better way, but I'm not familiar enough with the code-base to understand what the proper way to do it in rauc is.

You could define a G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC function for child_setup_args. Then you can use g_auto with it and don't need to handle the contents seprately.

@jluebbe jluebbe added the enhancement Adds new functionality or enhanced handling to RAUC label Apr 16, 2025
@omrisarig13 omrisarig13 force-pushed the omsa/add-groups-to-child-process branch from 3b1313b to e9618b0 Compare April 16, 2025 10:49
@omrisarig13
Copy link
Author

In contrast to the uid and gid values of the child-process, the groups require allocation of dynamic memory (to avoid allocating the maximum value, which is unreasonably high). This means that the groups values of child_setup_args need to be freed. Currently, this is not done very elegantly. The code is freeing the pointer in the parent process between the forking and the error validation, and is not being freed in the child (which we expect to be okay, as the child continue to "exec" shortly after). This maybe can be solved in a better way, but I'm not familiar enough with the code-base to understand what the proper way to do it in rauc is.

You could define a G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC function for child_setup_args. Then you can use g_auto with it and don't need to handle the contents seprately.

Thanks, I didn't know about this option, I've implemented it accordingly now.
If I haven't missed anything, to use this macros, the object must be referred to as a single word, so I've prepended a commit that adds this typedef.

@omrisarig13 omrisarig13 requested a review from jluebbe April 16, 2025 10:58
Omri Sarig added 2 commits April 16, 2025 13:04
Originally, the struct of child_setup_args did not have any typedef, and
was always used as "struct child_setup_args".

In the future, this struct will be surrounded by gmacro macros, to
handle memory management of the fields in this struct. The gmacro macros
are using the type name of the given object when creating their own
objects through these macros.
To make it possible for an object to have these macros, the type of the
object must be constructed of a single word, otherwise, the gmacros will
break by the space in the type name.

The current commit adds the typedef to the struct definition, and
updates all the usage in the code from "struct child_setup_args" to be
just "child_setup_args".

Signed-off-by: Omri Sarig <omri.sarig@prevas.dk>
When rauc is streaming a bundle during an installation operation, it
creates a sub-process, and switches the user of this subprocess, to
create e sandbox environment where this user have access only to the
required resources in the system.

Until now, when this subprocess received the UID and GID of the
streaming user, however, the groups of this user were not set
accordingly.
This can cause problems when resources needed for the installation
(e.g., installation keys) are available only through group permissions
of the sandbox user.

This commit adds relevant logic to the child preparation and setup
functions, where the code finds out which groups the user is a part of
and set them accordingly.

Setting the groups of the process must happen before changing the UID of
the process, otherwise, the group setting operation will fail with
permissions errors.

Signed-off-by: Omri Sarig <omri.sarig@prevas.dk>
@jluebbe jluebbe force-pushed the omsa/add-groups-to-child-process branch from e9618b0 to 109dfa6 Compare April 16, 2025 11:05
@jluebbe
Copy link
Member
jluebbe commented Apr 16, 2025

Force-pushed to align the commit titles with our convention (no capitalization).

@jluebbe
Copy link
Member
jluebbe commented Apr 16, 2025

In the commit message, you write:

Until now, when this subprocess received the UID and GID of the
streaming user, however, the groups of this user were not set
accordingly.
This can cause problems when resources needed for the installation
(e.g., installation keys) are available only through group permissions
of the sandbox use

With installation keys, you mean private keys for TLS client authentication, right? The subprocess shouldn't really need to access much else...

Copy link
codecov bot commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.

Project coverage is 84.45%. Comparing base (49ecc68) to head (109dfa6).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
src/nbd.c 63.63% 8 Missing ⚠️

❌ Your patch status has failed because the patch coverage (63.63%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff   
8000
          @@
##           master    #1695      +/-   ##
==========================================
- Coverage   84.47%   84.45%   -0.02%     
==========================================
  Files          76       76              
  Lines       22199    22219      +20     
==========================================
+ Hits        18752    18766      +14     
- Misses       3447     3453       +6     
Flag Coverage Δ
service=false 80.92% <63.63%> (-0.02%) ⬇️
service=true 84.41% <63.63%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@omrisarig13
Copy link
Author

In the commit message, you write:

Until now, when this subprocess received the UID and GID of the
streaming user, however, the groups of this user were not set
accordingly.
This can cause problems when resources needed for the installation
(e.g., installation keys) are available only through group permissions
of the sandbox use

With installation keys, you mean private keys for TLS client authentication, right? The subprocess shouldn't really need to access much else...

Yes.

In my specific case, I have a private key (for TLS client authentication) that is stored in a TPM, so the user needs to be part of the 'tss' group to be able to access the relevant devices, so it's a bit more than just a file, but in essence, it is only for the TLS client private-key access.

@jluebbe jluebbe changed the title src/nbd: Set groups of child process src/nbd:set groups of child process Apr 16, 2025
@jluebbe jluebbe merged commit 66b26cf into rauc:master Apr 16, 2025
18 of 19 checks passed
@jluebbe
Copy link
Member
jluebbe commented Apr 16, 2025

Thanks for your contribution!

@jluebbe jluebbe changed the title src/nbd:set groups of child process src/nbd: set groups of child process Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds new functionality or enhanced handling to RAUC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0