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.12k stars 138 forks source link

ruleset: detect integer overflow of the ID and bail out #600

Closed muelli closed 4 months ago

muelli commented 1 year ago

The check is semantically correct, because some IDs are reserved. Here, we exploit the fact that the LastID is defined to be max_int-2, so when we increment _id_next often enough, we will eventually reach the reserved LastID. If that ID is reached, we bail out to prevent further damage. We could have used the __builtin_add_overflow GCC extension but I don't know how compatible it is. Besides, we probably want to prevent the ID spilling into the reserved LastID anyway.

The daemon logs the exception and keeps running. But it appears dysfunctional, i.e. does not notice new device let alone authorise any. That may or may not be the desired behaviour. But it serves as base for discussion and is probably more desired than the behaviour with the overflow.

To test the change, I have the following line in the assignID function:

    const auto base = 1 ? (Rule::LastID - 10) : 0 ;
    const auto next_id = _id_next + 1  +  base;
    USBGUARD_LOG(Debug) << "Base: " << 0 << ", Current RuleID: " << _id_next << ", next " << next_id << ", (" << Rule::LastID << ")";
    ...
    _id_next = next_id - base;
muelli commented 1 year ago

ref: https://github.com/USBGuard/usbguard/issues/475

muelli commented 6 months ago

@Cropi do you have any opinion on the integer overflow?

Cropi commented 4 months ago

Hi @muelli, Thanks for the PR! I think it would be better to check if the new device ID is already assigned to an existing device. With this approach, we could search for a new ID when a device is inserted. In case there is no free ID (someone is messing with us) then we could throw the exepction as it is now. What do you think?

muelli commented 4 months ago

ah, sounds like a valid approach. More sane, anyway, than a device obtaining the reserved LastID. I wouldn't know how to test for the availability of an ID. I also wonder how a device could get an occupied ID, given that the ID is incremented after an assignment.

radosroka commented 4 months ago

I think this can be merged for now. @muelli can you rebase PR branch?

muelli commented 4 months ago

I've rebased the patch.

hartwork commented 4 months ago

@muelli @radosroka this commit and pull request broke the CI on multiple levels:

As such and with far-from-green CI, this pull request should not have been merged as-is in the first place.

In the future, please let's keep CI green, fix red CI before anything else, and not merge things that are not green. Thank you!

New pull request #628 provides fixes to all of the regression aspects and more, for a fully green CI. I'm happy to adjust details as needed, I believe it's the right way forward. Please let me know how you feel about it :pray:

CC @Cropi