Potential security issue with libbd_fs2

[luc14n0]

After building libblockdev 2.26 with open build service RPMLINT issues this report:

libbd_fs2.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib64/libbd_fs.so.2.0.0
This executable is calling setuid and setgid without setgroups or initgroups.
This means it didn't relinquish all groups, and this would be a potential
security issue.

From what I understand, this is almost only a concern when we are permanently dropping privileges. So, should we be worried about this error or can we safely ignore this one?

2021-10-14 - Luciano Santos.

[vojtechtrefny]

I think this shouldn't be an issue for us, we are dropping the privileges only to run mount so it's not permanent. Libmount (util-linux) has the same issue and they added the set_groups call in https://github.com/util-linux/util-linux/commit/17fc8693cd60733227204fe31395575778984e3c only to remove it month later in https://github.com/util-linux/util-linux/commit/54cb0dd60ca3768fffe70d62400936bf387389bc because it caused issue with some mount operations. I don't think the libblockdev code will be used for network mounts, but I don't want to risk introducing some regressions.

Thoughts @tbzatek?

[luc14n0]

I see, it's only to run mount. Since it only is a temporary relinquishing I'm now more at piece with that rpmlint warning.

As for Libmount's regression, from what I've seen it can be a pain to manage temporary privilege drops depending on the situation. But the developer there implemented the temporary privilege drop the wrong way, he didn't "save" the privileges before calling setgroups( ), so when they tried to restored them it end up incomplete. There's a brief discussion about implementing drop of privileges in this O'Reilly's Cookbook in a secure fashion, it's worth a reading. If only I were a C programmer I'd help you guys hardening LibBlockDev's code.

Would you mind telling me how this temporary privilege drop to run mount is being implemented here?

[vojtechtrefny]

Would you mind telling me how this temporary privilege drop to run mount is being implemented here?

We fork ourselves and use setresuid and setregid to set the real UID/GID and run the mount (or unmount) operation, see https://github.com/storaged-project/libblockdev/blob/master/src/plugins/fs/mount.c#L434

[luc14n0]

We fork ourselves and use setresuid and setregid to set the real UID/GID and run the mount (or unmount) operation, see https://github.com/storaged-project/libblockdev/blob/master/src/plugins/fs/mount.c#L434

Thank you @vojtechtrefny for the answer. I see! Now I have two more questions that are eating me:

1. So, I've spent some time reading some chunks of mount.c and saw that set_uid() is a static function, plus the fact that RPM Lint is complaining about FS Plugin makes me think mount.c is one of FS's "components", I don't know what to call them, am I right about that?

2. Another thing about set_uid(), it's being called only once to drop the privileges. Since you said it's a temporary drop, how does is re-elevate them back? Or did you mean "temporary" because the child process gets killed at some point?

[vojtechtrefny]

1. So, I've spent some time reading some chunks of mount.c and saw that set_uid() is a static function, plus the fact that RPM Lint is complaining about FS Plugin makes me think mount.c is one of FS's "components", I don't know what to call them, am I right about that?

Yes, everything in src/plugins/fs is a single plugin just split into multiple files (other plugins are just header+implementation in src/plugins).

2. Another thing about set_uid(), it's being called only once to drop the privileges. Since you said it's a temporary drop, how does is re-elevate them back? Or did you mean "temporary" because the child process gets killed at some point?

Yes, the child process is created just to run the single operation so we don't elevate the privileges back. This starts with either bd_fs_mount or bd_fs_unmount, these then call run_as_user which creates the child process, drops the privileges and call do_mount/do_unmount to actually perform the (un)mount operation and the child process exists immediately after the (un)mount finishes.

[luc14n0]

Yes, everything in src/plugins/fs is a single plugin just split into multiple files (other plugins are just header+implementation in src/plugins).

Yeah, duh! Makes sense, I don't know where my head was when I asked that.

Yes, the child process is created just to run the single operation so we don't elevate the privileges back. This starts with either bd_fs_mount or bd_fs_unmount, these then call run_as_user which creates the child process, drops the privileges and call do_mount/do_unmount to actually perform the (un)mount operation and the child process exists immediately after the (un)mount finishes.

Currently, setresuid()/setresgid() are being used, can you tell me why make use of such nonstandard calls? (And do you know if any BSD is using LibBlockDev?) Because that could allow the process to regain root privileges, as per their manual page:

setresuid() sets the real user ID, the effective user ID, and the saved set-user-ID of the calling process.

Unprivileged user processes may change the real UID, effective UID, and saved set-user-ID, each to one of: the current real UID, the current effective UID or the current saved set-user-ID.

We are only setting the real user ID, the effective ID and saved set-user-ID remain untouched, and since we're not checking them maybe a door for an exploit is there, allowing someone to re-elevate the privileges and do God knows what:

• if (setresuid (uid, -1, -1) != 0); and
• if (setresgid (gid, -1, -1) != 0)

We know when it comes to C and security, being a bit paranoid is a good thing.

If I'd tweak the code to tighten it and make it more secure, even using setgroups() the right way, would you guys be interested? If there's a concern that in the future a regain of privileges might be necessary, who knows what the future holds! I could tweak the code to allow that as well, just as I already pointed at the O'Reilly's Cookbook.

Adding an option whether the drop will be only temporary or permanent would be interesting in any case, in my humble opinion, and a little check if the changes were successful wouldn't hurt too.

[vojtechtrefny]

Currently, setresuid()/setresgid() are being used, can you tell me why make use of such nonstandard calls?

I don't remember why we use this, but there was a reason for that.

(And do you know if any BSD is using LibBlockDev?)

No, I'd say libblockdev is a Linux-specific library simply because of the technologies we support.

If I'd tweak the code to tighten it and make it more secure, even using setgroups() the right way, would you guys be interested?

Yes, definitely. It's always better to be more secure. The test coverage we have for mounting should be good enough (together with UDisks test suite) to make sure we're not causing any regressions.

[vojtechtrefny]

Btw. one way to make this safer would be to simply refuse to change UID/GID when running as a non-root user. I actually thought we are already doing that, but we aren't. We do not expect that people will run libblockdev without root privileges -- primary users are UDisks (which is a daemon that runs as root) and Blivet (Python storage used exclusively from applications that run as root) and most of the libblockdev functionality simply won't work when running not running as root (even most "read only" operations with LVM and partitions don't work).

[luc14n0]

I don't remember why we use this, but there was a reason for that.

OK. If you remember, or anyone else, please tell me because I can't think of a reason for it's use.

Yes, definitely. It's always better to be more secure. The test coverage we have for mounting should be good enough (together with UDisks test suite) to make sure we're not causing any regressions.

Oh?! I wasn't aware of them. I'm gonna build and run them to get familiarized with them.

Btw. one way to make this safer would be to simply refuse to change UID/GID when running as a non-root user.

That's a good idea.

... most of the libblockdev functionality simply won't work when not running as root (even most "read only" operations with LVM and partitions don't work).

Hmmhm! Another reason for us to make sure we're doing everything in our power to run it as safe as we can.

---

So I'm gonna start using G.H.'s gist to lay out the design of the functions, and how that piece of the code should look like, more or less. It's easier to handle and more dynamic while we sort out some details. After we get something solid I'll implement the changes, run tests, etc, to get it ready for merging.

And for clarity purposes, let me state that I'll work on this as much as my contributor time allows me, I have another couple of projects that I help too, but right now we're not really busy. It's a good thing that this piece of code I'm gonna touch is not really huge. Alright let's roll up our sleeves.

[vojtechtrefny]

I don't remember why we use this, but there was a reason for that.

OK. If you remember, or anyone else, please tell me because I can't think of a reason for it's use.

We (and by "we" I mean "I") should've documented this better. I'm still not sure I fully understand the reason for using setresuid and I now even question whether the code makes sense and does what I thought it should be doing, but the primary reason for using it is to make the libmount context restricted (see mnt_context_is_restricted) which triggers some additional checks in libmount (for example allowed mount options). I need to spend some more time on this, but it looks like we should at least document this behaviour better, because this is not primarily about running the mount operation as a different user.

One think is clear -- dropping privileges with setuid doesn't work, with it we can't mount a device specified in /etc/fstab in with the user/users option. Which makes sense, mount is a SUID binary and doesn't drop privileges to run mount for fstab devices so we are most likely using setresuid to mimic the situation where a user calls the binary without sudo. I remember dealing with this when originally writing the code and I can't decide whether I made a mistake back then or just documented it badly and named the functions in a way it doesn't really make sense.

I think hardening this code is still a good idea, but maybe don't spend your time on this right now, we might end up rewriting it or changing some parts of it. As I said, I plan to spend some more time on this, and I hope the only outcome will be better documenting what the run_as_user function is supposed to do in context of running bd_fs_mount.

Sorry for complicating things even more.

[tbzatek]

FWIW, similar case in udisks, however the use case is slightly different, spawning general commands: https://github.com/storaged-project/udisks/blob/master/src/udisksspawnedjob.c#L392

And I remember a related issue in udisks where supplemental groups were dropped and a mount helper couldn't access the block device that was accessible to that supplemental group we've dropped. Something to review one day thoroughly...