Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

muelli
Copy link
Contributor

@muelli muelli commented Sep 21, 2023

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
Copy link
Contributor Author

muelli commented Sep 21, 2023

ref: #475

@muelli
Copy link
Contributor Author

muelli commented Mar 27, 2024

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

@Cropi
Copy link
Member

Cropi commented May 30, 2024

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
Copy link
Contributor Author

muelli commented May 30, 2024

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
Copy link
Member

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

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.

The daemon logs the exception and keeps running. But it appears
dysfunctional, i.e. does not notice new device let alone authorise any.
@muelli
Copy link
Contributor Author

muelli commented May 31, 2024

I've rebased the patch.

@radosroka radosroka merged commit 3672e9f into USBGuard:main Jun 3, 2024
5 of 14 checks passed
@hartwork hartwork mentioned this pull request Jun 4, 2024
@hartwork
Copy link
Contributor

hartwork commented Jun 4, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants