Drop privileges after startup #364

ansasaki · 2022-04-25T18:53:30Z

After finishing operations that require privileges, drop the privileges to run as a normal user.

This adds support to the run_as configuration option to select the user:group under which the keylime agent will run.

This also rewrites check_mount() to use the information from /proc/self/mountinfo instead of the mount output file to check if secure is mounted. The implementation was based on python agent.

Fixes: #343, #317, #344

src/main.rs

src/permissions.rs

src/secure_mount.rs

ansasaki · 2022-04-28T12:19:30Z

src/keys_handler.rs

+        let mut fixture = QuoteData::fixture().unwrap(); //#[allow_ci]
+
+        // Create temporary working directory and secure mount
+        let temp_workdir = tempfile::tempdir().unwrap(); //#[allow_ci]
+        fixture.secure_mount =
+            PathBuf::from(&temp_workdir.path().join("tmpfs-dev"));
+        fs::create_dir(&fixture.secure_mount).unwrap(); //#[allow_ci]


I added this because when the secure mount path is passed directly to run_encrypted_payload(), the directory is expect to exist (since the secure mount should be ready from the beginning).

In the previous implementation the secure_mount would be created by secure_mount::mount(). It took me some time to understand what was happening to make the test to fail.

ueno · 2022-05-05T14:11:19Z

src/ima.rs

@@ -57,14 +58,16 @@ impl ImaMeasurementList {
 /// was read and the current number of entries in the file.
 pub(crate) fn read_measurement_list(
    ima_ml: &mut ImaMeasurementList,
-    filename: &Path,
+    ima_file: &Option<Arc<Mutex<File>>>,


Could you explain why Mutex is necessary here? Is it possible that the file is accessed from multiple threads?

In case multiple requests for quotes are received in parallel the file would be needed in multiple threads, wouldn't it?

Yeah, I understand that, but in that case I wonder if it might make more sense to handle locking in quotes_handler.rs (and if it's not feasible at least have some comment in ima.rs why this interface involves locking)?

Yeah, I understand that, but in that case I wonder if it might make more sense to handle locking in quotes_handler.rs (and if it's not feasible at least have some comment in ima.rs why this interface involves locking)?

I have to take some time and think if there is a better way and if your suggestion is feasible. Right now I can't see another way to open a file before dropping the privileges and later share the handle in multiple threads without using a mutex on the handle. That would be possible if I can clone the handle in each thread and operate on them independently. The content of the file is accessed only for reading, but the handle is modified by seek operations. I need to test this.

Right now I can't see another way to open a file before dropping the privileges and later share the handle in multiple threads without using a mutex on the handle.

I'm not saying that using a mutex is a problem, but suggest calling .lock() in integrity (in quotes_handler.rs) rather than read_measurement_list (in ima.rs). The lock for the other argument (ima_ml) is already implemented that way.

Here is my attempt (totally untested).

That would be possible if I can clone the handle in each thread and operate on them independently.

I am not sure why this mechanism is necessary. Maybe I am missing something but afaics the only code path that could access the file object is integrity → read_measurement_list, which is even protected by ima_ml.lock() so there is only one thread that could take this path.

The content of the file is accessed only for reading, but the handle is modified by seek operations.

That makes me think that the handle could be generalized as Read + Seek, but maybe overkill :-)

Ok, I think I understand now what you were asking. I'll try you approach and move the .lock().

Modified as suggested. I followed the same approach for the measured boot log file.

ueno · 2022-05-06T14:42:55Z

dist/systemd/system/installer.sh

+# prepare keylime service files and store them in systemd path
+sed "s|KEYLIMEDIR|$KEYLIMEDIR|g" $BASEDIR/keylime_agent.service.template > /etc/systemd/system/keylime_agent.service
+
+# Copy secure mount


Why not extend the existing GNUmakefile and use install rather than manual calls to chmod and chown?

Because I did it without thinking much :)
I will make the changes in the Makefile.

Removed the installer script completely and added minimal changes to the GNUmakefile.

ueno

Thanks for the update; looks much simpler.

src/ima.rs

ueno · 2022-05-12T03:33:38Z

src/ima.rs

@@ -57,25 +58,18 @@ impl ImaMeasurementList {
 /// was read and the current number of entries in the file.
 pub(crate) fn read_measurement_list(


I tend to think we could make this function as a method of ImaMeasurementList, but it could be a future refactoring.

I agree, I'll make the change in a separate PR.

Open files that require root privilege in the beginning of the startup process and pass the handles down instead of using the file paths. This is a preparation for dropping the privileges after opening the files. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Add support for the 'run_as' configuration option in 'cloud_agent' section. After executing the operation that require privilege, drop the privileges and run as the user set by the 'run_as' option. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

This is to allow the agent to use files possibly created by privileged processes. Otherwise the agent will try to set the file ownership and fail. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Follow the python implementation and parse the /proc/self/mountinfo content instead of parsing the mount command output. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

This also updates the agent service to add the secure mount service as a requirement and to enable logging through service environment variable. Fixes keylime#344 Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

The shim.py is used to run python revocation action scripts loaded as modules. This is for backward compatibility with the python agent implementation. Support for python revocation scripts was introduced in 73773e6 Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ueno · 2022-05-16T17:13:25Z

@ansasaki can we merge this, or do you plan any further update?

ansasaki · 2022-05-16T17:24:37Z

@ansasaki can we merge this, or do you plan any further update?

I think we can merge, I don't plan to add more changes here.

keylime-bot requested review from ueno, puiterwijk and lukehinds April 25, 2022 18:53

keylime-bot assigned lukehinds, puiterwijk and ueno Apr 25, 2022

ansasaki changed the title ~~WIP: Drop privileges after startup~~ Drop privileges after startup Apr 26, 2022

ansasaki force-pushed the drop_privileges branch from 9f89234 to df2b275 Compare April 26, 2022 15:46

ueno reviewed Apr 26, 2022

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/permissions.rs Outdated Show resolved Hide resolved

src/secure_mount.rs Outdated Show resolved Hide resolved

ansasaki force-pushed the drop_privileges branch 5 times, most recently from 0c9168f to 1e32557 Compare April 28, 2022 11:45

ansasaki commented Apr 28, 2022

View reviewed changes

ansasaki force-pushed the drop_privileges branch from 1e32557 to ce156a3 Compare May 4, 2022 10:00

lkatalin mentioned this pull request May 4, 2022

Roadmap for official Rust agent keylime/keylime#986

Closed

21 tasks

ueno reviewed May 5, 2022

View reviewed changes

ueno reviewed May 6, 2022

View reviewed changes

ansasaki force-pushed the drop_privileges branch 3 times, most recently from d6f2ddf to 2da579c Compare May 11, 2022 12:44

ueno approved these changes May 12, 2022

View reviewed changes

keylime-bot unassigned ueno May 12, 2022

ansasaki added 6 commits May 12, 2022 17:15

main: Mount secure mount before dropping the privileges

cd97b31

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

main: Ignore original ownership when unzipping files

dd645d6

This is to allow the agent to use files possibly created by privileged processes. Otherwise the agent will try to set the file ownership and fail. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

secure_mount: Rewrite check_mount()

e5cc009

Follow the python implementation and parse the /proc/self/mountinfo content instead of parsing the mount command output. Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

secure_mount: Do not try to give ownership to root

49f886f

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki added 2 commits May 12, 2022 17:15

Add service for secure mount

e30578e

This also updates the agent service to add the secure mount service as a requirement and to enable logging through service environment variable. Fixes keylime#344 Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>

ansasaki force-pushed the drop_privileges branch from 2da579c to c9ab9cb Compare May 12, 2022 15:16

ueno merged commit df988fc into keylime:master May 16, 2022

ansasaki deleted the drop_privileges branch May 25, 2022 14:33

ansasaki mentioned this pull request May 31, 2022

Use /proc/self/mountinfo instead of mount for checking secure mount point #317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop privileges after startup #364

Drop privileges after startup #364

ansasaki commented Apr 25, 2022 •

edited

Loading

ansasaki Apr 28, 2022

ueno May 5, 2022

ansasaki May 5, 2022

ueno May 5, 2022

ansasaki May 6, 2022

ueno May 6, 2022

ansasaki May 6, 2022

ansasaki May 10, 2022

ueno May 6, 2022

ansasaki May 6, 2022

ansasaki May 10, 2022

ueno left a comment

ueno May 12, 2022

ansasaki May 12, 2022

ueno commented May 16, 2022

ansasaki commented May 16, 2022

		@@ -57,25 +58,18 @@ impl ImaMeasurementList {
		/// was read and the current number of entries in the file.
		pub(crate) fn read_measurement_list(

Drop privileges after startup #364

Drop privileges after startup #364

Conversation

ansasaki commented Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueno commented May 16, 2022

ansasaki commented May 16, 2022

ansasaki commented Apr 25, 2022 •

edited

Loading