Re: [PATCH BlueZ 0/3] Per-device option to enable/disable internal profiles

[Sonny Sasaka <sonnysasaka@chromium.org>]

Hi Luiz,

On Wed, Jul 15, 2020 at 2:09 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Wed, Jul 15, 2020 at 1:13 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > On Wed, Jul 15, 2020 at 9:25 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Jul 14, 2020 at 1:55 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Mon, Jul 13, 2020 at 10:28 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Sonny,
> > > > >
> > > > > On Mon, Jul 13, 2020 at 4:48 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > > >
> > > > > > Hi Luiz,
> > > > > >
> > > > > > On Mon, Jul 13, 2020 at 3:59 PM Luiz Augusto von Dentz
> > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Sonny,
> > > > > > >
> > > > > > > On Mon, Jul 13, 2020 at 3:04 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Hi Luiz,
> > > > > > > >
> > > > > > > > I considered having such an approach that gives exception to some
> > > > > > > > profile to not claim exclusive access. However, I think that this
> > > > > > > > approach has a drawback that it can only be guaranteed to work
> > > > > > > > correctly for profiles that contain only read-only attributes. Any
> > > > > > > > profile that contains writable attributes, naturally, cannot be
> > > > > > > > guaranteed to always work correctly (as is the case with the Battery
> > > > > > > > profile). Therefore, the usefulness of that feature will be very
> > > > > > > > limited.
> > > > > > > >
> > > > > > > > I also considered the benefits of the AllowInternalProfiles approach:
> > > > > > > > * Applications can also have control over any profile, not just
> > > > > > > > Battery profile. For example, if in the future BlueZ has more internal
> > > > > > > > profiles, like (Blood Pressure Profile or any other profile that may
> > > > > > > > contain writable attributes), we can guarantee that applications can
> > > > > > > > still opt to have access to that profile, without relying on a profile
> > > > > > > > being "safe" to be shared by both BlueZ's internal and external
> > > > > > > > handlers.
> > > > > > > > * This has an added security benefit: applications which operate on a
> > > > > > > > specific GATT profile will not unintentionally activate internal
> > > > > > > > profiles such as HOG (which is able to hijack input control of the
> > > > > > > > host). This is the correct and expected behavior of Android apps that
> > > > > > > > connect over GATT and get access to a GATT profile.
> > > > > > >
> > > > > > > Not sure I follow these arguments, it seems AllowInternalProfiles may
> > > > > > > actually enable hijacking the profiles so I wonder if you got this
> > > > > > > backwards as we can't let things like HoG be controlled by
> > > > > > > applications directly it would be too easy to implement something like
> > > > > > > a keylogger, or perhaps you are suggesting that there is another layer
> > > > > > > for implementing the profiles? Note that it is intended that plugins
> > > > > > > shall be used for profiles that need to be integrated system wide,
> > > > > > > D-Bus interface shall be restricted to only application specific
> > > > > > > profiles.
> > > > > >
> > > > > > I think you misunderstood my point about HOG hijacking. Consider the
> > > > > > following case:
> > > > > > 1. A legit application (not System UI) on a host computer scans and
> > > > > > connects to a nearby peer. It makes a guess about the peer device
> > > > > > based on its advertising data. It intends to operate on a specific
> > > > > > GATT profile (not necessarily Battery).
> > > > > > 2. The peer device turns out to be malicious. It runs HOG GATT server
> > > > > > and triggers the host's HOG profile to be active.
> > > > > > 3. The malicious peer device's HOG GATT server can now maliciously
> > > > > > make mouse movements or enter keystrokes to the host.
> > > > >
> > > > > I'm not sure how you would like to prevent that, we could in theory
> > > > > attempt to authorize every single profile before connecting, just like
> > > > > it is done for legacy, but Im sure system would not be asking the user
> > > > > what profiles to connect so they just end up trusting the device,
> > > > > alternatively we could make ConnectProfile to work also for LE so you
> > > > > can provide a UUID and nothing else would be exposed, but note that
> > > > > this guess on the AD may actually be wrong and the device may end up
> > > > > malfunctioning.
> > > > >
> > > > > > In this case the user is considered being attacked, because he/she
> > > > > > doesn't consciously interact with the System UI to connect to a nearby
> > > > > > mouse/keyboard.
> > > > > > My example doesn't have to be HOG. It just happens to be a profile
> > > > > > which is attackable at the moment. My point is that, for applications
> > > > > > it's always safest to turn off all internal profiles to avoid such
> > > > > > incident. There is no use case where applications want to trigger
> > > > > > internal profiles.
> > > > > >
> > > > > > Note 1: By "applications", I mean things like Android apps or
> > > > > > JavaScript apps which are not considered System's Bluetooth UI.
> > > > >
> > > > > Well that doesn't make my point moot, let's say we do enable
> > > > > connecting by UUID and the application try to connect HoG, it could be
> > > > > a malicious application trying to eavesdrop what the user is typing,
> > > > > so this problem of malicious goes both ways Im afraid, besides the
> > > > > performance penalty that one would have if we need to transport HID
> > > > > over D-Bus.
> > > > If an application handles HOG, there will be nothing to eavesdrop
> > > > because that application shouldn't have an access to UHID in the first
> > > > place. If that malicious application had UHID access, that is already
> > > > a problem to begin with regardless of whether there is Bluetooth or
> > > > not. The security of this is handled above the Bluetooth layer. The
> > > > operating system that uses this feature is responsible for higher
> > > > layer security. For operating systems that don't need it I am okay
> > > > with adding an option to disable this feature altogether. But I can
> > > > see that there are systems that need it and I am not convinced that a
> > > > general purpose Bluetooth stack should not support it.
> > >
> > > All a malicious application has to do is to subscribe to notification
> > > of HoG characteristic, then any input generate by the device would be
> > > notified back to the application and that doesn't matter if uHID is
> > > accessible or not the application may not even forward the events to
> > > the system, now perhaps you are imagining that applications don't have
> > > direct access to the attribute objects but that would be system
> > > specific which is rather tricky to define.
> > When the HOG-related GATT object is available on D-Bus, that means the
> > internal HOG profile is not enabled in the first place, so there is
> > nothing to sniff anyway. Furthermore, the higher layer operating
> > system is also responsible to prevent this if it chooses to disable
> > BlueZ's internal HOG profile handler. If an operating system thinks
> > that it is safer to always enable internal HOG they can do just that,
> > as I mentioned I'm willing to add an option to always enable all
> > internal profiles.
> >
> > >
> > > > >
> > > > > More applications could be involved and then this all becomes a mess
> > > > > if they have to fight over AllowInternalProfiles, so instead of using
> > > > > a theoretical example we better find real apps and devices where
> > > > > conflicts happens and work out case by case, adding ConnectProfile
> > > > > should actually fix most of them if there is a single profile involved
> > > > > by we could also thing about an alternative to connect multiples.
> > > > > There is also the possibility of exposing the btd_service as objects,
> > > > > I've actually had this implemented for the car industry, that way
> > > > > AutoConnect property could actually be controlled on a per service
> > > > > basis instead of having just one switch for everything.
> > > > To be clear, applications do not have direct access to
> > > > AllowInternalProfiles. The higher layer operating system controls it.
> > > > This is just the same case as the org.bluez.Adapter1.Powered property
> > > > and many other examples where applications are not expected to have
> > > > direct control of. Therefore there should be no problem of many things
> > > > fighting over it if used correctly, just like many other properties.
> > > > Again, I am okay with adding an option to disable this for operating
> > > > systems that do not want it.
> > >
> > > I see, though you didn't comment on the idea of controlling this on a
> > > per-service basis, not just have everything disabled with
> > > AllowInternalProfiles, note though that there are some profiles we
> > > can't really disable like GAP and GATT as that involves things that
> > > bt_gatt_client itself does need to access in order to work properly.
> > Right, I missed that GAP should be an exception that it cannot be
> > disabled, I should've added that exception in my code.
> >
> > However, it seems that you still don't want a single switch to disable
> > all internal profiles (even with GAP exception). I'm willing to modify
> > this feature to be a blocklist of profiles per device (say
> > BlockedProfiles property on Device1 interface), and this feature can
> > be disabled altogether (all profiles always enabled) for operating
> > systems that do not want it.
> >
> > That idea is also similar to your service_api branch, so I will also
> > try to port your service_api branch on the master branch. I will test
> > this and I am okay with using this if it serves our purposes.
> > I do have a question, though: With this API design, the service
> > objects are not exposed until a remote profile is detected, and
> > sometimes a profile is not detected until connection takes place (if
> > the profile UUID is not in the advertisement). So, how does the BlueZ
> > client block certain profiles/services before connection takes place?
> > We can't wait until connection takes place because we already know
> > that we don't want certain profiles, and if we block a profile after
> > connection takes place would that work properly? I guess we still need
> > a way to block certain profiles in the Device1 API, and the blocked
> > profiles also need to be stored in store_device_info.
>
> That is a good point, but that would be a problem only for the very
> first time you connect since after that the blocked state should be
> recovered, we could however work out an API where one can enter the
> profiles to connect and to block but usually depends on the services
> being resolved so I wonder it wouldn't just be enough to block the
> service discovery and in the event the internal profile has already
> been connected it would just disconnect, the objects would only be
> exported after you had it blocked so that means there should be any
> race, but perhaps you want a dedicate Connect method where one can
> enumerate the profiles to auto-connect and to block, note though that
> it may not be possible to indicate a valid list in case the services
> have not been resolved.
Thanks for the input. In the meantime, I am okay with resolving the
battery profile specifically by giving it an exception from being
claimed internally. I have sent another patch for this.
>
> > >
> > > You can find the service_api commits in here:
> > >
> > > https://github.com/Vudentz/BlueZ/commits/service_api
> > >
> > > It does allow to control both the auto_connect logic as well a block:
> > >
> > > https://github.com/Vudentz/BlueZ/commit/9bd6dce59fe9978b3bf415fe74f89d72254b8075
> > > https://github.com/Vudentz/BlueZ/commit/42a7e479d5beb641a3d94f724a2df60db0f8221c
> > >
> > > > Note: I have been using the term "operating system" to refer to high
> > > > level components rather than the kernel.
> > > >
> > > >
> > > > >
> > > > > > >
> > > > > > > Note that we do allow external profiles to be registered with use of:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/profile-api.txt
> > > > > > >
> > > > > > > And for GATT:
> > > > > > >
> > > > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n366
> > > > > > >
> > > > > > > We could perhaps make the assumption that once an application
> > > > > > > registers itself as supporting a given profile we check if against a
> > > > > > > blacklist of profiles that may have security implications, which
> > > > > > > perhaps could be defined via main.conf or some other file, if that is
> > > > > > > not the case the internal profile can be disabled and the D-Bus object
> > > > > > > would be accessible over D-Bus. Also note that we do offer clients the
> > > > > > > ability to have exclusive access with AcquireWrite and AcquireNotify.
> > > > > > >
> > > > > > > > Therefore I think that this approach, although more complex, has
> > > > > > > > longer lasting benefits. Let me know if you have any objection to
> > > > > > > > having such a feature.
> > > > > > > >
> > > > > > > >
> > > > > > > > On Mon, Jul 13, 2020 at 1:35 PM Luiz Augusto von Dentz
> > > > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Sonny,
> > > > > > > > >
> > > > > > > > > On Mon, Jul 13, 2020 at 1:18 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > > > > > > >
> > > > > > > > > > This patch series adds a mechanism for clients to choose whether to
> > > > > > > > > > enable BlueZ internal profiles (e.g. A2DP, Battery) for specific
> > > > > > > > > > devices.
> > > > > > > > > >
> > > > > > > > > > The motivation behind this feature is that some applications (e.g. Web
> > > > > > > > > > Bluetooth or Android apps) need to have control over all remove GATT
> > > > > > > > > > services, like Battery service. With "battery" plugin being enabled on
> > > > > > > > > > BlueZ, it becomes not possible for those apps to work properly because
> > > > > > > > > > BlueZ "hides" the Battery-related attributes from its GATT Client API.
> > > > > > > > > > Disabling the "battery" plugin won't solve the problem either, since we
> > > > > > > > > > do also need to enable the plugin so that we can use org.bluez.Battery1
> > > > > > > > > > API.
> > > > > > > > > >
> > > > > > > > > > The solution that we propose is that clients can choose whether to
> > > > > > > > > > enable internal profiles for each device. Clients know when to enable
> > > > > > > > > > internal profiles (such as when a user chooses to pair/connect via a UI)
> > > > > > > > > > and when to disable internal profiles (such as when the connection is
> > > > > > > > > > initiated by a generic application).
> > > > > > > > >
> > > > > > > > > I wonder if it is not better to just have a flag indicating if the
> > > > > > > > > profile shall claim exclusive access (such as GAP and GATT services),
> > > > > > > > > so profiles that don't set that will have the services exposed so for
> > > > > > > > > battery we can probably just have it exposed by default since it
> > > > > > > > > doesn't appear to would be any conflicts on having it exposed.
> > > > > > > > >
> > > > > > > > > > Sonny Sasaka (3):
> > > > > > > > > >   doc: Add "AllowInternalProfiles" property to org.bluez.Device1
> > > > > > > > > >   device: Add "AllowInternalProfiles" property to org.bluez.Device1
> > > > > > > > > >   client: Add set-allow-internal-profiles command
> > > > > > > > > >
> > > > > > > > > >  client/main.c      | 38 ++++++++++++++++++
> > > > > > > > > >  doc/device-api.txt | 13 +++++++
> > > > > > > > > >  src/device.c       | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  src/hcid.h         |  2 +
> > > > > > > > > >  src/main.c         | 10 +++++
> > > > > > > > > >  src/main.conf      |  4 ++
> > > > > > > > > >  6 files changed, 163 insertions(+)
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > 2.26.2
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Luiz Augusto von Dentz
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Luiz Augusto von Dentz
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz