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> <815b138fb849b56a5ec71045b54c86f99ed9df2c.camel@hadess.net>
In-Reply-To: <815b138fb849b56a5ec71045b54c86f99ed9df2c.camel@hadess.net>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Thu, 19 Nov 2020 12:20:12 -0800
Message-ID: <CAO271mn-3qK6TqDUf2ua06EAN58K592ondrZjC-pt6byPQXG8Q@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API
To:     Bastien Nocera <hadess@hadess.net>
Cc:     BlueZ <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Bastien,

On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka 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.
>
> That wouldn't solve it, the point is to avoid one user causing problems
> for another logged in user. If both users are in the audio group, which
> they'd likely be to be able to use the computer, they'd be able to
> cause problems to each other.

If I understand your case correctly, both users being in "audio" group
still won't allow them both to become battery providers because the
D-Bus policy only allows "audio" user and not "audio" group.


>
> >
> > >
> > > 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 haven't looked at the code in depth, but I would expect property
> types to be checked before being exported, rather than relying on the
> original dbus type matching the expected export type, this sort of
> thing.
Yes, the code does not just forward information. The code processes
the input and exports it as a clearly defined output type.
>
> >
> > >
> > > 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
> > >
>
>