| Summary: | wl_keyboard::keymap fd is shared and can be modified from any client | ||
|---|---|---|---|
| Product: | Wayland | Reporter: | 67b0226d |
| Component: | weston | Assignee: | Daniel Stone <daniel> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | medium | CC: | 67b0226d, bryce, daniel, derekf, giuliocamuffo, jadahl, levans, mailroxas, mgraesslin, ppaalanen, ran234, thiago |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Minimal example that makes the keymap invalid
Minimal example that makes the keymap invalid Reproducer that also tries to reopen /proc/<pid>/<fd> with write access simple refactor preceding bug fix Fix to prevent keymap trashing Reprodurer that also tries to chmod the tmpfile Patch for KWayland |
||
memfd_create solution seems to have the severe problem that file descriptors sealed with F_SEAL_WRITE cannot be mmaped() with MAP_SHARED, even as PROT_READ only. The solution would be to use MAP_PRIVATE, but still many existing clients that use MAP_SHARED (because of copy-paste and because it made no difference previously) would break. On Fedora 26, I cannot reproduce the keymap corruption with neither weston or mutter given the attached client, as the client fails with a SIGSEGV when trying to write the 'a' into keymap[0]. As far as I can tell, "keymap" points to something looking like a keymap, so it should be pointing to the shared memory. Created attachment 132254 [details]
Minimal example that makes the keymap invalid
Uploaded wrong version of the test program, sorry
Mutter bug (restricted, has patch): https://bugzilla.gnome.org/show_bug.cgi?id=784206 KDE bug (not yet restricted, no patch): https://bugs.kde.org/show_bug.cgi?id=381674 Enlightenment: reported privately (server down) wlc: reported privately (no private issue reporting) QtWayland: reported via mail to security contact smithay: reported privately (no private issue reporting) Westeros: attempting to report privately Chromium: reported privately I would like to co-ordinate disclosure/release for all these implementations. If you know of any I've missed, please also feel free to add them here. The reason is that libxkbcommon's parser is, frankly, awful, and full of holes. I'm up to 10 patches, mostly for NULL dereferences (or very very small offsets to NULL), some infinite recursion, an attempt to free() stack memory (seems bad), one unbounded heap walk which could in theory lead to use of arbitrary memory as a read-only string source, and one out-of-bounds stack dereference whose effect again seems extremely limited (if (stack_var[-12]) output[c] = 'x'). This is what I've found thus far through AFL and libFuzzer/UBSAN/ASAN/MSAN runs, though they are not yet fully eating their own corpus. I've also been manually walking through the parser/lexer and am confident I'll find at least a fair few more by targeting hotspots. The parser is _extremely_ generous in what it will consume, and there are about a million spots which don't verify that the input is what they would expect. For example, the parser will happily accept a geometry section in the middle of an interpret section. Given this, I'd like at least a couple of weeks to not only be able to be confident that I've bottomed out the XKB issues, as well as give the fuzzers an extended run at the parser with a much more aggressive corpus. I've contacted xorg-security@ to request two CVEs: one covering the generic issue of clients given shared writeable FDs for keymaps, and another covering the million xkbcommon vulnerabilities. My apologies for this: I'd specifically spent some time reading up on fuzzing (and writing parsers in Rust ...) over the past few weeks for fear of this exact same issue, and now it seems that time has arrived. :\ Nice catch. :( Reopening the file descriptor read only may present additional hazards on linux if we're still only keeping a single file descriptor in the compositor and sharing it with everyone, even if it is read only. Once that descriptor is in the hands of a client it, the underlying (unlinked) file may be reopened with write permissions via /proc/self/fd/ Unless proc isn't mounted, or a client is prevented from accessing its own /proc/self/fd/ directory in some way, then a client is still capable of messing with the compositor's keyboard map. It may be that we need to create a new keymap file per client instead of trying to share one. Created attachment 132291 [details] Reproducer that also tries to reopen /proc/<pid>/<fd> with write access (In reply to Derek Foreman from comment #6) > Nice catch. :( > > Reopening the file descriptor read only may present additional hazards on > linux if we're still only keeping a single file descriptor in the compositor > and sharing it with everyone, even if it is read only. > > Once that descriptor is in the hands of a client it, the underlying > (unlinked) file may be reopened with write permissions via /proc/self/fd/ Yea, passing a read-only fd doesn't help. I tried run both unsandboxed and from within a flatpak sandbox (by running the new reproducer on a "fixed" mutter in a io.github.GnomeMpv sandbox), and I can see how the keymap still gets permanently trashed for every client after running the reproducer). Created attachment 132307 [details] [review] simple refactor preceding bug fix Setting up all callers that send keymaps to go through a helper function so we can create unique fds in that helper function. It doesn't really stand on its own so I've attached it for review here. Created attachment 132308 [details] [review] Fix to prevent keymap trashing This fixes the problem in my testing, and is robust against jonas' trashkeymapharder reproducer I've also fixed this similarly in enlightenment locally. We've not created a phab ticket for this, as even with restricted permissions I'm not sure what the scope of visibility is. I'll hold off any public action until Daniel's ready to co-ordinate disclosure. I'm going to create a similar patch for KWayland. Don't know yet how to handle the review for it as in KDE infrastructure everything is open (we have neither restricted bug reports, nor phab requests) and on the other hand frameworks (which KWayland is part of) has a requirement for code review. (In reply to Martin Gräßlin from comment #11) > I'm going to create a similar patch for KWayland. Don't know yet how to > handle the review for it as in KDE infrastructure everything is open (we > have neither restricted bug reports, nor phab requests) and on the other > hand frameworks (which KWayland is part of) has a requirement for code > review. Phabricator does allow restriction of tasks to groups, but restricting code review changes is far more difficult. If you want to share something here, I will review it in context; if you have additional reviewers, I'd recommend either inviting them here or sharing it directly with them. A different solution: if the file descriptor is pointing to an actual file, you can make the file read-only. This can be accomplished atomically with: open(name, O_RDWR | O_CREAT | O_EXCL, 0444); The file descriptor will be open for writing into a file that no one else can open for writing, not even via reopening via /proc/<n>/fd. The compositor will need to reopen as read-only so that fd can be shared without passing on the write permissions. The above also works for O_TMPFILE. It does not work for memfd, since the "file" that backs memfd is created with write permissions. I recommend either changing the kernel to make reopening read-only (or unreadable) or adding an extra flag or parameter so that the permissions can be set. I'm not up to speed on how applications will be sandboxed - if a client is the same uid as the compositor, all it would have to do is chmod(2) /proc/self/fd/whatever and add back the appropriate permissions before the open() Are we concerned about the case where the compositor and client are the same uid? Created attachment 132327 [details] Reprodurer that also tries to chmod the tmpfile (In reply to Derek Foreman from comment #14) > I'm not up to speed on how applications will be sandboxed - if a client is > the same uid as the compositor, all it would have to do is chmod(2) > /proc/self/fd/whatever and add back the appropriate permissions before the > open() > This was my concern regarding this too, so I tried. The attached reproducer still works when creating the tmpfile with mode 0444. Works both outside and inside a flatpak sandbox (again, tried with io.github.GnomeMpv since it was relatively small). (In reply to Derek Foreman from comment #14) > Are we concerned about the case where the compositor and client are the same > uid? I wouldn't recommend being so. If the UID is the same, you can just kill(2) the compositor and everyone dies. There could be an escalation if another process by a different user is also connected to the compositor and, by way of corrupting the file, that process crashes. But I'd argue that the bug lies in the crashing process in the first place. Also, there isn't much in terms of security here, since the current process could also ptrace(2) the compositor and make it send anything to that other user. In case we really want to prevent materialisation of the file from /proc, just use O_EXCL with O_TMPFILE. (In reply to Thiago Macieira from comment #17) > In case we really want to prevent materialisation of the file from /proc, > just use O_EXCL with O_TMPFILE. How would I create a O_TMPFILE file with I can write to in weston but not in the client but not re-open as read-only (as there is no path to open)? (In reply to Jonas Ådahl from comment #18) > How would I create a O_TMPFILE file with I can write to in weston but not in > the client but not re-open as read-only (as there is no path to open)? weston opens it with open("/tmp", O_RDWR | O_TMPFILE, 0444). This will return a file descriptor with which you can write to the temporary file, but no one else will be able to open it for writing. To pass it to other processes, you should reopen it O_RDONLY at /proc/self/fd/%d. Now, a process with suitable permissions can still chmod the /proc/PID/fd/%d file and thus make it writable again. But as I said, processes with that permission can already do damage to your compositor anyway and other applications. This solution works for when the compositor is running as a different user and against accidental mistakes. (In reply to Thiago Macieira from comment #19) > (In reply to Jonas Ådahl from comment #18) > > How would I create a O_TMPFILE file with I can write to in weston but not in > > the client but not re-open as read-only (as there is no path to open)? > > weston opens it with open("/tmp", O_RDWR | O_TMPFILE, 0444). This will > return a file descriptor with which you can write to the temporary file, but > no one else will be able to open it for writing. > > To pass it to other processes, you should reopen it O_RDONLY at > /proc/self/fd/%d. Oh, I see. I thought you meant the fd would *not* be materialized from /proc this way, but it still does, just than one doesn't have to unlink() it. The fact operating on the fd via /proc/<pid>/fd/<fd> is the thing that we can't allow (see below). > > Now, a process with suitable permissions can still chmod the /proc/PID/fd/%d > file and thus make it writable again. But as I said, processes with that > permission can already do damage to your compositor anyway and other > applications. This solution works for when the compositor is running as a > different user and against accidental mistakes. But a process that can chmod() and open() /proc/self/fd/<fd> should still not be able to affect other clients or the compositor in any unwanted way. For example in a flatpak sandbox, a process can access /proc/<pid>/fd/* of itself and other processes in the same sandbox environment (which the compositor is not part of) and from there chmod and reopen the any fd at ease. (In reply to Thiago Macieira from comment #16) > I wouldn't recommend being so. If the UID is the same, you can just kill(2) > the compositor and everyone dies. > > There could be an escalation if another process by a different user is also > connected to the compositor and, by way of corrupting the file, that process > crashes. But I'd argue that the bug lies in the crashing process in the > first place. Also, there isn't much in terms of security here, since the > current process could also ptrace(2) the compositor and make it send > anything to that other user. ptrace is not the problem, /proc is. While ptrace can be limited by yama_scope (which many distributions already do by default), /proc still allows very potent access to the running environment of other processes including anonymous file descriptors. Why there is no option to restrict this is beyond me, but I guess we won't be able to do anything about it. Do note that with cgroups, UID being the same does not mean that you can see and/or kill all other processes running under that UID. Main concern here is breaking out of sandboxes by way of the compositor. When cgroups are in action, the /proc entries change too. But just like in the chroot case, I don't know what exactly /proc looks like. I know some things change (/proc/<PID>/mount, cwd, exe, etc.) but I don't know what else is left. This needs some investigation. And regarding ptrace, I used it as an example. There are other things that a process with the same UID as the compositor may be allowed to do, like kill(2) the compositor, which would be as damaging. This can be solved with cgroups. I'll take a look at what /proc looks like from a chroot'ed and changed user namespace. It's not just chroot. If you're in a new PID namespace, you can't try to ptrace the compositor, access it via /proc, etc. https://github.com/flatpak/flatpak/wiki/Sandbox says "/proc shows only the processes in the app sandbox" so in a flatpak's sandbox, you don't see the /proc/PID of the compositor in the first place. That happens because Flatpak unshares the PID namespace. If you don't unshare that, you can still see the /proc/PID of the compositor and, in turn, kill the compositor. ptracing the compositor does not work, though. My conclusion is still that if you can see the /proc/PID of the compositor, you haven't secured enough and therefore we do not need to protect against the chmod. Separate file descriptors do not protect against a chmod+open attack anyway, if the file is open. Thiago, sorry if I misunderstand what you're saying... I don't need access to /proc/<compositor_pid>/fd at all if the compositor is running as the same uid as the client. I can chmod the fd in my own /proc/self/fd directory, because it was created with my uid, and therefore I have permission to do so (even if it's already unlinked). The attempted fix I've provided for weston creates a new file for every wl_keyboard_send_keymap() event - and the client is free to mess with it in any way they choose because it's no longer the compositor's shared copy for all clients. If the proc space isn't properly unshared a client could mess with these files at creation time during the race between mkostemp and unlink - I completely agree with your conclusion that seeing the compositors /proc/PID indicates an environment that isn't secured enough. But I think file permissions won't help us because even when sandboxed we're probably the legitimate owner of the file. (In reply to Derek Foreman from comment #25) > Thiago, sorry if I misunderstand what you're saying... > > I don't need access to /proc/<compositor_pid>/fd at all if the compositor is > running as the same uid as the client. I can chmod the fd in my own > /proc/self/fd directory, because it was created with my uid, and therefore I > have permission to do so (even if it's already unlinked). Oh, I see. Right, I had not thought of the /proc/self case from the client. You're completely right, my suggestion won't work. Back to having different files/memfds per client. Created attachment 132375 [details] [review] Patch for KWayland I created a patch for KDE's Wayland server library. For references it's attached. I'm going to push as soon as Daniel is ready for disclosure. Thanks Martin. I'm still bottoming out the xkbcommon side of things, which will take a while still. I'll aim for end-of-July disclosure, to give two weeks for further fixes and a more targeted fuzzing run, plus a week or two for distributions to prepare. Friendly ping - any update on this? End of July was some time ago. Hi again, can we at least land the patches that we have already? Daniel, what's your current ETA on the xkbcommon stuff? > Hi again, can we at least land the patches that we have already?
+ 1, I would also like to land the patch.
It looks like we're all pretty eager to land this... weston 4.0 final is coming up quickly (rc1 this coming monday) and it would be really nice if we could include this fix. Would everyone be ok with that? This is beginning to approach its first birthday, what needs to be done before we can land patches? (In reply to Derek Foreman from comment #33) > This is beginning to approach its first birthday, what needs to be done > before we can land patches? Nothing from my side. Jonas, Martin, any idea on disclosure timeline? From my side the disclosure can happen any time. As in KDE's case Frameworks is affected we need at max one month till a public release. The patch is prepared. Personally I tend towards doing a public review once it's disclosed. I don't have an opinion about a disclosure timeline, but I guess there is no point in waiting for too long. FWIW, the patch in mutter is reviewed and ready to learn. Ok, have just landed the weston bits for this. Landed the fix in mutter too. Marking this as fixed, since the fixes have landed everywhere now. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Created attachment 132248 [details] Minimal example that makes the keymap invalid The xkb keymap is sent to clients using the wl_keyboard::keymap() event that includes a file descriptor that should be mmap()ed and then given to xkb_keymap_new_from_string. Although there is commonly no reason to do so, the fd can be mmap()ed with PROT_WRITE and MAP_SHARED flags, so the client can modify it. Since weston only uses one global fd that it shares with all clients, changes to the mmap()ed keymap are visible to other clients until the compositor changes keymap and thus resets the fd. This means that the keymap can be made invalid or replaced altogether. As the change is not announced by the compositor, this will only apply to newly started clients. Qt apps seem to crash at start when the keymap is invalid. This is definitely a bug and potentially a security hole because the Wayland protocol was designed specifically such that clients cannot interfere with each other. As a result of discussing this in IRC, the possibilities here seem to be: * Reopen the file descriptor read-only before handing it out to clients. For this to work, the unlink() call on the temporary file will have to be delayed until after the reopen. * Use memfd_create() and seal the file descriptor. This requires Linux >= 3.17. memfd is generally desirable because it eliminates the race where other applications can grab the temporary file before it is unlinked.