Bug 246983

Summary: sbput(3) writes out the values of pointers verbatim
Product: Base System Reporter: goatshit54108
Component: kernAssignee: freebsd-fs (Nobody) <fs>
Status: Closed FIXED    
Severity: Affects Only Me CC: brooks, markj, mckusick, pi
Priority: --- Keywords: patch
Version: Unspecified   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
in ffs_sbput(), clear the pointers for the duration of writing none

Description goatshit54108 2020-06-04 15:31:49 UTC
`struct fs` contains some pointer fields. The binary representation of a `struct fs` is written out verbatim to the disk: see ffs_sbput() in sys/ufs/ffs/ffs_subr.c. This inadvertently causes garbage to be stored, in particular by newfs(1). (It is garbage because those pointers pointed to the then-current stack or heap.) Although a mere pointer doesn't reveal anything useful (like a part of a private key) to an attacker, garbage output deteriorates reproducibility.

In an attempt to work around this design, users of sbput(3) ought to zero out those pointer fields, but this isn't totally possible, because the function makes use of the `fs_csp` field. So the solution should be to save the pointers before writing the whole struct out, and to restore them afterwards.

It would have been ideal if `struct fs` didn't contain any pointers at all.
Comment 1 Brooks Davis freebsd_committer freebsd_triage 2020-06-04 18:36:48 UTC
In CheriBSD's cheri-purecap-kernel branch (https://github.com/CTSRD-CHERI/cheribsd/tree/cheri-purecap-kernel) we have a modification that reduces this to a single pointer to a structure holding the other pointers.  In our case this is because the in-kernel use includes too many pointers to fit when you blow them up to 128-bit.  This doesn't address the leakage issues, but might be a place to start from.
Comment 2 goatshit54108 2020-06-05 00:45:02 UTC
Created attachment 215239 [details]
in ffs_sbput(), clear the pointers for the duration of writing
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2020-06-08 15:36:58 UTC
(In reply to goatshit54108 from comment #2)
This won't apply to HEAD, where we additionally compute a checksum to store with the superblock.

I suspect a better solution might be to replace these pointer fields with spares, always, zero, and move the pointers into struct ufsmount.
Comment 4 Kirk McKusick freebsd_committer freebsd_triage 2020-06-10 05:33:00 UTC
(In reply to Mark Johnston from comment #3)
Although the ufsmount structure has a pointer to the fs structure, the inverse is not available. So, if we moved some of the fs_ pointers to the ufsmount structure, simply having the fs pointer would not be sufficient to get to the ufsmount fields (i.e., we could not have a macro to hide the move, we would have to go to every place they were used and figure out a way to find the ufsmount structure).
Comment 5 Kirk McKusick freebsd_committer freebsd_triage 2020-06-10 07:08:57 UTC
(In reply to Brooks Davis from comment #1)
I like the idea of reducing the number of pointers in the superblock to just one. I will look at adopting your CHERI changes.
Comment 6 Kirk McKusick freebsd_committer freebsd_triage 2020-06-10 07:10:43 UTC
(In reply to goatshit54108 from comment #2)
I concur with your proposed changes. I am first going to bring in the CHERI changes to reduce the pointers in the superblock to one. I will then NULL out that pointer when writing the superblock along the lines that you propose.
Comment 7 commit-hook freebsd_committer freebsd_triage 2020-06-19 01:03:45 UTC
A commit references this bug:

Author: mckusick
Date: Fri Jun 19 01:02:55 UTC 2020
New revision: 362358
URL: https://svnweb.freebsd.org/changeset/base/362358

Log:
  Move the pointers stored in the superblock into a separate
  fs_summary_info structure. This change was originally done
  by the CheriBSD project as they need larger pointers that
  do not fit in the existing superblock.

  This cleanup of the superblock eases the task of the commit
  that immediately follows this one.

  Suggested by: brooks
  Reviewed by:  kib
  PR:           246983
  Sponsored by: Netflix

Changes:
  head/sbin/newfs/mkfs.c
  head/stand/libsa/ufs.c
  head/sys/geom/journal/g_journal_ufs.c
  head/sys/geom/label/g_label_ufs.c
  head/sys/ufs/ffs/ffs_snapshot.c
  head/sys/ufs/ffs/ffs_subr.c
  head/sys/ufs/ffs/ffs_vfsops.c
  head/sys/ufs/ffs/fs.h
  head/usr.sbin/fstyp/ufs.c
  head/usr.sbin/quot/quot.c
Comment 8 commit-hook freebsd_committer freebsd_triage 2020-06-19 01:04:47 UTC
A commit references this bug:

Author: mckusick
Date: Fri Jun 19 01:04:26 UTC 2020
New revision: 362359
URL: https://svnweb.freebsd.org/changeset/base/362359

Log:
  The binary representation of the superblock (the fs structure) is written
  out verbatim to the disk: see ffs_sbput() in sys/ufs/ffs/ffs_subr.c.
  It contains a pointer to the fs_summary_info structure. This pointer
  value inadvertently causes garbage to be stored. It is garbage because
  the pointer to the fs_summary_info structure is the address the then
  current stack or heap. Although a mere pointer does not reveal anything
  useful (like a part of a private key) to an attacker, garbage output
  deteriorates reproducibility.

  This commit zeros out the pointer to the fs_summary_info structure
  before writing the out the superblock.

  Reviewed by:  kib
  Tested by:    Peter Holm
  PR:           246983
  Sponsored by: Netflix

Changes:
  head/sys/ufs/ffs/ffs_subr.c
  head/sys/ufs/ffs/ffs_vfsops.c
Comment 9 Kirk McKusick freebsd_committer freebsd_triage 2020-06-19 01:07:20 UTC
Fixed by two commits noted below.
Comment 10 Kirk McKusick freebsd_committer freebsd_triage 2020-06-23 21:19:55 UTC
Several other issues have been raised by the submitter.
Comment 11 Kirk McKusick freebsd_committer freebsd_triage 2020-06-23 21:59:10 UTC
The following four commits were made (and accidentally cited bug 247425):

-r362558 Clean up comments in libufs struct uufsd.
-r362559 Make libufs track and clean up fs_si structure.
-r362560 Correctly describe libufs library sbget() and sbput() return values.
-r362561 Avoid writing superblock summary info in g_journal.

With these changes, this bug can now be closed.
Comment 12 commit-hook freebsd_committer freebsd_triage 2020-08-15 21:41:24 UTC
A commit references this bug:

Author: mckusick
Date: Sat Aug 15 21:40:37 UTC 2020
New revision: 364262
URL: https://svnweb.freebsd.org/changeset/base/364262

Log:
  Use the sbput() function to write alternate superblocks so that
  they get a checkhash.

  PR:           246983
  Sponsored by: Netflix

Changes:
  head/sbin/fsck_ffs/main.c