|Summary:||Kernel panic from calling mq_open("/.", ...) as root|
|Product:||Base System||Reporter:||Torbjørn Birch Moltu <t.b.moltu>|
|Component:||kern||Assignee:||freebsd-bugs mailing list <bugs>|
|Severity:||Affects Only Me||CC:||cem, emaste, jilles|
Description Torbjørn Birch Moltu 2019-03-27 21:44:09 UTC
Created attachment 203197 [details] Untested patch for rejecting "/." and "/.." with EACCES After loading the mqueuefs module, calling mq_open() with "/.." or "/." as name in a C program run by root crashes the system. I assume it's a panic but it reboots too quickly to read the text. Doing this as non-root does nothing and EACCES is produced. mq_unlink("/.") as root successfully removes . from mqueuefs, and mq_unlink("/..") as root removes both .. and . Trying to unlink either as non-root just produces EACCES. After this a non-root user can create queues with these names and use them as any other queue. Listing the directory where mqueuefs is mounted while . or .. exists as queues also crashes the system. I have not tested whether programs running inside jails can cause this crash or also get EACCES. I've created a patch which I think should fix this, but I haven't tested it at all. I wasn't sure whether to pick 12.0-RELEASE or 12.0-STABLE; uname -a says: FreeBSD freebsd 12.0-RELEASE FreeBSD 12.0-RELEASE r341666 GENERIC amd64
Comment 1 Conrad Meyer 2019-03-27 22:51:00 UTC
Yeah, I see the bug. I think kern_kmq_open basically allows you to open any arbitrary single-component name you want in the mqfs, including "." and "..", which are special and reserved. I didn't try to replicate the exact panic, but the bug seems present in CURRENT as well.
Comment 2 Conrad Meyer 2019-03-27 22:51:56 UTC
I should add, your patch looks basically correct to me.
Comment 3 Jilles Tjoelker 2019-03-30 22:45:04 UTC
The concept seems good but I disagree with the chosen error numbers. Quotes below are from POSIX.1-2016. For mq_open(), [EINVAL] "The mq_open() function is not supported for the given name." seems to fit best. For mq_unlink(), [ENOENT] "The named message queue does not exist." seems plausible, or also [EINVAL].
Comment 4 Conrad Meyer 2019-03-31 04:59:31 UTC
(In reply to Jilles Tjoelker from comment #3) > The concept seems good but I disagree with the chosen error numbers. +1
Comment 5 Torbjørn Birch Moltu 2019-03-31 16:18:57 UTC
My reasoning for choosing EACCES is that it's what unprivileged processes already get, and Linux also returns this error. I agree that EINVAL is more accurate though, and compatibility shouldn't matter in this case. I'm a bit surprised that the approach in my patch is the proper solution. Wouldn't it be better to check the type of node before opening or unlinking it?
Comment 6 Torbjørn Birch Moltu 2019-03-31 16:23:07 UTC
Created attachment 203274 [details] Reject /. and /.. with EINVAL
Comment 7 Conrad Meyer 2019-03-31 18:54:15 UTC
(In reply to Torbjørn Birch Moltu from comment #5) > I'm a bit surprised that the approach in my patch is the proper solution. > Wouldn't it be better to check the type of node before opening or unlinking > it? Nodes don't exist in the mqfs namespace for '.' and '..'.
Comment 8 Ed Maste 2019-05-21 18:53:11 UTC
cem@ or jilles@ will you commit this? Torbjørn Birch Moltu would you care to add a test case?
Comment 9 Conrad Meyer 2019-05-21 20:24:55 UTC
Sure, I am happy to commit it.
Comment 10 commit-hook 2019-05-21 21:26:20 UTC
A commit references this bug: Author: cem Date: Tue May 21 21:26:14 UTC 2019 New revision: 348067 URL: https://svnweb.freebsd.org/changeset/base/348067 Log: mqueuefs: Do not allow manipulation of the pseudo-dirents "." and ".." "." and ".." names are not maintained in the mqueuefs dirent datastructure and cannot be opened as mqueues. Creating or removing them is invalid; return EINVAL instead of crashing. PR: 236836 Submitted by: Torbj?rn Birch Moltu <t.b.moltu AT lyse.net> Discussed with: jilles (earlier version) Changes: head/sys/kern/uipc_mqueue.c