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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions volatility3/framework/plugins/linux/pslist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# This file is Copyright 2021 Volatility Foundation and licensed under the Volatility Software License 1.0
# which is available at https://www.volatilityfoundation.org/license/vsl-v1.0
#
import logging
import datetime
import dataclasses
import contextlib
Expand All @@ -15,6 +16,8 @@
from volatility3.plugins import timeliner
from volatility3.plugins.linux import elfs

vollog = logging.getLogger(__name__)


@dataclasses.dataclass
class TaskFields:
Expand Down Expand Up @@ -112,6 +115,7 @@ def get_task_fields(
A TaskFields object with the fields to show in the plugin output.
"""
name = utility.array_to_string(task.comm)

if decorate_comm:
if task.is_kernel_thread:
name = f"[{name}]"
Expand Down Expand Up @@ -250,6 +254,14 @@ def list_tasks(

# Note that the init_task itself is not yielded, since "ps" also never shows it.
for task in init_task.tasks:
# the task list is often smeared in samples, espeically towards the end
# 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.

)
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


if filter_func(task):
continue

Expand Down
16 changes: 16 additions & 0 deletions volatility3/framework/symbols/linux/extensions/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,22 @@ def section_strtab(self):


class task_struct(generic.GenericIntelProcess):
def has_valid_name_and_pid(self) -> bool:
"""
Ensures the pid and name for this process are sane
When these are broken it is a sure sign of smear
during process enumeration
"""
try:
pid = self.pid
name = utility.array_to_string(self.comm)
except exceptions.InvalidAddressException:
return False

# 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,)


def add_process_layer(
self, config_prefix: Optional[str] = None, preferred_name: Optional[str] = None
) -> Optional[str]:
Expand Down
Loading