Bug 246412 - Return EISDIR when reading a directory
Summary: Return EISDIR when reading a directory
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Kyle Evans
URL: https://reviews.freebsd.org/D24596
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-12 02:46 UTC by Xin LI
Modified: 2020-07-08 18:29 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xin LI freebsd_committer freebsd_triage 2020-05-12 02:46:25 UTC
FreeBSD have shipped with readdir() and allowing read() on directories is not very useful.

Other POSIX compliant operating systems like macOS returns EISDIR when read()ing a directory too, it is allowed in the specification:

Quote https://pubs.opengroup.org/onlinepubs/009695399/functions/read.html :

EISDIR

[XSI] The fildes argument refers to a directory and the implementation does not allow the directory to be read using read() or pread(). The readdir() function should be used instead.
Comment 1 Kyle Evans freebsd_committer freebsd_triage 2020-05-12 04:15:00 UTC
Some recent discussion on this has taken place here: https://reviews.freebsd.org/D24596
Comment 2 Xin LI freebsd_committer freebsd_triage 2020-05-12 05:35:24 UTC
(In reply to Kyle Evans from comment #1)
Looks like you have a more complete patch :D
Comment 3 commit-hook freebsd_committer freebsd_triage 2020-06-04 18:10:44 UTC
A commit references this bug:

Author: kevans
Date: Thu Jun  4 18:09:57 UTC 2020
New revision: 361798
URL: https://svnweb.freebsd.org/changeset/base/361798

Log:
  vfs: add restrictions to read(2) of a directory [1/2]

  Historically, we've allowed read() of a directory and some filesystems will
  accommodate (e.g. ufs/ffs, msdosfs). From the history department staffed by
  Warner: <<EOF

  pdp-7 unix seemed to allow reading directories, but they were weird, special
  things there so I'm unsure (my pdp-7 assembler sucks).

  1st Edition's sources are lost, mostly. The kernel allows it. The
  reconstructed sources from 2nd or 3rd edition read it though.

  V6 to V7 changed the filesystem format, and should have been a warning, but
  reading directories weren't materially changed.

  4.1b BSD introduced readdir because of UFS. UFS broke all directory reading
  programs in 1983. ls, du, find, etc all had to be rewritten. readdir() and
  friends were introduced here.

  SysVr3 picked up readdir() in 1987 for the AT&T fork of Unix. SysVr4 updated
  all the directory reading programs in 1988 because different filesystem
  types were introduced.

  In the 90s, these interfaces became completely ubiquitous as PDP-11s running
  V7 faded from view and all the folks that initially started on V7 upgraded
  to SysV. Linux never supported this (though I've not done the software
  archeology to check) because it has always had a pathological diversity of
  filesystems.
  EOF

  Disallowing read(2) on a directory has the side-effect of masking
  application bugs from relying on other implementation's behavior
  (e.g. Linux) of rejecting these with EISDIR across the board, but allowing
  it has been a vector for at least one stack disclosure bug in the past[0].

  By POSIX, this is implementation-defined whether read() handles directories
  or not. Popular implementations have chosen to reject them, and this seems
  sensible: the data you're reading from a directory is not structured in some
  unified way across filesystem implementations like with readdir(2), so it is
  impossible for applications to portably rely on this.

  With this patch, we will reject most read(2) of a dirfd with EISDIR. Users
  that know what they're doing can conscientiously set
  bsd.security.allow_read_dir=1 to allow read(2) of directories, as it has
  proven useful for debugging or recovery. A future commit will further limit
  the sysctl to allow only the system root to read(2) directories, to make it
  at least relatively safe to leave on for longer periods of time.

  While we're adding logic pertaining to directory vnodes to vn_io_fault, an
  additional assertion has also been added to ensure that we're not reaching
  vn_io_fault with any write request on a directory vnode. Such request would
  be a logical error in the kernel, and must be debugged rather than allowing
  it to potentially silently error out.

  Commented out shell aliases have been placed in root's chsrc/shrc to promote
  awareness that grep may become noisy after this change, depending on your
  usage.

  A tentative MFC plan has been put together to try and make it as trivial as
  possible to identify issues and collect reports; note that this will be
  strongly re-evaluated. Tentatively, I will MFC this knob with the default as
  it is in HEAD to improve our odds of actually getting reports. The future
  priv(9) to further restrict the sysctl WILL NOT BE MERGED BACK, so the knob
  will be a faithful reversion on stable/12. We will go into the merge
  acknowledging that the sysctl default may be flipped back to restore
  historical behavior at *any* point if it's warranted.

  [0] https://www.freebsd.org/security/advisories/FreeBSD-SA-19:10.ufs.asc

  PR:		246412
  Reviewed by:	mckusick, kib, emaste, jilles, cy, phk, imp (all previous)
  Reviewed by:	rgrimes (latest version)
  MFC after:	1 month (note the MFC plan mentioned above)
  Relnotes:	absolutely, but will amend previous RELNOTES entry
  Differential Revision:	https://reviews.freebsd.org/D24596

Changes:
  head/bin/csh/dot.cshrc
  head/bin/sh/dot.shrc
  head/lib/libc/sys/read.2
  head/sys/kern/vfs_vnops.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2020-06-04 18:17:46 UTC
A commit references this bug:

Author: kevans
Date: Thu Jun  4 18:17:27 UTC 2020
New revision: 361799
URL: https://svnweb.freebsd.org/changeset/base/361799

Log:
  vfs: add restrictions to read(2) of a directory [2/2]

  This commit adds the priv(9) that waters down the sysctl to make it only
  allow read(2) of a dirfd by the system root. Jailed root is not allowed, but
  jail policy and superuser policy will abstain from allowing/denying it so
  that a MAC module can fully control the policy.

  Such a MAC module has been written, and can be found at:
  https://people.freebsd.org/~kevans/mac_read_dir-0.1.0.tar.gz

  It is expected that the MAC module won't be needed by many, as most only
  need to do such diagnostics that require this behavior as system root
  anyways. Interested parties are welcome to grab the MAC module above and
  create a port or locally integrate it, and with enough support it could see
  introduction to base. As noted in mac_read_dir.c, it is released under the
  BSD 2 clause license and allows the restrictions to be lifted for only
  jailed root or for all unprivileged users.

  PR:		246412
  Reviewed by:	mckusick, kib, emaste, jilles, cy, phk, imp (all previous)
  Reviewed by:	rgrimes (latest version)
  Differential Revision:	https://reviews.freebsd.org/D24596

Changes:
  head/lib/libc/sys/read.2
  head/sys/kern/kern_jail.c
  head/sys/kern/kern_priv.c
  head/sys/kern/vfs_vnops.c
  head/sys/sys/priv.h
Comment 5 Kyle Evans freebsd_committer freebsd_triage 2020-06-04 18:25:03 UTC
I'm going to go ahead and close this as FIXED, regardless of how MFC evaluation goes; the sysctl will get MFC'd and I'm surely not going to forget it, but the question is largely what the default will be.
Comment 6 commit-hook freebsd_committer freebsd_triage 2020-07-08 18:29:22 UTC
A commit references this bug:

Author: kevans
Date: Wed Jul  8 18:29:07 UTC 2020
New revision: 363017
URL: https://svnweb.freebsd.org/changeset/base/363017

Log:
  MFC r361798, r361800: vfs: default disallow read(2) of a directory

  This MFC is in accordance with the original MFC plan outlined in the commit
  message for r361798, appearing in full (with exception to metadata) below.

  To summarize: this MFC only merges back the sysctl with a default disallow
  policy, as in head, to ensure we hit any issues quickly but in a fashion
  that end users can easily revert. Interested parties can flip the
  security.bsd.allow_read_dir sysctl back to 1 to fully honor the previous
  behavior of allowing read(2) of any dir, filesystem permitting.

  r361798:
  vfs: add restrictions to read(2) of a directory [1/2]

  Historically, we've allowed read() of a directory and some filesystems will
  accommodate (e.g. ufs/ffs, msdosfs). From the history department staffed by
  Warner: <<EOF

  pdp-7 unix seemed to allow reading directories, but they were weird, special
  things there so I'm unsure (my pdp-7 assembler sucks).

  1st Edition's sources are lost, mostly. The kernel allows it. The
  reconstructed sources from 2nd or 3rd edition read it though.

  V6 to V7 changed the filesystem format, and should have been a warning, but
  reading directories weren't materially changed.

  4.1b BSD introduced readdir because of UFS. UFS broke all directory reading
  programs in 1983. ls, du, find, etc all had to be rewritten. readdir() and
  friends were introduced here.

  SysVr3 picked up readdir() in 1987 for the AT&T fork of Unix. SysVr4 updated
  all the directory reading programs in 1988 because different filesystem
  types were introduced.

  In the 90s, these interfaces became completely ubiquitous as PDP-11s running
  V7 faded from view and all the folks that initially started on V7 upgraded
  to SysV. Linux never supported this (though I've not done the software
  archeology to check) because it has always had a pathological diversity of
  filesystems.
  EOF

  Disallowing read(2) on a directory has the side-effect of masking
  application bugs from relying on other implementation's behavior
  (e.g. Linux) of rejecting these with EISDIR across the board, but allowing
  it has been a vector for at least one stack disclosure bug in the past[0].

  By POSIX, this is implementation-defined whether read() handles directories
  or not. Popular implementations have chosen to reject them, and this seems
  sensible: the data you're reading from a directory is not structured in some
  unified way across filesystem implementations like with readdir(2), so it is
  impossible for applications to portably rely on this.

  With this patch, we will reject most read(2) of a dirfd with EISDIR. Users
  that know what they're doing can conscientiously set
  bsd.security.allow_read_dir=1 to allow read(2) of directories, as it has
  proven useful for debugging or recovery. A future commit will further limit
  the sysctl to allow only the system root to read(2) directories, to make it
  at least relatively safe to leave on for longer periods of time.

  While we're adding logic pertaining to directory vnodes to vn_io_fault, an
  additional assertion has also been added to ensure that we're not reaching
  vn_io_fault with any write request on a directory vnode. Such request would
  be a logical error in the kernel, and must be debugged rather than allowing
  it to potentially silently error out.

  Commented out shell aliases have been placed in root's chsrc/shrc to promote
  awareness that grep may become noisy after this change, depending on your
  usage.

  A tentative MFC plan has been put together to try and make it as trivial as
  possible to identify issues and collect reports; note that this will be
  strongly re-evaluated. Tentatively, I will MFC this knob with the default as
  it is in HEAD to improve our odds of actually getting reports. The future
  priv(9) to further restrict the sysctl WILL NOT BE MERGED BACK, so the knob
  will be a faithful reversion on stable/12. We will go into the merge
  acknowledging that the sysctl default may be flipped back to restore
  historical behavior at *any* point if it's warranted.

  [0] https://www.freebsd.org/security/advisories/FreeBSD-SA-19:10.ufs.asc

  r361800:
  RELNOTES and UPDATING: Document the new policy on read(2) of dirfd

  These changes have been completely flushed as of r361799; note it.

  PR:		246412
  Relnotes:	yes 100%

Changes:
_U  stable/12/
  stable/12/UPDATING
  stable/12/bin/csh/dot.cshrc
  stable/12/lib/libc/sys/read.2
  stable/12/sys/kern/vfs_vnops.c