ruleset: detect integer overflow of the ID and bail out

[muelli]

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]

ref: #475

[muelli]

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

[Cropi]

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]

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]

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

[muelli]

I've rebased the patch.

[hartwork]

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

• it started using unsupported post-C++17 features,
• it changed return x++ semantics to return ++x,
• which in turn broke the test suite (which speaks for the tests), and
• it was not suitably formatted.

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 🙏

CC @Cropi