Skip to content

Detect buffer overflow in fcntl.fcntl() and fcntl.ioctl() #132915

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

Closed
serhiy-storchaka opened this issue Apr 25, 2025 · 3 comments
Closed

Detect buffer overflow in fcntl.fcntl() and fcntl.ioctl() #132915

serhiy-storchaka opened this issue Apr 25, 2025 · 3 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Apr 25, 2025

fcntl() and ioctl() take an argument which can be a pointer to a buffer of unspecified length, depending on operation. They can also write in that buffer, depending on operation. A temporary buffer of size 1024 is used, so a chance of directly overflowing the bytes-like object provided by user is small, but if its size than necessary, the user will get truncated data in best case, and in worst case it will cause the C stack corruption.

We cannot prevent this, unless we limit the set of supported operations to a small set of allowed operations. This is not practical, because fcntl() and ioctl() exist to support operations not explicitly supported by Python. But we can detect a buffer overflow, and raise an exception. It may be too late, if the stack or memory are corrupted, but it is better than silently ignore error.

Linked PRs

@serhiy-storchaka serhiy-storchaka self-assigned this Apr 25, 2025
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Apr 25, 2025
SystemError is raised when buffer overflow is detected.
The stack and memory can already be corrupted, so treat this error as fatal.
@picnixz picnixz added type-feature A feature request or enhancement extension-modules C modules in the Modules dir labels Apr 25, 2025
@serhiy-storchaka
Copy link
Member Author

This is one of rare cases when a Python code can trigger a SystemError. But these functions are as unsafe as ctypes, so I think that it is appropriate use of SystemError.

@vstinner
Copy link
Member

fcntl() and ioctl() take an argument which can be a pointer to a buffer of unspecified length, depending on operation.

Do you have examples?

Before you, nobody requested this feature, so I'm a little surprised that this feature is really needed.

@serhiy-storchaka
Copy link
Member Author

The third argument of fcntl() and ioctl() is either void (ignored), int, or a pointer. For example, F_SETLK, F_SETLKW, and F_GETLK operations in fcntl() need a pointer to struct flock. struct flock should have some fields (l_type, l_whence, etc), but the size of the structure, and even the order of the fields are different on different OS. If you pass a bytes object of wrong size to fcntl.fcntl(), F_GETLK will return a truncated data of wrong size, and F_SETLK, F_SETLKW can read and use unitialized memory from a buffer variable on stack. F_KINFO on FreeBSD needs a pointer to a structure whose layout changed several times between the OS versions.

We cannot do anything with this. Unlike getsockopt()/setsockopt() we cannot specify the size of the buffer, so the OS cannot check it and fail if it is not enough. It will just read/write past the end of the buffer. But I hope that we can make the error more loud in some cases (either by raising SystemError after buffer overflow happened or by making a crash more reproducible).

See also #95380. It is more dangerous when the size exceeds 1024 and we read/write directly from/to the bytes-like object, but I hope that the user will be more accurate there.

serhiy-storchaka added a commit that referenced this issue Apr 28, 2025
…-132919)

SystemError is raised when buffer overflow is detected.
The stack and memory can already be corrupted, so treat this error as fatal.
vstinner added a commit to vstinner/cpython that referenced this issue May 1, 2025
Fix the warning:

    Modules/fcntlmodule.c:27:36: warning: initializer-string for
    array of 'char' truncates NUL terminator but destination lacks
    'nonstring' attribute (9 chars into 8 available)
    [-Wunterminated-string-initialization]
    static const char guard[GUARDSZ] = "\x00\xfa\x69\xc4\x67\xa3\x6c\x58";
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants