search

USBGuard / usbguard

USBGuard is a software framework for implementing USB device authorization policies (what kind of USB devices are authorized) as well as method of use policies (how a USB device may interact with the system)
https://usbguard.github.io/
GNU General Public License v2.0
1.1k stars 133 forks source link

Restore support for access control filenames without a group (fixes #540) #541

Closed hartwork closed 2 years ago

hartwork commented 2 years ago

Fixes #540

Regression from commit b15ef713a9ac47e84525bbf829c7f444b84c3c81 of release 1.1.0, detailed analysis online at https://github.com/USBGuard/usbguard/issues/540#issuecomment-1059784284 .

ZoltanFridrich commented 2 years ago

@hartwork Please, add the same check for username as well. You can have ACL files in a form ":groupname", ie. with empty username.

hartwork commented 2 years ago

@hartwork Please, add the same check for username as well. You can have ACL files in a form ":groupname", ie. with empty username.

@ZoltanFridrich @Cropi good point, thanks for speaking up about it! I wonder if cases user: and : should be (or were previously) supported. I have updated the PR with a version including those two cases and adding a comment about all the effectively supported variations. Please re-check, pushed.

ZoltanFridrich commented 2 years ago

LGTM! I am not sure how it works with : name. Does it work like a global IPC ACL meaning that it gets applied for everyone?

hartwork commented 2 years ago

LGTM! I am not sure how it works with : name. Does it work like a global IPC ACL meaning that it gets applied for everyone?

That's a good point to bring up! From looking at IPCServerPrivate::matchACLByName code, my impression is that we are dealing with algorithm "start with nothing, then match user OR match group" rather than "start with everyone, then cut things away". I believe that filename : does effectively match no one (rather than everyone). I'm not sure that's the best way to implement it, but we probably cannot change semantics of it before 2.0.0 if we wanted to. My personal focus would be to support what was supported in 1.0.0. @ZoltanFridrich what do you think?

ZoltanFridrich commented 2 years ago

I was just curious about : name situation. I agree with you and I think this PR can be merged.