Bug 236836 - Kernel panic from calling mq_open("/.", ...) as root
Summary: Kernel panic from calling mq_open("/.", ...) as root
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: panic, patch
Depends on:
Blocks:
 
Reported: 2019-03-27 21:44 UTC by Torbjørn Birch Moltu
Modified: 2019-05-21 21:26 UTC (History)
3 users (show)

See Also:


Attachments
Untested patch for rejecting "/." and "/.." with EACCES (806 bytes, patch)
2019-03-27 21:44 UTC, Torbjørn Birch Moltu
no flags Details | Diff
Reject /. and /.. with EINVAL (806 bytes, patch)
2019-03-31 16:23 UTC, Torbjørn Birch Moltu
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 freebsd_committer 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 freebsd_committer 2019-03-27 22:51:56 UTC
I should add, your patch looks basically correct to me.
Comment 3 Jilles Tjoelker freebsd_committer 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 freebsd_committer 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 freebsd_committer 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 freebsd_committer 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 freebsd_committer 2019-05-21 20:24:55 UTC
Sure, I am happy to commit it.
Comment 10 commit-hook freebsd_committer 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