Bug 238725 - Severe NFS exports(5) -maproot regression for :group definition
Summary: Severe NFS exports(5) -maproot regression for :group definition
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.3-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2019-06-20 18:45 UTC by Harald Schmalzbauer
Modified: 2019-06-27 14:28 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Harald Schmalzbauer 2019-06-20 18:45:48 UTC
Hello,

I've been using semi-sophisticated exports(5), last adjusted with FreeBSD-9 and reused sucessfully on FreeBSD-10+11.
Recently I upgraded one machine From FreeBSD-11 to FreeBSD-12-stable and now the ":group" definition of -maproot= in exports(5) has no effect anymore.

Here are the relevant infos for reproduction (NFSv4):
/zfs/netshares/deployment  -ro -maproot=65534:65533 -network 192.0.2.0/24
getent passwd 65534
nobody:*:65534:65534:Unprivileged user:/nonexistent:/usr/sbin/nologin
getent group 65534
nobody:*:65534
This is verified to be identical on the 11 and 12 servers!


On the NFS server, cd into /zfs/netshares/deploymemt and:
mkdir test && touch test/testfile
setfacl -b test && chown root:nogroup test && chmod 750 test                                                             

On the client, issue as root: ls /$nfsservermounpoint/zfs/netshares/deployment/test
Clients mounting from FreeBSD-12 tell "ls: .../deployment/test: Permission denied"
Clients mounting from FreeBSD-11 list the "testfile".

The -maproot=user part works, but not the :group anymore.
This is also falsified using nfsv3 (with ESXi client).

Hope somebody has an idea which change could be the culprit.  Needless to say that this was really unexpected and badly breaks a lot of things.

Thanks,
-harry
Comment 1 Harald Schmalzbauer 2019-06-21 14:06:35 UTC
Found some time to falsify r346976 (MFCd in https://svnweb.freebsd.org/base?view=revision&revision=347340).
Reverting that single commit restores full -maproot support.

Will see if I can come up with a patch, but I guess more skilled hackers need much less time...

Thanks,

-harry
Comment 2 Harald Schmalzbauer 2019-06-21 18:33:06 UTC
Sorry for the bad reproduction info.  The choosen values are nonsense and inconsistent in the example...
But I guess that doesn't matter much.

Here's the breaking part:

Index: usr.sbin/mountd/mountd.c
===================================================================
--- usr.sbin/mountd/mountd.c    (Revision 349275)
+++ usr.sbin/mountd/mountd.c    (Arbeitskopie)
@@ -2958,8 +2958,8 @@
        /*
         * Get the user's password table entry.
         */
-       names = namelist;
-       name = strsep_quote(&names, ":");
+       names = strsep_quote(&namelist, " \t\n");
+       name = strsep(&names, ":");
        /* Bug?  name could be NULL here */
        if (isdigit(*name) || *name == '-')
                pw = getpwuid(atoi(name));

This reverse-diff restores working state.
I have no idea what the intention was.
At least dlim seems to be unintentionally changed?

Thanks,

-harry

P.S.: Like mentioned, I consider this as severe surprise for useres updating to 11.3 and wondering why NFS doesn't work as before...
Comment 3 commit-hook freebsd_committer freebsd_triage 2019-06-25 17:01:28 UTC
A commit references this bug:

Author: mav
Date: Tue Jun 25 17:00:54 UTC 2019
New revision: 349376
URL: https://svnweb.freebsd.org/changeset/base/349376

Log:
  Fix strsep_quote() on strings without quotes.

  For strings without quotes and escapes dstptr and srcptr are equal, so
  zeroing *dstptr before checking *srcptr is not a good idea.  In practice
  it means that in -maproot=65534:65533 everything after the colon is lost.

  The problem was there since r293305, but before r346976 it was covered by
  improper strsep_quote() usage.

  PR:		238725
  MFC after:	3 days
  Sponsored by:	iXsystems, Inc.

Changes:
  head/usr.sbin/mountd/mountd.c
Comment 4 Alexander Motin freebsd_committer freebsd_triage 2019-06-25 17:02:05 UTC
I've reproduced the problem. r349376 in head should fix it properly.  Your revert just hides the problem as it was hidden before r346976.
Comment 5 Harald Schmalzbauer 2019-06-25 19:12:47 UTC
Thanks, I just needed a quick fix for my production system.  Glad that you really fixed the original bug now.

-harry
Comment 6 Glen Barber freebsd_committer freebsd_triage 2019-06-26 15:18:02 UTC
(In reply to Alexander Motin from comment #4)
> I've reproduced the problem. r349376 in head should fix it properly.  Your
> revert just hides the problem as it was hidden before r346976.

Alexander, thank you for fixing this so quickly.

Can you please send a request for approval against releng/11.3 before 22:00 UTC June 27 (tomorrow) for inclusion in 11.3-RELEASE?

Thank you in advance.
Comment 7 commit-hook freebsd_committer freebsd_triage 2019-06-27 14:11:19 UTC
A commit references this bug:

Author: mav
Date: Thu Jun 27 14:10:59 UTC 2019
New revision: 349456
URL: https://svnweb.freebsd.org/changeset/base/349456

Log:
  MFC r349376: Fix strsep_quote() on strings without quotes.

  For strings without quotes and escapes dstptr and srcptr are equal, so
  zeroing *dstptr before checking *srcptr is not a good idea.  In practice
  it means that in -maproot=65534:65533 everything after the colon is lost.

  The problem was there since r293305, but before r346976 it was covered by
  improper strsep_quote() usage.

  PR:	238725

Changes:
_U  stable/12/
  stable/12/usr.sbin/mountd/mountd.c
Comment 8 commit-hook freebsd_committer freebsd_triage 2019-06-27 14:13:24 UTC
A commit references this bug:

Author: mav
Date: Thu Jun 27 14:12:21 UTC 2019
New revision: 349457
URL: https://svnweb.freebsd.org/changeset/base/349457

Log:
  MFC r349376: Fix strsep_quote() on strings without quotes.

  For strings without quotes and escapes dstptr and srcptr are equal, so
  zeroing *dstptr before checking *srcptr is not a good idea.  In practice
  it means that in -maproot=65534:65533 everything after the colon is lost.

  The problem was there since r293305, but before r346976 it was covered by
  improper strsep_quote() usage.

  PR:	238725

Changes:
_U  stable/11/
  stable/11/usr.sbin/mountd/mountd.c
Comment 9 commit-hook freebsd_committer freebsd_triage 2019-06-27 14:27:37 UTC
A commit references this bug:

Author: mav
Date: Thu Jun 27 14:26:57 UTC 2019
New revision: 349458
URL: https://svnweb.freebsd.org/changeset/base/349458

Log:
  MFC r349376: Fix strsep_quote() on strings without quotes.

  For strings without quotes and escapes dstptr and srcptr are equal, so
  zeroing *dstptr before checking *srcptr is not a good idea.  In practice
  it means that in -maproot=65534:65533 everything after the colon is lost.

  The problem was there since r293305, but before r346976 it was covered by
  improper strsep_quote() usage.

  PR:	238725

  Approved by:	re (gjb)

Changes:
_U  releng/11.3/
  releng/11.3/usr.sbin/mountd/mountd.c