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
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.
I should add, your patch looks basically correct to me.
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].
(In reply to Jilles Tjoelker from comment #3) > The concept seems good but I disagree with the chosen error numbers. +1
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?
Created attachment 203274 [details] Reject /. and /.. with EINVAL
(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 '..'.
cem@ or jilles@ will you commit this? Torbjørn Birch Moltu would you care to add a test case?
Sure, I am happy to commit it.
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