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

Prevent Linux's list_tasks from yielding completely broken process in… #1518

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

atcuno
Copy link
Contributor

@atcuno atcuno commented Jan 4, 2025

…stances

@atcuno atcuno requested a review from ikelos January 4, 2025 03:32
Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

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

Yep, looks good, minor text alteration and a question but I'm happy otherwise. 5:)

# this stops the processing
if not task.has_valid_name_and_pid():
vollog.debug(
f"Found an smeared/invalid task at offset {task.vol.offset:#x}"
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the smeared/ from this because otherwise the an sticks out.


# ensure the pid within the bounds of a signed int
# and that we have something of a name
return (0 < pid < 2147483647) and len(name) > 0
Copy link
Member

Choose a reason for hiding this comment

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

If the type of pid is signed int, can it ever be outside these bounds?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we opening the door to task hiding in memory forensics with this? Writing \x00 to the comm field could even be done from user space with just a syscall. Also, we definitely shouldn't use break there. Doing so not only allows a single task to hide but also exposes all subsequent tasks to the same risk.

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor but technically speaking it has to be (0 < pid <= 2147483647)

>>> struct.unpack('i', b"\xff\xff\xff\x7f")
(2147483647,)
>>> struct.unpack('i', b"\xff\xff\xff\x80")
(-2130706433,)

vollog.debug(
f"Found an smeared/invalid task at offset {task.vol.offset:#x}"
)
break
Copy link
Contributor

Choose a reason for hiding this comment

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

continue please

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

Successfully merging this pull request may close these issues.

3 participants