New issue
Advanced search Search tips

Issue 901021 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

[Mac] Input Injection fails on Mojave

Project Member Reported by joedow@chromium.org, Nov 1

Issue description

This is due to new security restrictions in Mojave.  We can no longer inject input w/o being added as an accessibility app in the security applet.

The problem with this is:
1.) We don't use the restricted API until a user connects (so they cannot take action)
2.) The dialog appears to only show up once (regardless of approve/deny status)
3.) Users connecting to the lock screen will never see the dialog
 
Components: Services>Chromoting
Labels: -Pri-3 OS-Mac Pri-1
Owner: joedow@chromium.org
Status: Assigned (was: Untriaged)
Note that this is affecting quite a few users, basically everyone who upgrades to Mojave will experience this one way or another.  We should get something in place to lessen the impact of this problem.

A few other notes, there is no way that I can see to specify this permission in the manifest or set up via a script / at install time.  It requires a user action to complete.  Numerous other apps have the same issue (apparent via a web search) so I don't think there is an elegant way to fix this w/o prompting the user, which is likely by design from MacOSes viewpoint.
Labels: Proj-MacMojave
Cc: rsesek@chromium.org
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19b1b85cd5c3227697d348a39cec89ddf5980690

commit 19b1b85cd5c3227697d348a39cec89ddf5980690
Author: Joe Downing <joedow@google.com>
Date: Thu Nov 01 22:13:58 2018

[MacOs Host] Provide user with a prompt to enable input injection on Mojave

This change is required due to new security restrictions in Mojave.  We can no longer
inject input w/o being added as an accessibility app in the security applet.

While this sounds like a usefulk speedbump, it causes remote access applications quite
a bit of trouble:
1.) We don't use the restricted API until a user connects so they cannot approve remotely
2.) The dialog appears to only show up once (regardless of approve/deny status)
3.) Users connecting to a locked machine will never see the dialog

This is affecting quite a few CRD users, basically everyone who upgrades to Mojave
will experience this one way or another.  This is the simplest fix (and easiest to merge)
that I could think of to unblock users.  The prompt will only be shown on 10.14+ platforms
and the request is only shown if the app has not been approved.  I'd like to look at the
user feedback after releasing this change to see if more work is needed.

One problem I anticipate is that the dialog shown doesn't have a lot of context and it
refers to the wrapper script (org.chromium.chromoting.me2me.sh) instead of Chrome Remote
Desktop.  If this is confusing, we can wrap the prompt request in a dialog where we control
the text.  My concern with checking in the feature first is that the new strings won't be
available for merging.

Another behavior to call about this impl is that the prompt will be displayed in two instances:
1.) When the host is first started (choosing enable via app/website)
2.) When the user signs in and the host service is started

Scenario #2 will have less context but that is the only way to ask for permission for
users who upgraded and had CRD installed previously.  The dialog is not displayed at the login
screen (i.e. when no one is signed in).

One last note, there is no way that I can see to specify this permission in the manifest or
set up via a script / at install time.  It requires a user action to complete.

Bug:  901021 
Change-Id: I9dd1b24b6d4d083e7e019af32a0da816f6060a86
Reviewed-on: https://chromium-review.googlesource.com/c/1313170
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604723}
[modify] https://crrev.com/19b1b85cd5c3227697d348a39cec89ddf5980690/remoting/host/mac/BUILD.gn
[add] https://crrev.com/19b1b85cd5c3227697d348a39cec89ddf5980690/remoting/host/mac/permission_utils.h
[add] https://crrev.com/19b1b85cd5c3227697d348a39cec89ddf5980690/remoting/host/mac/permission_utils.mm
[modify] https://crrev.com/19b1b85cd5c3227697d348a39cec89ddf5980690/remoting/host/remoting_me2me_host.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ee7bac4b03590e4c7904e5270ec8264d8f09df1

commit 7ee7bac4b03590e4c7904e5270ec8264d8f09df1
Author: Mike Wittman <wittman@chromium.org>
Date: Thu Nov 01 23:39:10 2018

Revert "[MacOs Host] Provide user with a prompt to enable input injection on Mojave"

This reverts commit 19b1b85cd5c3227697d348a39cec89ddf5980690.

Reason for revert: breaks mac-dbg build

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/mac-dbg/1574

In file included from ../../remoting/host/remoting_me2me_host.cc:15:
In file included from ../../base/bind.h:10:
In file included from ../../base/bind_internal.h:13:
In file included from ../../base/callback_internal.h:14:
In file included from ../../base/memory/ref_counted.h:16:
../../base/logging.h:786:26: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
DEFINE_CHECK_OP_IMPL(NE, !=)
~~~~~~~~~~~~~~~~~~~~~~~~~^~~
../../base/logging.h:774:33: note: expanded from macro 'DEFINE_CHECK_OP_IMPL'
    if (ANALYZER_ASSUME_TRUE(v1 op v2))                                      \
                             ~~ ^  ~~
../../base/logging.h:338:36: note: expanded from macro 'ANALYZER_ASSUME_TRUE'
#define ANALYZER_ASSUME_TRUE(arg) (arg)
                                   ^~~
../../remoting/host/remoting_me2me_host.cc:1576:3: note: in instantiation of function template specialization 'logging::CheckNEImpl<unsigned int, int>' requested here
  DCHECK_NE(getuid(), 0);
  ^
../../base/logging.h:960:31: note: expanded from macro 'DCHECK_NE'
#define DCHECK_NE(val1, val2) DCHECK_OP(NE, !=, val1, val2)
                              ^
../../base/logging.h:913:18: note: expanded from macro 'DCHECK_OP'
      ::logging::Check##name##Impl((val1), (val2),                     \
                 ^
<scratch space>:21:1: note: expanded from here
CheckNEImpl
^
1 error generated.


Original change's description:
> [MacOs Host] Provide user with a prompt to enable input injection on Mojave
> 
> This change is required due to new security restrictions in Mojave.  We can no longer
> inject input w/o being added as an accessibility app in the security applet.
> 
> While this sounds like a usefulk speedbump, it causes remote access applications quite
> a bit of trouble:
> 1.) We don't use the restricted API until a user connects so they cannot approve remotely
> 2.) The dialog appears to only show up once (regardless of approve/deny status)
> 3.) Users connecting to a locked machine will never see the dialog
> 
> This is affecting quite a few CRD users, basically everyone who upgrades to Mojave
> will experience this one way or another.  This is the simplest fix (and easiest to merge)
> that I could think of to unblock users.  The prompt will only be shown on 10.14+ platforms
> and the request is only shown if the app has not been approved.  I'd like to look at the
> user feedback after releasing this change to see if more work is needed.
> 
> One problem I anticipate is that the dialog shown doesn't have a lot of context and it
> refers to the wrapper script (org.chromium.chromoting.me2me.sh) instead of Chrome Remote
> Desktop.  If this is confusing, we can wrap the prompt request in a dialog where we control
> the text.  My concern with checking in the feature first is that the new strings won't be
> available for merging.
> 
> Another behavior to call about this impl is that the prompt will be displayed in two instances:
> 1.) When the host is first started (choosing enable via app/website)
> 2.) When the user signs in and the host service is started
> 
> Scenario #2 will have less context but that is the only way to ask for permission for
> users who upgraded and had CRD installed previously.  The dialog is not displayed at the login
> screen (i.e. when no one is signed in).
> 
> One last note, there is no way that I can see to specify this permission in the manifest or
> set up via a script / at install time.  It requires a user action to complete.
> 
> Bug:  901021 
> Change-Id: I9dd1b24b6d4d083e7e019af32a0da816f6060a86
> Reviewed-on: https://chromium-review.googlesource.com/c/1313170
> Commit-Queue: Joe Downing <joedow@chromium.org>
> Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#604723}

TBR=jamiewalch@chromium.org,joedow@chromium.org

Change-Id: I7c948b26c00f6c6fc7c4e0a3ec4175dcae17e459
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  901021 
Reviewed-on: https://chromium-review.googlesource.com/c/1313747
Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604747}
[modify] https://crrev.com/7ee7bac4b03590e4c7904e5270ec8264d8f09df1/remoting/host/mac/BUILD.gn
[delete] https://crrev.com/0564739b836ebcec0fd43ef603d3282557d2bf2f/remoting/host/mac/permission_utils.h
[delete] https://crrev.com/0564739b836ebcec0fd43ef603d3282557d2bf2f/remoting/host/mac/permission_utils.mm
[modify] https://crrev.com/7ee7bac4b03590e4c7904e5270ec8264d8f09df1/remoting/host/remoting_me2me_host.cc

The revert was due to a comparison issue in a DCHECK, that has been fixed and landed:
https://chromium-review.googlesource.com/c/chromium/src/+/1315468

ChangeID: 19e52db3f4a744f7343a8e8f21339bc50709c11f
Labels: Merge-Request-71
Requesting a merge for this change to M71.  It only affects the Mac Chrome Remote Desktop Host and addresses a problem for CRD users on Mojave.

Thanks!
Labels: -Merge-Request-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #7. Pls merge ASAP. Thank you.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 2

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60

commit 118c5611f1ab02c92c792a74bbcaf43fe8bbdc60
Author: Joe Downing <joedow@google.com>
Date: Fri Nov 02 21:31:51 2018

[MacOs Host] Provide user with a prompt to enable input injection on Mojave

This change is required due to new security restrictions in Mojave.  We can no longer
inject input w/o being added as an accessibility app in the security applet.

While this sounds like a usefulk speedbump, it causes remote access applications quite
a bit of trouble:
1.) We don't use the restricted API until a user connects so they cannot approve remotely
2.) The dialog appears to only show up once (regardless of approve/deny status)
3.) Users connecting to a locked machine will never see the dialog

This is affecting quite a few CRD users, basically everyone who upgrades to Mojave
will experience this one way or another.  This is the simplest fix (and easiest to merge)
that I could think of to unblock users.  The prompt will only be shown on 10.14+ platforms
and the request is only shown if the app has not been approved.  I'd like to look at the
user feedback after releasing this change to see if more work is needed.

One problem I anticipate is that the dialog shown doesn't have a lot of context and it
refers to the wrapper script (org.chromium.chromoting.me2me.sh) instead of Chrome Remote
Desktop.  If this is confusing, we can wrap the prompt request in a dialog where we control
the text.  My concern with checking in the feature first is that the new strings won't be
available for merging.

Another behavior to call about this impl is that the prompt will be displayed in two instances:
1.) When the host is first started (choosing enable via app/website)
2.) When the user signs in and the host service is started

Scenario #2 will have less context but that is the only way to ask for permission for
users who upgraded and had CRD installed previously.  The dialog is not displayed at the login
screen (i.e. when no one is signed in).

One last note, there is no way that I can see to specify this permission in the manifest or
set up via a script / at install time.  It requires a user action to complete.

Bug:  901021 

Change-Id: I54247dd56111feee9608e1b50387cb3501a659a4
Reviewed-on: https://chromium-review.googlesource.com/c/1315468
Reviewed-by: Gary Kacmarcik <garykac@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604947}(cherry picked from commit 19e52db3f4a744f7343a8e8f21339bc50709c11f)
Reviewed-on: https://chromium-review.googlesource.com/c/1315706
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#478}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60/remoting/host/mac/BUILD.gn
[add] https://crrev.com/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60/remoting/host/mac/permission_utils.h
[add] https://crrev.com/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60/remoting/host/mac/permission_utils.mm
[modify] https://crrev.com/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60/remoting/host/remoting_me2me_host.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/118c5611f1ab02c92c792a74bbcaf43fe8bbdc60

Commit: 118c5611f1ab02c92c792a74bbcaf43fe8bbdc60
Author: joedow@google.com
Commiter: joedow@chromium.org
Date: 2018-11-02 21:31:51 +0000 UTC

[MacOs Host] Provide user with a prompt to enable input injection on Mojave

This change is required due to new security restrictions in Mojave.  We can no longer
inject input w/o being added as an accessibility app in the security applet.

While this sounds like a usefulk speedbump, it causes remote access applications quite
a bit of trouble:
1.) We don't use the restricted API until a user connects so they cannot approve remotely
2.) The dialog appears to only show up once (regardless of approve/deny status)
3.) Users connecting to a locked machine will never see the dialog

This is affecting quite a few CRD users, basically everyone who upgrades to Mojave
will experience this one way or another.  This is the simplest fix (and easiest to merge)
that I could think of to unblock users.  The prompt will only be shown on 10.14+ platforms
and the request is only shown if the app has not been approved.  I'd like to look at the
user feedback after releasing this change to see if more work is needed.

One problem I anticipate is that the dialog shown doesn't have a lot of context and it
refers to the wrapper script (org.chromium.chromoting.me2me.sh) instead of Chrome Remote
Desktop.  If this is confusing, we can wrap the prompt request in a dialog where we control
the text.  My concern with checking in the feature first is that the new strings won't be
available for merging.

Another behavior to call about this impl is that the prompt will be displayed in two instances:
1.) When the host is first started (choosing enable via app/website)
2.) When the user signs in and the host service is started

Scenario #2 will have less context but that is the only way to ask for permission for
users who upgraded and had CRD installed previously.  The dialog is not displayed at the login
screen (i.e. when no one is signed in).

One last note, there is no way that I can see to specify this permission in the manifest or
set up via a script / at install time.  It requires a user action to complete.

Bug:  901021 

Change-Id: I54247dd56111feee9608e1b50387cb3501a659a4
Reviewed-on: https://chromium-review.googlesource.com/c/1315468
Reviewed-by: Gary Kacmarcik <garykac@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604947}(cherry picked from commit 19e52db3f4a744f7343a8e8f21339bc50709c11f)
Reviewed-on: https://chromium-review.googlesource.com/c/1315706
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#478}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Assigned)
Issue has been merged and is being included as part of our M71 host rollout.

I will follow-up with a separate bug to track UI changes needed to make the experience a little better (we currently rely on the Mac AX dialog which is pretty terse, it would be nice to provide more context to the user).

Sign in to add a comment