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

ParseArgs unnecessary casting leads to wrong values #4504

Open
rscampos opened this issue Jan 14, 2025 · 4 comments
Open

ParseArgs unnecessary casting leads to wrong values #4504

rscampos opened this issue Jan 14, 2025 · 4 comments
Labels

Comments

@rscampos
Copy link
Collaborator

rscampos commented Jan 14, 2025

Description

I'll open an issue since I don't if the current approach is correct or not. Once we discuss it, I can submit a PR.

I was reviewing #4442 when I realized that some casting might lead to incorrect values.

The syscall execveat has the field flags as type int in the definition:

{Type: "int", Name: "flags"},

But here happens a cast to uint64 before passing parseExecFlag:

case Execveat:
if flagsArg := GetArg(event, "flags"); flagsArg != nil {
if flags, isInt32 := flagsArg.Value.(int32); isInt32 {
parseExecFlag(flagsArg, uint64(flags))
}
}

The issue happens when we call execveat using flag=-1. The value will be converted to 0xFFFFFFFFFFFFFFFF, setting all flags:

sudo ./dist/tracee -e execveat -s comm=python3.10
TIME             UID    COMM             PID     TID     RET              EVENT                     ARGS
11:29:25:554926  1000   python3.10       638196  638196  0                execveat                  dirfd: -100, pathname: /bin/ls, argv: [/bin/ls], envp: 0x0, flags: AT_EMPTY_PATH|AT_SYMLINK_NOFOLLOW|AT_EACCESS|AT_REMOVEDIR|AT_NO_AUTOMOUNT|AT_STATX_SYNC_TYPE|AT_STATX_FORCE_SYNC|AT_STATX_DONT_SYNC|AT_RECURSIVE
11:29:25:554926  1000   python3.10       638196  638196  -22              execveat                  dirfd: -100, pathname: /bin/ls, argv: [/bin/ls], envp: 0x0, flags: AT_EMPTY_PATH|AT_SYMLINK_NOFOLLOW|AT_EACCESS|AT_REMOVEDIR|AT_NO_AUTOMOUNT|AT_STATX_SYNC_TYPE|AT_STATX_FORCE_SYNC|AT_STATX_DONT_SYNC|AT_RECURSIVE

Usually, this behavior occurs when the type of the event in the definition is signed and we cast the negative value to unsigned. I think each function should have the signature aligned with the type of the field from the event defined in the definition.

Output of tracee version:

(paste your output here)

Output of uname -a:

(paste your output here)

Additional details

Python code to call execveat with flag=-1.

import ctypes
import errno
import os

SYS_EXECVEAT = 281  # ARM

AT_FDCWD = -100
exec_path = b"/bin/ls"
argv = (ctypes.c_char_p * 2)()
argv[0] = exec_path
argv[1] = None  # Null-terminated argument list

# Environment variables (null-terminated array)
envp = (ctypes.c_char_p * 1)()
envp[0] = None

# trigger
flags = -1

libc = ctypes.CDLL("libc.so.6", use_errno=True)

ret = libc.syscall(SYS_EXECVEAT, AT_FDCWD, exec_path, ctypes.byref(argv), ctypes.byref(envp), flags)

if ret == -1:
    err = ctypes.get_errno()
    print(f"Error: {os.strerror(err)} (errno={err})")
    if err == errno.ENOENT:
        print("File not found.")
    elif err == errno.EACCES:
        print("Permission denied.")
    else:
        print("Unexpected error occurred.")
else:
    print("Execveat syscall succeeded.")
@geyslan
Copy link
Member

geyslan commented Jan 14, 2025

I totally agree.

But before tackling it let's get this one in?

#4279

It has fixes and improvements which need to be followed.

@ShohamBit
Copy link
Collaborator

looks good to me, should I add it to my pr? @rscampos #4442

@rscampos
Copy link
Collaborator Author

@ShohamBit I think we should have another PR for that since we need to do some refactoring and testing, WDYT?

@ShohamBit
Copy link
Collaborator

sounds good

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

No branches or pull requests

3 participants