Bug 281003 - NFS client can open the server on ZFS with sharenfs dataset
Summary: NFS client can open the server on ZFS with sharenfs dataset
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Many People
Assignee: Rick Macklem
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-23 03:12 UTC by Toshio Kagiwada
Modified: 2024-09-27 22:23 UTC (History)
1 user (show)

See Also:


Attachments
Check for a '=arg' after exports options (3.00 KB, patch)
2024-09-06 00:49 UTC, Rick Macklem
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Toshio Kagiwada 2024-08-23 03:12:23 UTC
I wonder if it's ordinary thing or not for you.
But I felt needs to report.

I found the issue following:

I set sharenfs dataset like this:
#+begin_src
zfs set sharenfs="192.168.1.55,ro=192.168.1.56" zroot/usr/src
#+end src

Of course I can open this on 192.168.1.55 with NFS.
And then I set like this:

#+begin_src
zfs set sharenfs="ro=192.168.1.56" zroot/usr/src
#+end_src

I can open zroot/usr/src on 192.168.1.55

I thought this can be a security issue for many people.
Thanks.
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2024-08-23 22:54:57 UTC
This appears to be a bug in mountd.
-ro=192.168.1.56 is bogus, but mountd
does not spot it as an error.

Btw, both of your sharenfs settings are
bogus, since -ro is not supposed to have
"=192.168.1.56" after it.

All the sharenfs property does is autogenerate
an exports line. It can be found in /etc/zfs/exports
and should obey the same syntax as "man 5 exports"
describes.
(Althougn the sharenfs parser in OpenZFS should be
improved, the bogus case should not get past mountd.)

I'll take a look at mountd.c and try to come up with a patch.

I am not sure what you were trying to do by specifying one
IP address followed by ro=anther-ip, but you cannot mix read/write
and read-only exports on the same line.
--> Until you have the very recent patch in PR#147881,
    you cannot generate multiple lines via sharenfs.
   --> To do multiple lines, you need to create them manually in
       /etc/exports and not use the sharenfs property.


Thanks for reporting it, rick
ps: And never put a "=XXX" after "ro" for an exports option.
Comment 2 Toshio Kagiwada 2024-08-24 05:35:36 UTC
I git-pulled the 14-STABLE code AM 6:26 JST.

And I make-install-ed for the all machines that FreeBSD are installed.
I checked the same operations that I had yesterday:

mount 192.168.1.55:/usr/src /usr/src
on 192.168.1.52

zfs set sharenfs="192.168.1.51,192.168.1.52,192.168.1.56" zroot/usr/src

-> mounted

zfs set sharenfs="192.168.1.51,ro=192.168.1.56" zroot/usr/src

-> couldn't mount

zfs set sharenfs="192.168.1.52,ro=192.168.1.56" zroot/usr/src

-> mounted

zfs set sharenfs="ro=192.168.1.56" zroot/usr/src

-> error occured
[tcp] 192.168.1.55:/usr/src: RPCPROG_NFS: RPC: Remote system error - Connection refused

This error wasn't seen yesterday's operation.
So I thought probably I had something like a mistake.
Thanks.
Comment 3 Rick Macklem freebsd_committer freebsd_triage 2024-08-24 14:23:59 UTC
Well, the "-ro" exports option is not supposed
to have "=XXX" after it. There are only certain
exports options that do have "=XXX". -network, -mask
and -sec are the ones that come to mind.

mountd should spot this error, generate a syslog
error message for it and fail the exports line.

I will take a look and come up with a patch.
(mountd.c is actually code that was first written
 in 1987, believe it or not.)

As I noted, all the ZFS sharenfs property does is
generate an exports file in /etc/zfs/exports that
is concatenated with /etc/exports by mountd.
Comment 4 Rick Macklem freebsd_committer freebsd_triage 2024-08-25 02:12:17 UTC
(In reply to Toshio Kagiwada from comment #2)
Just to make sure it is clear to you (and others
reading this PR):

zfs set sharenfs="192.168.1.51,192.168.1.52,192.168.1.56" zroot/usr/src

This is valid syntax and exports 192.168.1.51, 192.168.1.52 and
192.168.1.56 read-write.

zfs set sharenfs="192.168.1.51,ro=192.168.1.56" zroot/usr/src

This in NOT valid syntax (there should never be a =192.168.1.56
after "ro") and mountd should fail this entry with an error.
(The bug is that mountd does not fail the entry. I suspect it just
ignores the =192.168.1.56, which is incorrect.)

zfs set sharenfs="192.168.1.52,ro=192.168.1.56" zroot/usr/src

Again, this is NOT valid syntax and mountd should fail it with an error.

If you wish to export 192.168.1.56 read-only, the correct syntax is:

zfs set sharenfs="-ro,192.168.1.56" zroot/usr/src
Comment 5 Toshio Kagiwada 2024-08-26 20:21:05 UTC
I found I had a mistake about NFS and ZFS settings.

And I'd like to cancel this report in Bugzilla.
Thank you for advice, Rick.

I'll change the status to "closed" and "works as intended".
Please fix this if I made a wrong choice.
Comment 6 Rick Macklem freebsd_committer freebsd_triage 2024-08-26 22:30:54 UTC
(In reply to Toshio Kagiwada from comment #5)
Nope, there is a real bug, as I have noted.
(mountd does not fail the bogus exports line.)

I will come up with a patch. Please leave the
bug report open until the system is patched.
(I will close it then.)

rick
Comment 7 Rick Macklem freebsd_committer freebsd_triage 2024-09-06 00:49:34 UTC
Created attachment 253366 [details]
Check for a '=arg' after exports options

This patch adds a check for an '=arg' after exports
options that do not take a 'arg' and generates an
error for these bogus entries in the exports file(s).

This will avoid at least this case, where an improperly
formed exports file (/etc/zfs/exports, generated by the
ZFS sharenfs property) would be allowed by mountd.

I will be committing this patch to main and MFC'ng it
soon.

In general, it is a good idea to look at the /etc/zfs/exports
file generated by ZFS's sharenfs property, to ensure it is
of the format you had intended it to be. (With reference
to "man 5 exports".)
Comment 8 commit-hook freebsd_committer freebsd_triage 2024-09-06 23:42:48 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3df987c99d1194a0e43a84853e934aa0c0ab09db

commit 3df987c99d1194a0e43a84853e934aa0c0ab09db
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2024-09-06 23:41:12 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2024-09-06 23:41:12 +0000

    mountd: Add check for "=" after exports(5) options

    Some exports(5) options take a "=arg" component that provides an
    argument value for the option.  Others do not.
    Without this patch, if "=arg" was provided for an option that did
    not take an argument value, the "=arg" was simply ignored.
    This could result in confusion w.r.t. what was being exported,
    as noted by the Problem Report.

    This patch adds a check for "=arg" for the options that do not
    take an argument value and fails the exports line if one is found.

    PR:     281003
    MFC after:       2 weeks

 usr.sbin/mountd/mountd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)
Comment 9 commit-hook freebsd_committer freebsd_triage 2024-09-27 22:15:58 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c09ca8f43de12fecf701920675b793cbafba58c5

commit c09ca8f43de12fecf701920675b793cbafba58c5
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2024-09-06 23:41:12 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2024-09-27 22:13:49 +0000

    mountd: Add check for "=" after exports(5) options

    Some exports(5) options take a "=arg" component that provides an
    argument value for the option.  Others do not.
    Without this patch, if "=arg" was provided for an option that did
    not take an argument value, the "=arg" was simply ignored.
    This could result in confusion w.r.t. what was being exported,
    as noted by the Problem Report.

    This patch adds a check for "=arg" for the options that do not
    take an argument value and fails the exports line if one is found.

    PR:     281003
    (cherry picked from commit 3df987c99d1194a0e43a84853e934aa0c0ab09db)

 usr.sbin/mountd/mountd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-09-27 22:20:00 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=132f5d03d358d89d4030de9173cd6ca5b4b48d68

commit 132f5d03d358d89d4030de9173cd6ca5b4b48d68
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2024-09-06 23:41:12 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2024-09-27 22:18:46 +0000

    mountd: Add check for "=" after exports(5) options

    Some exports(5) options take a "=arg" component that provides an
    argument value for the option.  Others do not.
    Without this patch, if "=arg" was provided for an option that did
    not take an argument value, the "=arg" was simply ignored.
    This could result in confusion w.r.t. what was being exported,
    as noted by the Problem Report.

    This patch adds a check for "=arg" for the options that do not
    take an argument value and fails the exports line if one is found.

    PR:     281003
    (cherry picked from commit 3df987c99d1194a0e43a84853e934aa0c0ab09db)

 usr.sbin/mountd/mountd.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)
Comment 11 Rick Macklem freebsd_committer freebsd_triage 2024-09-27 22:23:47 UTC
The patch has now been commmitted and MFC'd.