sudo-rs
sudo-rs copied to clipboard
memorysafety
String encodings
Right now we are assuming that several entities are valid Rust strings. The list of such entities includes but it is not restricted to:
- user names, passwords, home directory, shell directory and GECOS field.
- environment variables.
We've discussed this and concluded that this is not a pressing issue right now but it is something to take into account in the future.
I've opened https://github.com/memorysafety/sudo-rs/pull/215 which changes the user's home and shell directory to be PathBufs because they are more useful in that way.
While doing that change I've noticed that all the user information is extracted from C strings which means that this info is stored in a buffer of bytes. This is fine for Linux (and Unix in general), but it also means that these fields are poorly represented as OsStrings.
At the same time, OsString is an opaque type with few ways to manipulate it. Maybe we should use CString or just Vec<u8> for the remaining user information.
Note: some investigation in the PAM module: we don't allow users to authenticate that, say, use a Latin1 encoding for their shell and have non-ASCII characters in their password. Original sudo does.
How about bstr?
That would be nice but it would be an extra dependency which is not ideal.
Perhaps OsStr is good enough?
Sadly OsStr is too opaque for some usecases as you cannot do simple things like .starts_with(whatever). I think we'll just have to do this on a case-to-case basis.
Why extra dep is not ideal?
Having an extra dep means we have more code to worry about in security terms
Yes that's an issue. But having an extra dep that is very popular means that once you find a security breach and fix it you will improve the security of ecosystem overall.
For example:
- You don't have to call unsafe tcsetattr, tcgetattr, you got rustix for it.
- You don't have to play around lossy strings, you got bstr for it.
- You don't have to implement your own bzero, you got zeroize for it.
I understand your concerns but for now there are too many unsafe lines where you don't have to.
This is something we have discussed internally [1]. We try as best to isolate our unsafe crates from the rest of the project. That's one of the main reasons to have a workspace in the first place. So having unsafe code when we don't "have" to is actually a deliberate decision where we traded off introducing a new dependency against maintaining that unsafe parts ourselves.
Having a dependency that's popular means that we need to become familiar with the dependency to be able to fix or suggest a fix for a potential security issue so this actually increases the amount of eyes we have to keep on top of code that we don't maintain and might not be familiar with. So yeah, it has the nice effect of not having to write that code, but once it breaks it is actually harder for us to get it fixed.
So the TLDR is that you have raised very important points, we appreciate your input and we already consider such points when deciding if we introduce a new dependency or not.
[1] We're working on how to communicate these discussions better now that the project has some visibility