ddcutil icon indicating copy to clipboard operation
ddcutil copied to clipboard

incorrect access checks for I2C devices

Open Qyriad opened this issue 10 months ago • 5 comments

ddcutil uses access(2) to check that the relevant I²C devices exist and that its process has permission to write to them:

https://github.com/rockowitz/ddcutil/blob/618dbd8b4302f3a8eed66229faf69dd92d4a7ba1/src/app_ddcutil/main.c#L335

https://github.com/rockowitz/ddcutil/blob/618dbd8b4302f3a8eed66229faf69dd92d4a7ba1/src/app_ddcutil/main.c#L395

This is a time-of-check to time-of-use race condition, but more importantly, access() does not actually determine if the calling process can access the file, but if the calling user can access the file. From the man page:

The check is done using the calling process's real UID and GID, rather than the effective IDs as is done when actually attempting an operation (e.g., open(2)) on the file. Similarly, for the root user, the check uses the set of permitted capabilities rather than the set of effective capabilities; and for non-root users, the check uses an empty set of capabilities.

This allows set-user-ID programs and capability-endowed programs to easily determine the invoking user's authority. In other words, access() does not answer the "can I read/write/execute this file?" question. It answers a slightly different question: "(assuming I'm a setuid binary) can the user who invoked me read/write/execute this file?", which gives set-user-ID programs the possibility to prevent malicious users from causing them to read files which users shouldn't be able to read.

(Emphasis mine.)

This prevents using capabilities or setuid to allow non-root users to use ddcutil. ddcutil should instead simply use open() to determine if it can actually do so.

Qyriad avatar Apr 29 '25 09:04 Qyriad

Thank you for the clear discussion of the issue. I will have to think through the implications.

If what you're trying to do is give non-root users the ability to use ddcutil, the udev rule installed by ddcutil uses tag uaccess to give the logged on user RW permission for ddcutil. Does this not address your use case?

rockowitz avatar Apr 30 '25 04:04 rockowitz

@rockowitz The uaccess tag is excellent for the use case of using ddcutil while sitting in front of your computer, but (intentionally) doesn't cover remote access cases. For my case I need to switch monitor inputs sometimes before the user has logged on locally at all. I can of course just run ddcutil as root, which is what I'm currently doing, but I would prefer to not have to.

It's also really useful for testing purposes.

Qyriad avatar May 10 '25 11:05 Qyriad

How can you run ddcutil without being logged on as some user? Or are you running without the udev rules having been executed?

I have put a set of changes into branch 2.2.1-dev that bypass use of access() if running as root with the setuid bit set. Does this address your need?

rockowitz avatar May 15 '25 20:05 rockowitz

How can you run ddcutil without being logged on as some user? Or are you running without the udev rules having been executed?

You can be logged on as a user without being logged on locallyuaccess is specifically for the local seat. Automated and remote processes are intentionally not covered by uaccess.

I have put a set of changes into branch 2.2.1-dev that bypass use of access() if running as root with the setuid bit set. Does this address your need?

Not as I intended, as I intended to use capabilities(7) for my use case rather than setuid, to limit the privileges given.

Qyriad avatar May 19 '25 12:05 Qyriad

Given the imminence of its release, I'm afraid no further changes can be made in release 2.2.1 regarding this enhancement request. I'll keep it in mind for the next release.

rockowitz avatar May 22 '25 12:05 rockowitz