Hi Bastien, On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@xxxxxxxxxx> 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@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. > > 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 > > > > >
