You know, I was going to say that its OK, as it’s a handful of unsafe blocks around FFI, its comparatively easy to just eyeball them, and, unless you go full CHERI, unsafety has to live somewhere.
And I think that’s a bug and technically UB! CString::new allocates, and you can’t allocate in pre-exec.
So kinda no maybe?
But also maybe yes, or at least much better than alternatives? This bug really jumps out at me, it’s trivial to notice for someone unfamiliar with the code. pre_exec is tricky, and unsafe makes it really stick out, much better then if it were hidden in the guts of the standard library or some native extension.
(also, obligatory PSA: the biggest problem with sudo is not memory unsafety per se, but just the vast scope for this security sensitive utility. For personal use, you can replace sudo with doas, and it’ll probably make a bigger dent in insecurity. If you have to be API and feature-compatible with sudo though, then, yes, what Ferrous folks are doing makes most sense to me)
I often see doas recommended as simpler than sudu. When I compare the documentation I see an almost identical feature set. The only difference in security surface are seems to be the configuration. Is there more that sudo does that doas does not do?
That’s also the funny thing — I don’t actually know what sudo does. I know that OpenDoas clocs at under 5k lines of code, while sudo is a lot more (see the nearby comment by andyc). So, that’s a non-constructive proof that it does something extra!
You’re allowed to allocate in pre-exec. The restrictions the documentation mentions mostly stem from other threads potentially holding locks or having been mid-modification of the env variables.
If you guarantee that there is no threads (and other pre-conditions like reentrancy) running at the time of fork(), you can in fact malloc without any big issues.
I think in case of Rust it’s actually quite murky technically:
First, Rust doesn’t have assert_no_thread!() functionality, so, if you assume at pre_exec time that the program is single-threaded, the calling function should be marked as unsafe with the safety precondition of “must be single threaded”. Which mostly boils down to just “don’t allocate” in practice.
Second, allocation in Rust calls #[global_allocator], which is arbitrary user code. As a general pattern, when you call arbitrary code from within unsafe blocks, there usually is some Rube Goldberg contraption which ends with a shotgun aimed at your feet. That is, if the user tags their own static as a global allocator, they can call various API on that static directly, and that should be enough to maneuver the thing into “technically UB” even without threads. In particular, you could smuggle something like https://github.com/rust-lang/rust/issues/39575#issuecomment-437658766 that way.
But yeah, this is quite subtle, there was some debate whether before_exec needs to be unsafe at all, and I personally am not clear as to what’s the safety contract of before_exec actually is, in terms of Rust APIs.
To be fair, pre_exec is an unsafe function for precisely this reason and it should stay that way. The API contract is largely unspecified because fork() does some amazing things to program state. I think the solution here would be to swap in a CStr over a CString, to avoid the allocation.
edit: One way we could avoid it is by introducing a new unsafe trait; Interruptable. It must be explicitly declared on structs. The trait would simply declare the struct and it’s functions reentrancy safe. A function is automatically interruptible if all non-local data it uses is also interruptible. Then PreExec could simply require the function to also be Interruptable.
To be fair, pre_exec is an unsafe function for precisely this reason and it should stay that way. The API contract is largely unspecified because fork() does some amazing things to program state.
It looks as if it’s intended as an abstraction over different process-creation things and fork and vfork do different amazing things to process state.
Fork creates a copy of the address space and file descriptor table, but that copy has only the current thread in it. If other threads are holding locks, can cannot acquire those locks without deadlocking. You need to register pre-fork hooks to drop them (typically, the pre-fork hook should acquire the locks in the prepare stage and then drop them in the child, it should also try to guarantee consistent state in both). It is unsafe to call malloc from a multithreaded program between fork and execve because it may deadlock with a not-copied thread.
Vfork creates a copy of the file descriptor table but does not create a copy of the address space. You can modify the file-descriptor table until you execve and then you effectively longjmp back to the vfork call (I believe this is actually how Cygwin implements vfork). Because the code in the vfork context is just normal code, it is safe to call malloc there, but anything you don’t free will leak.
This is the main reason that I prefer using vfork: I can use RAII in my setup code and, as long as I reach the end of the scope before calling execve, everything is fine.
This is often a very constrained environment where normal operations like malloc, accessing environment variables through std::env or acquiring a mutex are not guaranteed to work
Which doesn’t read like memory allocations are forbidden in that closure to me.
Ninja edit: To me, it reads like if e.g: your program is single-threaded then you’re fine.
Yes, that rule is mostly about multithreaded environments. And violations usually don’t result in memory corruption but in deadlocks.
The reason is that after forking, you only have the current thread, all the other ones are “frozen” in time. So if you forked while some other thread held a lock inside malloc and then call malloc yourself, you can deadlock.
And also, glibc in particular has special code to make this work, so you can indeed malloc after fork there safely.
So IMO this is POSIX UB and therefore Rust UB by fiat, but very likely not a security issue in practice. It should be fixed, but it’s not super alarming to me.
You definitely can replace sudo with doas… unless you run a RHEL or a clone. I have two Rocky machines and my Ansible does not coexist with the RHEL clones as compared to FreeBSD and my myriad of other operating systems.
I have effectively replaced sudo with doas in all situations except for those platforms.
unsafe does not necessarily mean it does not have memory safety, but that that code needs more scrutiny. I am curious why there aren’t SAFETY comment blocks on each instance of unsafe to explain the invariants that makes it safe or necessary.
Most of them look pretty clearly justified, although it’d still be helpful to have an argument why they’re correct. The unsafe blocks I looked at are all used to interact with unsafe APIs, which is sort of a given for this sort of program, but they’re short and at first glance don’t seem to do anything that would be hard to reason about.
This would mean that the whole titular claim, that this is the “first stable release of a memory safe sudo implementation”, is probably wrong. C code can also “have memory safety”, so the first stable release of a memory safe sudo implementation is probably (at least some version of) sudo itself.
It might be my own failing, but I’m having trouble understanding your comment. That the “memory safe version of sudo itself has no bugs leading to memory errors” seems like a tautology.
If you mean we don’t know if there’s any such version, that’s technically true, and why I said “probably”. It seems pretty likely, though, and it’s certainly at least possible, bar any proof otherwise.
The point is that it doesn’t seem likely to me that sudo, as a reasonably large program in a language with few safety features, has no more such bugs. Sudo has had memory safety bugs in the past, and if I had to guess, I’d expect more to emerge at some point. You may be able to prove that there’s no memory-safe version simply by waiting for that to happen.
Of course, if there is such a version, finding it and proving it to be safe may be considerably more challenging. Which is important in itself: its safety wouldn’t have much value to us unless we knew about it.
You may be able to prove that there’s no memory-safe version simply by waiting for that to happen.
That would show there is (was) a memory-error bug in some version(s), not all prior versions. And it’s all a bit hand-wavy; that we know that there were memory safety bugs in the past only shows that such bugs were found and fixed. From reading elsewhere sudo sounds like it’s more complex than I would’ve thought it should be, but it’s still not so big that it’s impossible that it is (or has been at some point) free of such bugs.
If you prefer, I can rephrase my point (making it slightly weaker): the linked article is claiming the “first stable release of a memory safe sudo implementation” has been made, referring to this Rust implementation, but there is no certainty that all prior implementations had memory-error bugs, so if we take “memory safe” to mean “has no memory-error bugs” then the claim is unproven and could be wrong. (It seems we can’t take “memory safe” to mean “written completely in a memory safe language”, since it apparently uses “unsafe”).
(As to how likely the claim is to be wrong, we may disagree, but there’s probably not much in the way of objective argument that we can make about it).
Even if you implement a program in a memory safe language you do not know for sure that it is memory safe.
You have a very strong reason to believe so, but there is always the possibility of a mistake in the type system design or the language implementation. After a few years the rust language has been addressed with formal methods that mathematically prove correctness for rust programs as well as specifically proving the correctness of various parts of the rust standard library that make use of unsafe blocks. This helps give very strong confidence about memory safety, but again there is the possibility of gaps.
I’m not sure how much of this gets configured on a typical Linux distro, but it looks pretty bloated. I was pretty surprised to count 175K total lines of *.[ch] in the tarball.
There are 60K lines of code in lib/ , which are some vendored deps. (Why the heck does sudo need a copy of zlib? Feels like a non-Unixy layering violation. Reminds me of looking at GNU grep and awk and sed and seeing the same 10K or 20K lines of regex engine copied and pasted with slight changes.)
Also, I really, really hope they are using some kind of privilege separation for parsing /etc/sudoers.
Having a 2118 line yacc file with 133 instances of free() in its semantic actions does not seem like great code.
DJB has written about avoiding parsing in low level utilities like this. So I hope this is not actually parsed in the main sudo process. A conceivable use of the libprotobuf I saw would be to parse in one process and then deserialize in another, but I don’t know if they’re doing that.
They also have a JSON parser in there. Again I really hope most of this stuff is not configured typically … and not in the main process. Kinda shocked at how big it is
If you want to know how sudo works in the “default” case (sudoers files), you can look at basically two files.
src/sudo.c has a main that is “just there”. Inside there there’s in particular the policy_check function. This replies on a plugin system
The “main sudo thing” people use is the sudoers file. That is in plugins/sudoers/. Check out verify_user inside of plugins/sudoers/check.c.
A lot of this code is pretty much straight down the line. Very little indentation or conditional paths. It’s honestly stuff that inspires confidence.
There are a lot of places where the fact that it’s C makes the code a bit more futzy. A lot of struct instatiations that look right but merely look it. But I think that implying sudo is a mess because it has a lot of lines of code in the whole project is not really painting a fair picture.
For ssh and userv, the privileged part of the system is run in a clean environment, so they cannot be confused by weird file descriptors, signal dispositions, environment variables, or other oddities that a setuid program can inherit from the unprivileged process that invoked it. It is very difficult to use setuid safely, so it is important to minimize complexity. But sudo is designed to be an all-purpose tool for jumping privilege boundaries with all sorts of complicated rules and restrictions. It’s fundamentally a bad idea.
In most (maybe all?) cases where I have seen sudo used in practice, none of that complexity is actually used. So many people say without thinking that you must not login as root, and use sudo instead; but then their user account becomes defacto equivalent to root, and they have not mitigated whatever unstated risks they might have been trying to avoid. The really command gets rid of all of the configurability, apart from a privileged group, so it is simply a safety guard for use with a non-root privileged account.
I have never understood the point of using sudo with ansible.
Ack for ssh and userv. Basically, instead of suid binary, we use a daemon which is spawned by the root.
For really, I didn’t get this. Its still basically the suid binary right? So its the same basic design as sudo, except that it has much smaller feature set. So it’s roughly the same thing as doas?
Like sudo, doas wants to be a swiss army knife: it can raise or lower privilege, it has a bespoke language for access control, etc. whereas really will only raise privilege and has one hardcoded access control rule. An account that can run really (much like most uses of sudo) is basically equivalent to root, but does not keep the footgun cocked at all times.
Thanks! It is indeed seems a fundamentally better approach for what I (and I expect many people) use doas / sudo for. Will switch to really as soon as someone else packages it for NixOS.
If you want to get a root shell, use really or ssh
On Linux, another option is to log in as root on the Linux kernel console, which I (naïvely) imagine makes it much more difficult for things running in one’s user session to snoop on one’s sensitive typing through X11.
Are memory safety exploits the problem with sudo? I thought it was mostly things like “unix environments have another environment variable that makes setuid processes load code” or logic errors - maybe Rust can help with those, but it’s not the right case they’re making.
I see a bunch of instances of
unsafein the codebase. Does it still count as memory safe?You know, I was going to say that its OK, as it’s a handful of unsafe blocks around FFI, its comparatively easy to just eyeball them, and, unless you go full CHERI, unsafety has to live somewhere.
But also, one of those blocks is not FFI:
https://github.com/memorysafety/sudo-rs/blob/9a7f38fbddc59f40a5e0a57555131b36e09811e4/src/exec/mod.rs#L92-L97
And I think that’s a bug and technically UB!
CString::newallocates, and you can’t allocate in pre-exec.So kinda no maybe?
But also maybe yes, or at least much better than alternatives? This bug really jumps out at me, it’s trivial to notice for someone unfamiliar with the code. pre_exec is tricky, and unsafe makes it really stick out, much better then if it were hidden in the guts of the standard library or some native extension.
(also, obligatory PSA: the biggest problem with sudo is not memory unsafety per se, but just the vast scope for this security sensitive utility. For personal use, you can replace
sudowithdoas, and it’ll probably make a bigger dent in insecurity. If you have to be API and feature-compatible with sudo though, then, yes, what Ferrous folks are doing makes most sense to me)I often see doas recommended as simpler than sudu. When I compare the documentation I see an almost identical feature set. The only difference in security surface are seems to be the configuration. Is there more that sudo does that doas does not do?
Generally when I think of sudo’s features where replacing it is somewhat difficult it is usually around plugins and things like storing rules in LDAP.
That’s also the funny thing — I don’t actually know what sudo does. I know that OpenDoas clocs at under 5k lines of code, while sudo is a lot more (see the nearby comment by andyc). So, that’s a non-constructive proof that it does something extra!
You’re allowed to allocate in pre-exec. The restrictions the documentation mentions mostly stem from other threads potentially holding locks or having been mid-modification of the env variables.
If you guarantee that there is no threads (and other pre-conditions like reentrancy) running at the time of fork(), you can in fact malloc without any big issues.
I think in case of Rust it’s actually quite murky technically:
First, Rust doesn’t have
assert_no_thread!()functionality, so, if you assume at pre_exec time that the program is single-threaded, the calling function should be marked asunsafewith the safety precondition of “must be single threaded”. Which mostly boils down to just “don’t allocate” in practice.Second, allocation in Rust calls
#[global_allocator], which is arbitrary user code. As a general pattern, when you call arbitrary code from withinunsafeblocks, there usually is some Rube Goldberg contraption which ends with a shotgun aimed at your feet. That is, if the user tags their own static as a global allocator, they can call various API on that static directly, and that should be enough to maneuver the thing into “technically UB” even without threads. In particular, you could smuggle something like https://github.com/rust-lang/rust/issues/39575#issuecomment-437658766 that way.But yeah, this is quite subtle, there was some debate whether
before_execneeds to beunsafeat all, and I personally am not clear as to what’s the safety contract ofbefore_execactually is, in terms of Rust APIs.To be fair, pre_exec is an unsafe function for precisely this reason and it should stay that way. The API contract is largely unspecified because fork() does some amazing things to program state. I think the solution here would be to swap in a CStr over a CString, to avoid the allocation.
edit: One way we could avoid it is by introducing a new unsafe trait; Interruptable. It must be explicitly declared on structs. The trait would simply declare the struct and it’s functions reentrancy safe. A function is automatically interruptible if all non-local data it uses is also interruptible. Then PreExec could simply require the function to also be Interruptable.
It looks as if it’s intended as an abstraction over different process-creation things and
forkandvforkdo different amazing things to process state.Fork creates a copy of the address space and file descriptor table, but that copy has only the current thread in it. If other threads are holding locks, can cannot acquire those locks without deadlocking. You need to register pre-fork hooks to drop them (typically, the pre-fork hook should acquire the locks in the prepare stage and then drop them in the child, it should also try to guarantee consistent state in both). It is unsafe to call
mallocfrom a multithreaded program betweenforkandexecvebecause it may deadlock with a not-copied thread.Vfork creates a copy of the file descriptor table but does not create a copy of the address space. You can modify the file-descriptor table until you
execveand then you effectivelylongjmpback to thevforkcall (I believe this is actually how Cygwin implementsvfork). Because the code in the vfork context is just normal code, it is safe to callmallocthere, but anything you don’tfreewill leak.This is the main reason that I prefer using
vfork: I can use RAII in my setup code and, as long as I reach the end of the scope before callingexecve, everything is fine.Looking at the documentation, it only mentions the following:
Which doesn’t read like memory allocations are forbidden in that closure to me.
Ninja edit: To me, it reads like if e.g: your program is single-threaded then you’re fine.
Yes, that rule is mostly about multithreaded environments. And violations usually don’t result in memory corruption but in deadlocks. The reason is that after
forking, you only have the current thread, all the other ones are “frozen” in time. So if you forked while some other thread held a lock insidemallocand then callmallocyourself, you can deadlock.And also, glibc in particular has special code to make this work, so you can indeed
mallocafterforkthere safely.So IMO this is POSIX UB and therefore Rust UB by fiat, but very likely not a security issue in practice. It should be fixed, but it’s not super alarming to me.
You definitely can replace sudo with doas… unless you run a RHEL or a clone. I have two Rocky machines and my Ansible does not coexist with the RHEL clones as compared to FreeBSD and my myriad of other operating systems.
I have effectively replaced sudo with doas in all situations except for those platforms.
unsafedoes not necessarily mean it does not have memory safety, but that that code needs more scrutiny. I am curious why there aren’tSAFETYcomment blocks on each instance of unsafe to explain the invariants that makes it safe or necessary.Most of them look pretty clearly justified, although it’d still be helpful to have an argument why they’re correct. The unsafe blocks I looked at are all used to interact with unsafe APIs, which is sort of a given for this sort of program, but they’re short and at first glance don’t seem to do anything that would be hard to reason about.
This would mean that the whole titular claim, that this is the “first stable release of a memory safe sudo implementation”, is probably wrong. C code can also “have memory safety”, so the first stable release of a memory safe sudo implementation is probably (at least some version of) sudo itself.
Yes, but the memory safe version of sudo itself has no bugs leading to memory errors. I don’t know if that version has been written yet.
It might be my own failing, but I’m having trouble understanding your comment. That the “memory safe version of sudo itself has no bugs leading to memory errors” seems like a tautology.
If you mean we don’t know if there’s any such version, that’s technically true, and why I said “probably”. It seems pretty likely, though, and it’s certainly at least possible, bar any proof otherwise.
The point is that it doesn’t seem likely to me that sudo, as a reasonably large program in a language with few safety features, has no more such bugs. Sudo has had memory safety bugs in the past, and if I had to guess, I’d expect more to emerge at some point. You may be able to prove that there’s no memory-safe version simply by waiting for that to happen.
Of course, if there is such a version, finding it and proving it to be safe may be considerably more challenging. Which is important in itself: its safety wouldn’t have much value to us unless we knew about it.
That would show there is (was) a memory-error bug in some version(s), not all prior versions. And it’s all a bit hand-wavy; that we know that there were memory safety bugs in the past only shows that such bugs were found and fixed. From reading elsewhere sudo sounds like it’s more complex than I would’ve thought it should be, but it’s still not so big that it’s impossible that it is (or has been at some point) free of such bugs.
If you prefer, I can rephrase my point (making it slightly weaker): the linked article is claiming the “first stable release of a memory safe sudo implementation” has been made, referring to this Rust implementation, but there is no certainty that all prior implementations had memory-error bugs, so if we take “memory safe” to mean “has no memory-error bugs” then the claim is unproven and could be wrong. (It seems we can’t take “memory safe” to mean “written completely in a memory safe language”, since it apparently uses “unsafe”).
(As to how likely the claim is to be wrong, we may disagree, but there’s probably not much in the way of objective argument that we can make about it).
Even if you implement a program in a memory safe language you do not know for sure that it is memory safe.
You have a very strong reason to believe so, but there is always the possibility of a mistake in the type system design or the language implementation. After a few years the rust language has been addressed with formal methods that mathematically prove correctness for rust programs as well as specifically proving the correctness of various parts of the rust standard library that make use of unsafe blocks. This helps give very strong confidence about memory safety, but again there is the possibility of gaps.
I don’t disagree. (Or at least, I’m not arguing any of those points).
No.
FWIW I looked at the latest release of the C sudo, to see how much code it is - https://www.sudo.ws/releases/stable/
I’m not sure how much of this gets configured on a typical Linux distro, but it looks pretty bloated. I was pretty surprised to count 175K total lines of
*.[ch]in the tarball.There are 60K lines of code in lib/ , which are some vendored deps. (Why the heck does sudo need a copy of zlib? Feels like a non-Unixy layering violation. Reminds me of looking at GNU grep and awk and sed and seeing the same 10K or 20K lines of regex engine copied and pasted with slight changes.)
Excluding that lib/ dir, there are 115 K lines of code! Biggest files are parsers generated from yacc for /etc/sudoers.
Excluding both lib/ and plugin/ dirs, there are still 40K lines of code. Including an 880 line file that parses flags.
And 876 lines of src/net_ifs.c which lists network interfaces or something.
I guess TIL I have no idea how sudo works :-( :-( :-(
The Rust version clocked in at 15K lines in the repo, but I didn’t look at dependencies. Not sure how much feature parity they have.
I’d be interested in knowing how much code are in FreeBSD or NetBSD sudo.
Also, I really, really hope they are using some kind of privilege separation for parsing /etc/sudoers.
Having a 2118 line yacc file with 133 instances of free() in its semantic actions does not seem like great code.
DJB has written about avoiding parsing in low level utilities like this. So I hope this is not actually parsed in the main sudo process. A conceivable use of the libprotobuf I saw would be to parse in one process and then deserialize in another, but I don’t know if they’re doing that.
They also have a JSON parser in there. Again I really hope most of this stuff is not configured typically … and not in the main process. Kinda shocked at how big it is
Yes, I think this is a sign of bad engineering.
If you want to know how sudo works in the “default” case (sudoers files), you can look at basically two files.
src/sudo.chas amainthat is “just there”. Inside there there’s in particular thepolicy_checkfunction. This replies on a plugin systemThe “main sudo thing” people use is the
sudoersfile. That is inplugins/sudoers/. Check outverify_userinside ofplugins/sudoers/check.c.A lot of this code is pretty much straight down the line. Very little indentation or conditional paths. It’s honestly stuff that inspires confidence.
There are a lot of places where the fact that it’s C makes the code a bit more futzy. A lot of struct instatiations that look right but merely look it. But I think that implying
sudois a mess because it has a lot of lines of code in the whole project is not really painting a fair picture.OK but what’s the other 173 K lines of code for besides the 2K lines in sudo.c ? Honest question.
It looks like some kind of logging server? Who uses that?
The biggest problem with sudo is its design. It is always the wrong tool.
reallyorsshsshis safer, or something likeuserv.All of them do not try to implement a complicated security boundary in an environment tainted by the untrusted user.
I am curious, could you expand a bit on what these design differences are specifically?
For ssh and userv, the privileged part of the system is run in a clean environment, so they cannot be confused by weird file descriptors, signal dispositions, environment variables, or other oddities that a setuid program can inherit from the unprivileged process that invoked it. It is very difficult to use setuid safely, so it is important to minimize complexity. But sudo is designed to be an all-purpose tool for jumping privilege boundaries with all sorts of complicated rules and restrictions. It’s fundamentally a bad idea.
In most (maybe all?) cases where I have seen sudo used in practice, none of that complexity is actually used. So many people say without thinking that you must not login as root, and use sudo instead; but then their user account becomes defacto equivalent to root, and they have not mitigated whatever unstated risks they might have been trying to avoid. The
reallycommand gets rid of all of the configurability, apart from a privileged group, so it is simply a safety guard for use with a non-root privileged account.I have never understood the point of using sudo with ansible.
Ack for ssh and userv. Basically, instead of suid binary, we use a daemon which is spawned by the root.
For
really, I didn’t get this. Its still basically the suid binary right? So its the same basic design as sudo, except that it has much smaller feature set. So it’s roughly the same thing as doas?It’s much less than doas. It’s specifically for things like
really make install.There are 750 lines of C and 400 lines of yacc in doas http://cvsweb.openbsd.org/src/usr.bin/doas/
But only 250 LOC in really https://www.chiark.greenend.org.uk/ucgi/~ian/git?p=chiark-utils.git;a=blob;f=cprogs/really.c;hb=HEAD
Like sudo, doas wants to be a swiss army knife: it can raise or lower privilege, it has a bespoke language for access control, etc. whereas really will only raise privilege and has one hardcoded access control rule. An account that can run really (much like most uses of sudo) is basically equivalent to root, but does not keep the footgun cocked at all times.
Thanks! It is indeed seems a fundamentally better approach for what I (and I expect many people) use
doas/sudofor. Will switch toreallyas soon as someone else packages it for NixOS.On Linux, another option is to log in as root on the Linux kernel console, which I (naïvely) imagine makes it much more difficult for things running in one’s user session to snoop on one’s sensitive typing through X11.
Are memory safety exploits the problem with sudo? I thought it was mostly things like “unix environments have another environment variable that makes setuid processes load code” or logic errors - maybe Rust can help with those, but it’s not the right case they’re making.
I offered them a way to improve sudo-rs security even more: https://github.com/memorysafety/sudo-rs/issues/291 . By rejecting setuid binaries. They rejected my proposal
I think this is actually a very good suggestion. but it kind of does make sense that it would be a separate tool not called sudo.
Reading your proposal seems less of an “offer” (to implement) than a request to completely re-invent sudo/doas.