Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939263003:
Add support for CRD user-session to operate setuid (Closed)
DescriptionAdd support for CRD user-session to operate setuid
This introduces the code changes necessary for CRD's user_session to
operate as a setuid binary. Code is added to enforce that a user
executing at setuid user_session binary can only use it to start a
session for themselves. Additional refactoring reduces the amount of code that runs as root.
This CL does not make user_session install setuid by default.
Review-Url: https://codereview.chromium.org/2939263003
Cr-Commit-Position: refs/heads/master@{#489096}
Committed: https://chromium.googlesource.com/chromium/src/+/2db73c0d825ec2dbcd8b2d3185df0dc7bc90ef34
Patch Set 1 : Allow support to CRD user_session to operate setuid #
Total comments: 4
Patch Set 2 : Consistent identifier for real uid #Patch Set 3 : Refactor to reduce code run as root #
Total comments: 37
Patch Set 4 : Address review feedback #
Total comments: 2
Messages
Total messages: 24 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
jamiewalch@chromium.org changed reviewers: + jamiewalch@chromium.org
https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:509: uid_t real_uid = getuid(); Please be consistent with the naming of this; real_uid or real_id are both fine, but pick one :) https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:516: std::exit(EXIT_FAILURE); Since you already check LOGNAME and USER and use them if they are valid, couldn't you allow the user to be specified for non-root, but subject it to the same check? Prohibiting it doesn't buy you anything because a user can set one of the variables in lieu of passing the command-line flag. If you allow the username to be specified on the command-line, you could even get rid of the special-casing for LOGNAME and USER if you think it's sufficiently rare that it will be needed, since users who do need it would have a workaround.
https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:509: uid_t real_uid = getuid(); On 2017/06/16 19:24:23, Jamie wrote: > Please be consistent with the naming of this; real_uid or real_id are both fine, > but pick one :) Good catch. https://codereview.chromium.org/2939263003/diff/10002/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:516: std::exit(EXIT_FAILURE); On 2017/06/16 19:24:23, Jamie wrote: > Since you already check LOGNAME and USER and use them if they are valid, > couldn't you allow the user to be specified for non-root, but subject it to the > same check? Prohibiting it doesn't buy you anything because a user can set one > of the variables in lieu of passing the command-line flag. > > If you allow the username to be specified on the command-line, you could even > get rid of the special-casing for LOGNAME and USER if you think it's > sufficiently rare that it will be needed, since users who do need it would have > a workaround. user-session isn't really intended to be called directly by the user. Instead, it is expected that the user will continue to call `/etc/init.d/chrome-remote-desktop start` (or possibly `/opt/google/chrome-remote-desktop/chrome-remote-desktop --start`) as they currently do, and user_session will be used transparently. In this context, it really only makes sense to try to automatically obtain the correct username. While we certainly *could* accept --user when run as non-root for only a few additional lines of code, I don't think it would buy us anything. Even with it, I imagine a user would be much more apt to run `sudo -u other_username /etc/init.d/chrome-remote-desktop start` than `/opt/google/chrome-remote-desktop/user-session --user=other_username --me2me-script=/opt/google/chrome-remote-desktop/chrome-remote-desktop`. By checking the USER environment variable, we ensure that the former does the right thing.
Description was changed from ========== Allow support to CRD user_session to operate setuid This introduces the code changes necessary for CRD's user_session to operate as a setuid binary. Code is added to enforce that a user executing at setuid user_session binary can only use it to start a session for themselves. This CL does not make user_session install setuid by default. ========== to ========== Add support for CRD user-session to operate setuid This introduces the code changes necessary for CRD's user_session to operate as a setuid binary. Code is added to enforce that a user executing at setuid user_session binary can only use it to start a session for themselves. Additional refactoring reduces the amount of code that runs as root. This CL does not make user_session install setuid by default. ==========
rkjnsn@chromium.org changed reviewers: + lambroslambrou@chromium.org
Jamie, Lambros: Please take a look.
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:248: readlink("/proc/self/exe", &script_path[0], script_path.size()); Use base::ReadSymbolicLink() from base/files/file_util.h ? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:256: std::size_t last_slash = script_path.find_last_of('/'); Use base::FilePath:: DirName() and Append() instead of raw string manipulations here? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:265: void ChildProcess(base::EnvironmentMap environment, Please name the function as a verb. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:288: for (auto& env_var : environment) { const auto& https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:295: env_ptrs.reserve(env_strings.size() + 1); I don't know if we're allowed to malloc() in the child process? Have a look at https://cs.chromium.org/chromium/src/base/process/launch_posix.cc?type=cs&q=G... where it calls reserve() in advance of doing the fork. Might not matter to us (and maybe GNU glibc actually lets us malloc in the child anyway)? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:296: for (auto& env_string : env_strings) { const auto& https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:303: PCHECK(false) << "failed to exec session script"; Maybe log the filename (full path) of the script that failed? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:308: // will be changed to the target user. If match_uid is not nullopt, this I think "present" or "provided" reads better than "not nullopt"? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:360: pid_t child_pid = fork(); Why have you removed base::LaunchProcess() here? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:370: // waidpid will return if the child is ptraced, so loop until the process waitpid https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:371: // actually exits. Could this be a busy loop? Have a look at https://cs.chromium.org/chromium/src/base/process/process_posix.cc?type=cs&q=... to see how it's done there. They ramp up the usleep() but I guess a simple fixed duration is good enough for us? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:419: // to attempt a reverse lookup. Note: It's possible to more than user with the .. to have more .. Is this really true? I thought user IDs were unique on a system? https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:497: if (argc < 2 || strcmp(argv[1], "start") != 0) { Why remove base::CommandLine() ? The code manipulating argc/argv directly is much harder to read and check for correctness. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:537: bool chown_stdout = false; Why add a new flag? If you prefer this new flag for readability, better to declare it just before it's used: bool chown_stdout = !foreground; ExecuteSession(..chown_stdout..);
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:248: readlink("/proc/self/exe", &script_path[0], script_path.size()); On 2017/07/20 01:42:49, Lambros wrote: > Use base::ReadSymbolicLink() from base/files/file_util.h ? This probably applies to a few of your comments, and it needs to be decided on a case-by-case basis, but the rationale behind minimizing the use of library code is that most of it doesn't normally run setuid root. We also want to minimize the amount of code that gets pulled into this binary.
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:248: readlink("/proc/self/exe", &script_path[0], script_path.size()); On 2017/07/20 01:42:49, Lambros wrote: > Use base::ReadSymbolicLink() from base/files/file_util.h ? When I was developing this patch, this happened before forking and thus I wanted to limit the amount of external code run as root. Now that I have pushed it to be after forking and dropping privileges, using base::ReadSymbolicLink() is probably okay. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:256: std::size_t last_slash = script_path.find_last_of('/'); On 2017/07/20 01:42:49, Lambros wrote: > Use base::FilePath:: DirName() and Append() instead of raw string manipulations > here? Same here. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:265: void ChildProcess(base::EnvironmentMap environment, On 2017/07/20 01:42:49, Lambros wrote: > Please name the function as a verb. Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:288: for (auto& env_var : environment) { On 2017/07/20 01:42:49, Lambros wrote: > const auto& Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:295: env_ptrs.reserve(env_strings.size() + 1); On 2017/07/20 01:42:49, Lambros wrote: > I don't know if we're allowed to malloc() in the child process? > Have a look at > https://cs.chromium.org/chromium/src/base/process/launch_posix.cc?type=cs&q=G... > where it calls reserve() in advance of doing the fork. > > Might not matter to us (and maybe GNU glibc actually lets us malloc in the child > anyway)? I think this should be okay. As far as I understand it, the potential problem occurs when the forking process has multiple threads. If the fork occurs while another thread happens to be holding a lock, then that lock will never be released in the child process, resulting in a deadlock if the child later tries to obtain the lock. In a large application like Chromium, where there will be many threads possibly allocating memory or calling POSIX functions, the only safe way to ensure the child doesn't deadlock is to avoid calling anything that might try to acquire a lock, which in practice means limiting oneself to operations that are defined to be async-signal-safe. Since we know we're the only thread in the process, we can be sure that the fork is not going to occur in the middle of some lock-holding operation. (As a side note, I believe glibc actually adds an atfork handler which will obtain an allocator lock before forking and release it afterword, ensuring that no other thread has an allocator lock during the fork. This only applies to the system allocator, though, while I seem to recall that Chromium uses tcmalloc.) https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:296: for (auto& env_string : env_strings) { On 2017/07/20 01:42:50, Lambros wrote: > const auto& Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:303: PCHECK(false) << "failed to exec session script"; On 2017/07/20 01:42:49, Lambros wrote: > Maybe log the filename (full path) of the script that failed? Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:308: // will be changed to the target user. If match_uid is not nullopt, this On 2017/07/20 01:42:49, Lambros wrote: > I think "present" or "provided" reads better than "not nullopt"? Agreed. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:360: pid_t child_pid = fork(); On 2017/07/20 01:42:49, Lambros wrote: > Why have you removed base::LaunchProcess() here? This is actually the key change for this version of the patch. Using LaunchProcess requires us to figure out the exec arguments and environment before forking, while we are still root. By removing LaunchProcess, we can move all of our code to set environment variables, determine the script location, and build the command line all in the child process after we have dropped privileges. This significantly reduces the amount of code executed as root, which is especially important in a setuid binary, as any exploitable bug in the code that runs as root could be used for a privilege escalation attack. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:370: // waidpid will return if the child is ptraced, so loop until the process On 2017/07/20 01:42:49, Lambros wrote: > waitpid > Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:371: // actually exits. On 2017/07/20 01:42:49, Lambros wrote: > Could this be a busy loop? Have a look at > https://cs.chromium.org/chromium/src/base/process/process_posix.cc?type=cs&q=... > > to see how it's done there. They ramp up the usleep() but I guess a simple fixed > duration is good enough for us? > I don't think so. In that code, they need to sleep because they are calling waitpid with the WNOHANG option, which means it returns immediately even if the child process is still running. It looks like they do that to implement timeout functionality. Since we're not using WNOHANG here, waitpid should block until it has something to report. (Unless a signal causes an EINTR, which I just realized I'm not handling.) https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:419: // to attempt a reverse lookup. Note: It's possible to more than user with the On 2017/07/20 01:42:50, Lambros wrote: > .. to have more .. > > Is this really true? I thought user IDs were unique on a system? Yes, I should probably word this differently. Having two different people with the same user ID would be a very bad idea. However, what one can do is have multiple passwd entries for the same user, distinguished by having different user names, with potentially different group membership, home directories, default shells, et cetera. Here's an example of someone doing this in the real world: https://askubuntu.com/a/427257 (One could probably argue that there's a better way to solve that particular problem, but the point is that people do do it in practice.) https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:497: if (argc < 2 || strcmp(argv[1], "start") != 0) { On 2017/07/20 01:42:50, Lambros wrote: > Why remove base::CommandLine() ? The code manipulating argc/argv directly is > much harder to read and check for correctness. > This is something that specifically came up in security review. Command-line parsing is not something that's usually security sensitive, as exploiting a bug in command-line parsing doesn't generally let you do anything you couldn't do, otherwise. In the case of of a setuid binary, however, a bug and command-line parsing can lead to a privilege escalation vulnerability, especially given that the user can pass whatever they want as the command-line. base::CommandLine has a fair amount of code to support features we don't need for the wrapper, and even if we audited it today, there's no guarantee that a bug won't get added, tomorrow. By using very basic command-line parsing, we hopefully reduce the attack surface and make it more likely that future changes will receive the appropriate amount of scrutiny. I agree that manual parsing is harder to read, though. Let me know if you have any suggestions for improving it. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:537: bool chown_stdout = false; On 2017/07/20 01:42:49, Lambros wrote: > Why add a new flag? > > If you prefer this new flag for readability, better to declare it just before > it's used: > > bool chown_stdout = !foreground; > ExecuteSession(..chown_stdout..); I thought this was more readable, both because one can tell what the argument does without referring to the declaration of ExecuteSession and because it makes it more apparent why it should be the negation of foreground. You're way probably works just as well if I keep the comment, though.
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:12: // --foreground - Don't doemonize. daemonize https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:249: PCHECK(readlink_result >= 0) << "readlink failed"; Why PCHECK instead of CHECK here? I couldn't find any documentation on the difference. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:514: std::fputs("Too many command-line arguments.\n", stderr); exit here?
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:12: // --foreground - Don't doemonize. On 2017/07/20 16:56:48, Jamie wrote: > daemonize Acknowledged. https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:249: PCHECK(readlink_result >= 0) << "readlink failed"; On 2017/07/20 16:56:48, Jamie wrote: > Why PCHECK instead of CHECK here? I couldn't find any documentation on the > difference. PCHECK additionally prints the error message associated with errno. See https://cs.chromium.org/chromium/src/base/logging.h?l=125 https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:514: std::fputs("Too many command-line arguments.\n", stderr); On 2017/07/20 16:56:48, Jamie wrote: > exit here? Doh!
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:248: readlink("/proc/self/exe", &script_path[0], script_path.size()); On 2017/07/20 06:45:52, rkjnsn wrote: > On 2017/07/20 01:42:49, Lambros wrote: > > Use base::ReadSymbolicLink() from base/files/file_util.h ? > > When I was developing this patch, this happened before forking and thus I wanted > to limit the amount of external code run as root. Now that I have pushed it to > be after forking and dropping privileges, using base::ReadSymbolicLink() is > probably okay. Actually, looking at the implementation in file_util_posix.cc, it doesn't look like ReadSymbolicLink checks if the link was too long for the buffer. PATH_MAX is arbitrary and not actually enforced by anything, so that can lead to a truncated result. It also discards all error information and just returns false if something goes wrong.
https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/70001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:248: readlink("/proc/self/exe", &script_path[0], script_path.size()); On 2017/07/20 17:49:43, rkjnsn wrote: > On 2017/07/20 06:45:52, rkjnsn wrote: > > On 2017/07/20 01:42:49, Lambros wrote: > > > Use base::ReadSymbolicLink() from base/files/file_util.h ? > > > > When I was developing this patch, this happened before forking and thus I > wanted > > to limit the amount of external code run as root. Now that I have pushed it to > > be after forking and dropping privileges, using base::ReadSymbolicLink() is > > probably okay. > > Actually, looking at the implementation in file_util_posix.cc, it doesn't look > like ReadSymbolicLink checks if the link was too long for the buffer. PATH_MAX > is arbitrary and not actually enforced by anything, so that can lead to a > truncated result. It also discards all error information and just returns false > if something goes wrong. Okay, further testing indicates that Linux will return ENAMETOOLONG if the path to the executable is longer that PATH_MAX, so using ReadSymbolicLink should be okay.
Updated in response to feedback. Please take a look. https://codereview.chromium.org/2939263003/diff/90001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/90001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:247: PCHECK(result) << "Failed to determine binary location"; Using PCHECK here assumes the failure stems from the underlying readlink call and errno hasn't been clobbered. This assumption is correct for the current implementation of ReadSymbolicLink, but if we don't want to rely on it, I can change this to a normal CHECK.
lgtm https://codereview.chromium.org/2939263003/diff/90001/remoting/host/linux/rem... File remoting/host/linux/remoting_user_session.cc (right): https://codereview.chromium.org/2939263003/diff/90001/remoting/host/linux/rem... remoting/host/linux/remoting_user_session.cc:247: PCHECK(result) << "Failed to determine binary location"; On 2017/07/21 01:55:35, rkjnsn wrote: > Using PCHECK here assumes the failure stems from the underlying readlink call > and errno hasn't been clobbered. This assumption is correct for the current > implementation of ReadSymbolicLink, but if we don't want to rely on it, I can > change this to a normal CHECK. I think this is fine.
lgtm
The CQ bit was checked by rkjnsn@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1500933550734270,
"parent_rev": "4a7bb7aca272eecc62437c28c0921925c04af03f", "commit_rev":
"2db73c0d825ec2dbcd8b2d3185df0dc7bc90ef34"}
Message was sent while issue was closed.
Description was changed from ========== Add support for CRD user-session to operate setuid This introduces the code changes necessary for CRD's user_session to operate as a setuid binary. Code is added to enforce that a user executing at setuid user_session binary can only use it to start a session for themselves. Additional refactoring reduces the amount of code that runs as root. This CL does not make user_session install setuid by default. ========== to ========== Add support for CRD user-session to operate setuid This introduces the code changes necessary for CRD's user_session to operate as a setuid binary. Code is added to enforce that a user executing at setuid user_session binary can only use it to start a session for themselves. Additional refactoring reduces the amount of code that runs as root. This CL does not make user_session install setuid by default. Review-Url: https://codereview.chromium.org/2939263003 Cr-Commit-Position: refs/heads/master@{#489096} Committed: https://chromium.googlesource.com/chromium/src/+/2db73c0d825ec2dbcd8b2d3185df... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:90001) as https://chromium.googlesource.com/chromium/src/+/2db73c0d825ec2dbcd8b2d3185df... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
