Hi Luiz, On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > Hi Sonny, > > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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. These are two different issues. The issue with bad applications can be overcome with D-Bus policy. The issue with multiple providers is inherent: we have to have a duplicate resolution because one device may report battery from different sources, e.g. via HFP and A2DP at the same time. The latter case is rare in practice but still possible, so I propose the simplest duplication resolution which is accept the first provider registered (of that specific device) and ignore the rest. Coupling Battery1 with Profile1 will limit the flexibility of this feature. For example, some speakers report battery status via a separate LE channel (GATT). If we require Battery provider to be also a registered Profile, we won't be able to support these cases since GATT clients do not register any profile. > > > > > > > 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
