session_manager blindly trusts chrome on what kind of ARC++ container to launch |
||||
Issue descriptionStartArcInstance contains a request parameter that indicates whether the launch request is for the login screen or not. AFAICT, session_manager blindly trusts that signal. There's a security interest to not allow starting a full ARC++ container until the point where a session has started. session_manager tracks whether a session is active in session_started, so we should either switch over the decision logic to use that instead or add validation. It's possible that I'm missing something here, but better to file an invalid bug than not to file a valid one :)
,
Nov 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/247604f8eabaae4d15c12b189e27cd68e22e7659 commit 247604f8eabaae4d15c12b189e27cd68e22e7659 Author: yusukes <yusukes@google.com> Date: Sat Nov 11 07:19:09 2017 login: Add more comment to StartArcInstanceInternal() The function checks if |account_id| is valid before starting a full container. This CL add a comment to that part to explicitly mention that the check is important from the security perspective. BUG= chromium:783120 TEST=emerge Change-Id: I4cd46bf7f0f7727151cdea6b00026a9e7928bf67 Reviewed-on: https://chromium-review.googlesource.com/760880 Commit-Ready: Yusuke Sato <yusukes@chromium.org> Tested-by: Yusuke Sato <yusukes@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/247604f8eabaae4d15c12b189e27cd68e22e7659/login_manager/session_manager_impl.cc
,
Nov 11 2017
Closing with the patch above for now. Please reopen if you still see the problem.
,
Jan 22 2018
,
Jan 23 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by yusukes@chromium.org
, Nov 9 2017Status: Started (was: Unconfirmed)
Actually, SessionManagerImpl::StartArcInstanceInternal() does such a validation to prevent a full container from starting on login screen: std::string account_id; if (!NormalizeAccountId(in_request.account_id(), &account_id, error)) { DCHECK(*error); return false; } if (user_sessions_.count(account_id) == 0) { constexpr char kMessage[] = "Provided user ID does not have a session."; LOG(ERROR) << kMessage; *error = CreateError(dbus_error::kSessionDoesNotExist, kMessage); return false; } but it's probably better to add a comment to say the check is important from the security standpoint. I'll send you a CL shortly.