Bug 215202
| Summary: | Linuxulator: LOCAL_PEERCRED / xucred does not have process ID | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Val Packett <val> | ||||
| Component: | kern | Assignee: | Dmitry Chagin <dchagin> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | dchagin, emaste, hselasky, jbeich, jilles | ||||
| Priority: | --- | Flags: | jbeich:
mfc-stable12?
|
||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 247219 | ||||||
| Attachments: |
|
||||||
|
Description
Val Packett
2016-12-10 22:15:44 UTC
Created attachment 188271 [details]
peercred.patch
Well, that was easy. Here's a tiny patch that fixes the problem.
Should XUCRED_VERSION be raised? Adding an extra field at the end of the struct doesn't break backwards compatibility…
The pid in LOCAL_PEERCRED is not a security feature but only a feature to enforce that a proper security feature can be added later. This is because there is nothing to enforce that the pid refers to the same process or that it has not executed some other binary; also, applications are not isolated in a way that makes different Wayland privileges useful for security. Adding pid to struct ucred seems wrong since it is shared between processes that do not change their credentials. This would make more sense as a property of the socket. The pid would then be the pid of the process that called listen() or connect(). (In reply to Jilles Tjoelker from comment #2) sway just uses the pid to read /proc/PID/file and check the file against a list of allowed files. The pid is in ucred on Linux, I think it makes sense to match what they do. A commit references this bug: Author: dchagin Date: Thu May 30 14:24:28 UTC 2019 New revision: 348419 URL: https://svnweb.freebsd.org/changeset/base/348419 Log: Complete LOCAL_PEERCRED support. Cache pid of the remote process in the struct xucred. Do not bump XUCRED_VERSION as struct layout is not changed. PR: 215202 Reviewed by: tijl MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D20415 Changes: head/crypto/heimdal/lib/ipc/server.c head/share/man/man4/unix.4 head/sys/compat/linux/linux_socket.c head/sys/kern/kern_prot.c head/sys/kern/uipc_usrreq.c head/sys/sys/ucred.h head/usr.sbin/mountd/mountd.c A commit references this bug: Author: dchagin Date: Mon Jun 10 05:28:04 UTC 2019 New revision: 348847 URL: https://svnweb.freebsd.org/changeset/base/348847 Log: Use C11 anonymous unions. PR: 215202 Reported by: glebius MFC after: 2 weeks Changes: head/sys/sys/ucred.h Anything needed here (other than perhaps MFC)? (In reply to Ed Maste from comment #6) Nah, it's solved. Actually back when I first looked at the relevant commits I somehow got to the conclusion that it was special cased for Linuxulator only. But more recently I've discovered that this has been actually properly available to FreeBSD binaries :) I guess MFC could be nice but I personally don't really care about STABLE/releases. I'd prefer MFC to /stable/12 to make porting easier: /stable/11 will reach EOL by the time 12.3 is released. Currently, sysutils/basu, sysutils/seatd, graphics/wayland, lang/rust have to return cr_pid = -1 as a workaround. A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=925f44f33862908f9a2e72520a17af148c7d0db5 commit 925f44f33862908f9a2e72520a17af148c7d0db5 Author: Dmitry Chagin <dchagin@FreeBSD.org> AuthorDate: 2019-05-30 14:24:26 +0000 Commit: Dmitry Chagin <dchagin@FreeBSD.org> CommitDate: 2021-03-21 05:33:42 +0000 Complete LOCAL_PEERCRED support. Cache pid of the remote process in the struct xucred. Do not bump XUCRED_VERSION as struct layout is not changed. PR: 215202 Differential Revision: https://reviews.freebsd.org/D20415 (cherry picked from commit c5afec6e895a11c64f58eb99e493adb8ad5dc361) crypto/heimdal/lib/ipc/server.c | 2 +- share/man/man4/unix.4 | 1 + sys/compat/linux/linux_socket.c | 5 +---- sys/kern/kern_prot.c | 8 ++++++++ sys/kern/uipc_usrreq.c | 8 ++++---- sys/sys/ucred.h | 7 ++++++- usr.sbin/mountd/mountd.c | 2 +- 7 files changed, 22 insertions(+), 11 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=2b61bda2c75f30f6eadd18fb891fd885e4c8d19d commit 2b61bda2c75f30f6eadd18fb891fd885e4c8d19d Author: Dmitry Chagin <dchagin@FreeBSD.org> AuthorDate: 2019-06-10 05:28:03 +0000 Commit: Dmitry Chagin <dchagin@FreeBSD.org> CommitDate: 2021-03-21 10:32:03 +0000 Use C11 anonymous unions. PR: 215202 Reported by: glebius (cherry picked from commit a5ec4a9dba5629dfe146ae9534e91e5e957747eb) sys/sys/ucred.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) |
