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.
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
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:
Date: Tue May 21 21:26:14 UTC 2019
New revision: 348067
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.
Submitted by: Torbj?rn Birch Moltu <t.b.moltu AT lyse.net>
Discussed with: jilles (earlier version)