Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201111011745.2016-1-sonnysasaka@chromium.org>
 <20201111011745.2016-7-sonnysasaka@chromium.org> <aa1c080e8a7813299e6a093608211684e074e427.camel@hadess.net>
 <CAO271m=O3hyS6Pp4fQ1pnsir7wYbLFwDm7f-a5yd0o4NTUUewA@mail.gmail.com>
In-Reply-To: <CAO271m=O3hyS6Pp4fQ1pnsir7wYbLFwDm7f-a5yd0o4NTUUewA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 17 Nov 2020 14:26:30 -0800
Message-ID: <CABBYNZJjgdjsDXOQk=v4wJ7PLyHr-u2w6d+jgLKgwxV9J5OYdQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     Bastien Nocera <hadess@hadess.net>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Sonny,

On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Bastien,
>
> Thank you for the feedback. Please find my answers below.
>
> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > Hey Sonny,
> >
> > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > This patch implements the BatteryProvider1 and
> > > BatteryProviderManager1
> > > API. This is a means for external clients to feed battery information
> > > to
> > > BlueZ if they handle some profile and can decode battery reporting.
> > >
> > > The battery information is then exposed externally via the existing
> > > Battery1 interface. UI components can consume this API to display
> > > Bluetooth peripherals' battery via a unified BlueZ API.
> >
> > Was this patch reviewed for potential security problems? From the top
> > of my head, the possible problems would be:
> > - I don't see any filters on which user could register battery
> > providers, so on a multi user system, you could have a user logged in
> > via SSH squatting all the battery providers, while the user "at the
> > console" can't have their own providers. Also, what happens if the user
> > at the console changes (fast user switching)?
> > - It looks like battery providers don't check for paired, trusted or
> > even connected devices, so I would be able to create nearly unbound
> > number of battery providers depending on how big the cache for "seen"
> > devices is.
> For security, the API can be access-limited at D-Bus level using D-Bus
> configuration files. For example, we can let only trusted UNIX users
> as the callers for this API. This D-Bus config file would be
> distribution-specific. In Chrome OS, for example, only the "audio" and
> "power" users are allowed to call this API. This way we can make sure
> that the callers do not abuse the API for denial-of-service kind of
> attack.

I guess there is still some the risk of conflicts even with the use of
D-Bus policy blocking roge applications, there could still be multiple
entities providing the battery status from the same source, which is
why I suggested we couple the Battery1 with the Profile1, so only the
entity that has registered to handle let say HFP can provide a battery
status and we automatically deduce the source is from HFP.

> >
> > Given that the interface between upower and bluez is supposedly
> > trusted, it might be good to ensure that there are no fuzzing problems
> > on the bluez API side that could translate to causing problems in
> > upower itself.
> Could you give an example of what potential problems of upower can be
> caused by communicating with BlueZ through this API?
>
> >
> > I didn't review the code in depth, but, having written this mechanism
> > for Bluetooth battery reporting, I think that this is the right way to
> > go to allow daemons like pulseaudio to report battery status.
> >
> > Cheers
> >


-- 
Luiz Augusto von Dentz