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

Fix a couple CWE issues in ManyMouse #2040

Merged
merged 4 commits into from
Oct 23, 2022
Merged

Fix a couple CWE issues in ManyMouse #2040

merged 4 commits into from
Oct 23, 2022

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Oct 22, 2022

Suggest reviewing commit by commit.

@kcgen kcgen added cleanup Non-functional changes that simplify, improve maintainability, or squash warnings external Depends on something outside of this project labels Oct 22, 2022
@kcgen kcgen self-assigned this Oct 22, 2022
src/libs/manymouse/linux_evdev.c Show resolved Hide resolved
src/libs/manymouse/linux_evdev.c Show resolved Hide resolved
In pump_events, the event struct members are used initialized in
three places:

1. Line 479, the event struct is passed into queue_event() which
   accesses the event's minval member, but it hasn't been written to
   at this point.

2. Line 488, the event struct is passed into queue_event() which
   accesses the event's minval member, but it hasn't been written to
   at this point.

3. Line 505, the event struct is passed into queue_event() which
   accesses the event's item member, but it hasn't been written to at
   this point.
Copy link
Collaborator

@FeralChild64 FeralChild64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me. It seems it still works :)

Fixes:
 warning: implicit conversion changes signedness:
 'unsigned int' to 'int'
TOCTOU security issues exists between a check event and a use event
in which an attacker can change properties about the thing being
used.

In this case, the code checked several stat-based properties about
the mouse's character-device file path, and once verified, opened
the the file path as a bona fide mouse device.

Because the same open file descriptor isn't maintained across the
checks through to the usage, we can't be guaranteed that file used
is the one we checked (the crux of the TOCTOU vulnerability).

To fix it, we hold the sample file descriptor across the checks and
usage.
@kcgen kcgen merged commit 41498dc into main Oct 23, 2022
@pr-triage pr-triage bot added the PR: merged label Oct 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the kc/manymouse-cwes-1 branch October 23, 2022 13:33
@kcgen kcgen removed the PR: merged label Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non-functional changes that simplify, improve maintainability, or squash warnings external Depends on something outside of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants