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. > > > > 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
