kNFSD Signed Filehandles
Subsystems (6)
cover kNFSD Signed Filehandles
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the export_operation "encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Filehandles are easy to guess because they are well-formed. The open_by_handle_at(2) man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:
# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c
^--------- filehandle ----------^
^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using open_by_handle_at(2). For a local system, open_by_handle_at(2) requires CAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
In contrast to a local user using open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
I had a previous attempt to solve this problem by encrypting filehandles, which turned out to be a problematic, poor solution. The discussion on that previous attempt is here:
https://lore.kernel.org/linux-nfs/[email protected]/T/#t
There are some changes from that version:
- sign filehandles instead of encrypt them (Eric Biggers)
- fix the NFSEXP_ macros, specifically NFSEXP_ALLFLAGS (NeilBrown)
- rebase onto cel/nfsd-next (Chuck Lever)
- condensed/clarified problem explantion (thanks Chuck Lever)
- add nfsctl file "fh_key" for rpc.nfsd to also set the key
I plan on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "sign_fh" is requested.
Thanks for any comments and critique.
Benjamin Coddington (4):
nfsd: Convert export flags to use BIT() macro
nfsd: Add a key for signing filehandles
NFSD/export: Add sign_fh export option
NFSD: Sign filehandles
Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfs3xdr.c | 20 +++--- fs/nfsd/nfs4xdr.c | 12 ++-- fs/nfsd/nfsctl.c | 87 ++++++++++++++++++++++++++- fs/nfsd/nfsfh.c | 72 +++++++++++++++++++++- fs/nfsd/nfsfh.h | 22 +++++++ fs/nfsd/trace.h | 19 ++++++ include/linux/sunrpc/svc.h | 1 + include/uapi/linux/nfsd/export.h | 38 ++++++------ include/uapi/linux/nfsd_netlink.h | 2 + 14 files changed, 272 insertions(+), 36 deletions(-)
base-commit: bfd453acb5637b5df881cef4b21803344aa9e7ac
Signature · 1 line
2.50.1
PATCH 1/4 nfsd: Convert export flags to use BIT() macro
Simplify these defines for consistency, readability, and clarity.
--- fs/nfsd/nfsctl.c | 2 +- include/uapi/linux/nfsd/export.h | 38 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)
fs/nfsd/nfsctl.c +1 -1
@@ -169,7 +169,7 @@ static const struct file_operations exports_nfsd_operations = {static int export_features_show (struct seq_file *m,void *v) { -seq_printf (m,"0x%x 0x%x\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS ); +seq_printf (m,"0x%lx 0x%lx\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS );return 0 ; }
include/uapi/linux/nfsd/export.h +19 -19
@@ -26,22 +26,22 @@* Please update the expflags[] array in fs/nfsd/export.c when adding * a new flag. */ -#define NFSEXP_READONLY 0x0001 -#define NFSEXP_INSECURE_PORT 0x0002 -#define NFSEXP_ROOTSQUASH 0x0004 -#define NFSEXP_ALLSQUASH 0x0008 -#define NFSEXP_ASYNC 0x0010 -#define NFSEXP_GATHERED_WRITES 0x0020 -#define NFSEXP_NOREADDIRPLUS 0x0040 -#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ -#define NFSEXP_NOHIDE 0x0200 -#define NFSEXP_NOSUBTREECHECK 0x0400 -#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */ -#define NFSEXP_MSNFS 0x1000/* do silly things that MS clients expect; no longer supported */ -#define NFSEXP_FSID 0x2000 -#define NFSEXP_CROSSMOUNT 0x4000 -#define NFSEXP_NOACL 0x8000/* reserved for possible ACL related use */ +#define NFSEXP_READONLY BIT(0) +#define NFSEXP_INSECURE_PORT BIT(1) +#define NFSEXP_ROOTSQUASH BIT(2) +#define NFSEXP_ALLSQUASH BIT(3) +#define NFSEXP_ASYNC BIT(4) +#define NFSEXP_GATHERED_WRITES BIT(5) +#define NFSEXP_NOREADDIRPLUS BIT(6) +#define NFSEXP_SECURITY_LABEL BIT(7) +/* BIT(8) currently unused */ +#define NFSEXP_NOHIDE BIT(9) +#define NFSEXP_NOSUBTREECHECK BIT(10) +#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */ +#define NFSEXP_MSNFS BIT(12)/* do silly things that MS clients expect; no longer supported */ +#define NFSEXP_FSID BIT(13) +#define NFSEXP_CROSSMOUNT BIT(14) +#define NFSEXP_NOACL BIT(15)/* reserved for possible ACL related use */ /* * The NFSEXP_V4ROOT flag causes the kernel to give access only to NFSv4 * clients, and only to the single directory that is the root of the
@@ -51,11 +51,11 @@* pseudofilesystem, which provides access only to paths leading to each * exported filesystem. */ -#define NFSEXP_V4ROOT 0x10000 -#define NFSEXP_PNFS 0x20000 +#define NFSEXP_V4ROOT BIT(16) +#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v1 1/4] nfsd: Convert export flags to use BIT() macro
Quoted 91 lines · Benjamin Coddington
Simplify these defines for consistency, readability, and clarity.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsctl.c | 2 +- include/uapi/linux/nfsd/export.h | 38 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)fs/nfsd/nfsctl.c +1
-1@@ -169,7 +169,7 @@ static const struct file_operations exports_nfsd_operations = {static int export_features_show (struct seq_file *m,void *v) { -seq_printf (m,"0x%x 0x%x\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS ); +seq_printf (m,"0x%lx 0x%lx\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS );return 0 ; }include/uapi/linux/nfsd/export.h +0
-12@@ -26,22 +26,22 @@* Please update the expflags[] array in fs/nfsd/export.c when adding * a new flag. */ -#define NFSEXP_READONLY 0x0001 -#define NFSEXP_INSECURE_PORT 0x0002 -#define NFSEXP_ROOTSQUASH 0x0004 -#define NFSEXP_ALLSQUASH 0x0008 -#define NFSEXP_ASYNC 0x0010 -#define NFSEXP_GATHERED_WRITES 0x0020 -#define NFSEXP_NOREADDIRPLUS 0x0040 -#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ -#define NFSEXP_NOHIDE 0x0200 -#define NFSEXP_NOSUBTREECHECK 0x0400 -#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
-#defineNFSEXP_MSNFS0x1000/* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +0
-3-#define NFSEXP_FSID 0x2000 -#define NFSEXP_CROSSMOUNT 0x4000 -#define NFSEXP_NOACL 0x8000/* reserved for possible ACL related use */
include/uapi/linux/nfsd/export.h +12
-0+#define NFSEXP_READONLY BIT(0) +#define NFSEXP_INSECURE_PORT BIT(1) +#define NFSEXP_ROOTSQUASH BIT(2) +#define NFSEXP_ALLSQUASH BIT(3) +#define NFSEXP_ASYNC BIT(4) +#define NFSEXP_GATHERED_WRITES BIT(5) +#define NFSEXP_NOREADDIRPLUS BIT(6) +#define NFSEXP_SECURITY_LABEL BIT(7) +/* BIT(8) currently unused */ +#define NFSEXP_NOHIDE BIT(9) +#define NFSEXP_NOSUBTREECHECK BIT(10) +#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
+#defineNFSEXP_MSNFSBIT(12) /* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +3
-0+#define NFSEXP_FSID BIT(13) +#define NFSEXP_CROSSMOUNT BIT(14) +#define NFSEXP_NOACL BIT(15)/* reserved for possible ACL related use */
/*
* TheNFSEXP_V4ROOTflag causes the kernel to give access only to
NFSv4
* clients, and only to the single directory that is the root of theinclude/uapi/linux/nfsd/export.h +0
-0@@ -51,11 +51,11 @@* pseudofilesystem, which provides access only to paths leading to each
* exported filesystem.
*/include/uapi/linux/nfsd/export.h +3
-3-#define NFSEXP_V4ROOT 0x10000 -#define NFSEXP_PNFS 0x20000 +#define NFSEXP_V4ROOT BIT(16) +#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \Signature · 1 line
2.50.1
This might constitute a breaking user space API change. BIT() is a kernel convention. What defines BIT() for user space consumers of this header?
Signature · 1 line
Chuck Lever
Re: [PATCH v1 1/4] nfsd: Convert export flags to use BIT() macro
Quoted 88 lines · Chuck Lever
Quoted 87 lines · Benjamin Coddington
Simplify these defines for consistency, readability, and clarity.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsctl.c | 2 +- include/uapi/linux/nfsd/export.h | 38 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)fs/nfsd/nfsctl.c +1
-1@@ -169,7 +169,7 @@ static const struct file_operations exports_nfsd_operations = {static int export_features_show (struct seq_file *m,void *v) { -seq_printf (m,"0x%x 0x%x\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS ); +seq_printf (m,"0x%lx 0x%lx\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS );return 0 ; }include/uapi/linux/nfsd/export.h +0
-12@@ -26,22 +26,22 @@* Please update the expflags[] array in fs/nfsd/export.c when adding * a new flag. */ -#define NFSEXP_READONLY 0x0001 -#define NFSEXP_INSECURE_PORT 0x0002 -#define NFSEXP_ROOTSQUASH 0x0004 -#define NFSEXP_ALLSQUASH 0x0008 -#define NFSEXP_ASYNC 0x0010 -#define NFSEXP_GATHERED_WRITES 0x0020 -#define NFSEXP_NOREADDIRPLUS 0x0040 -#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ -#define NFSEXP_NOHIDE 0x0200 -#define NFSEXP_NOSUBTREECHECK 0x0400 -#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
-#defineNFSEXP_MSNFS0x1000/* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +0
-3-#define NFSEXP_FSID 0x2000 -#define NFSEXP_CROSSMOUNT 0x4000 -#define NFSEXP_NOACL 0x8000/* reserved for possible ACL related use */
include/uapi/linux/nfsd/export.h +12
-0+#define NFSEXP_READONLY BIT(0) +#define NFSEXP_INSECURE_PORT BIT(1) +#define NFSEXP_ROOTSQUASH BIT(2) +#define NFSEXP_ALLSQUASH BIT(3) +#define NFSEXP_ASYNC BIT(4) +#define NFSEXP_GATHERED_WRITES BIT(5) +#define NFSEXP_NOREADDIRPLUS BIT(6) +#define NFSEXP_SECURITY_LABEL BIT(7) +/* BIT(8) currently unused */ +#define NFSEXP_NOHIDE BIT(9) +#define NFSEXP_NOSUBTREECHECK BIT(10) +#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
+#defineNFSEXP_MSNFSBIT(12) /* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +3
-0+#define NFSEXP_FSID BIT(13) +#define NFSEXP_CROSSMOUNT BIT(14) +#define NFSEXP_NOACL BIT(15)/* reserved for possible ACL related use */
/*
* TheNFSEXP_V4ROOTflag causes the kernel to give access only to
NFSv4
* clients, and only to the single directory that is the root of theinclude/uapi/linux/nfsd/export.h +0
-0@@ -51,11 +51,11 @@* pseudofilesystem, which provides access only to paths leading to each
* exported filesystem.
*/include/uapi/linux/nfsd/export.h +3
-3-#define NFSEXP_V4ROOT 0x10000 -#define NFSEXP_PNFS 0x20000 +#define NFSEXP_V4ROOT BIT(16) +#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1
Quoted 8 lines
Quoted 4 lines
/* The flags that may vary depending on security flavor: */
#defineNFSEXP_SECINFO_FLAGS(NFSEXP_READONLY|NFSEXP_ROOTSQUASH\Signature · 1 line
2.50.1This might constitute a breaking user space API change.
BIT()is a kernel convention. What definesBIT()for user space consumers of this header?
Doh - good catch. I can drop this, or maybe add:
#ifndef BIT
#define BIT(n) (1UL << (n))
#endif
Ben
Chuck Lever
maintainerRe: [PATCH v1 1/4] nfsd: Convert export flags to use BIT() macro
Quoted 99 lines · Benjamin Coddington
Quoted 88 lines · Chuck Lever
Quoted 87 lines · Benjamin Coddington
Simplify these defines for consistency, readability, and clarity.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsctl.c | 2 +- include/uapi/linux/nfsd/export.h | 38 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)fs/nfsd/nfsctl.c +1
-1@@ -169,7 +169,7 @@ static const struct file_operations exports_nfsd_operations = {static int export_features_show (struct seq_file *m,void *v) { -seq_printf (m,"0x%x 0x%x\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS ); +seq_printf (m,"0x%lx 0x%lx\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS );return 0 ; }include/uapi/linux/nfsd/export.h +0
-12@@ -26,22 +26,22 @@* Please update the expflags[] array in fs/nfsd/export.c when adding * a new flag. */ -#define NFSEXP_READONLY 0x0001 -#define NFSEXP_INSECURE_PORT 0x0002 -#define NFSEXP_ROOTSQUASH 0x0004 -#define NFSEXP_ALLSQUASH 0x0008 -#define NFSEXP_ASYNC 0x0010 -#define NFSEXP_GATHERED_WRITES 0x0020 -#define NFSEXP_NOREADDIRPLUS 0x0040 -#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ -#define NFSEXP_NOHIDE 0x0200 -#define NFSEXP_NOSUBTREECHECK 0x0400 -#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
-#defineNFSEXP_MSNFS0x1000/* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +0
-3-#define NFSEXP_FSID 0x2000 -#define NFSEXP_CROSSMOUNT 0x4000 -#define NFSEXP_NOACL 0x8000/* reserved for possible ACL related use */
include/uapi/linux/nfsd/export.h +12
-0+#define NFSEXP_READONLY BIT(0) +#define NFSEXP_INSECURE_PORT BIT(1) +#define NFSEXP_ROOTSQUASH BIT(2) +#define NFSEXP_ALLSQUASH BIT(3) +#define NFSEXP_ASYNC BIT(4) +#define NFSEXP_GATHERED_WRITES BIT(5) +#define NFSEXP_NOREADDIRPLUS BIT(6) +#define NFSEXP_SECURITY_LABEL BIT(7) +/* BIT(8) currently unused */ +#define NFSEXP_NOHIDE BIT(9) +#define NFSEXP_NOSUBTREECHECK BIT(10) +#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
+#defineNFSEXP_MSNFSBIT(12) /* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +3
-0+#define NFSEXP_FSID BIT(13) +#define NFSEXP_CROSSMOUNT BIT(14) +#define NFSEXP_NOACL BIT(15)/* reserved for possible ACL related use */
/*
* TheNFSEXP_V4ROOTflag causes the kernel to give access only to
NFSv4
* clients, and only to the single directory that is the root of theinclude/uapi/linux/nfsd/export.h +0
-0@@ -51,11 +51,11 @@* pseudofilesystem, which provides access only to paths leading to each
* exported filesystem.
*/include/uapi/linux/nfsd/export.h +3
-3-#define NFSEXP_V4ROOT 0x10000 -#define NFSEXP_PNFS 0x20000 +#define NFSEXP_V4ROOT BIT(16) +#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1Quoted 8 lines
Quoted 4 lines
/* The flags that may vary depending on security flavor: */
#defineNFSEXP_SECINFO_FLAGS(NFSEXP_READONLY|NFSEXP_ROOTSQUASH\Signature · 1 line
2.50.1This might constitute a breaking user space API change.
BIT()is a kernel convention. What definesBIT()for user space consumers of this header?
Quoted 5 lines
Doh - good catch. I can drop this, or maybe add:
#ifndef BIT
#define BIT(n) (1UL << (n))
#endif
I'm happy leaving these as hex constants.
Signature · 1 line
Chuck Lever
Re: [PATCH v1 1/4] nfsd: Convert export flags to use BIT() macro
Quoted 100 lines · Chuck Lever
Quoted 99 lines · Benjamin Coddington
Quoted 96 lines · Chuck Lever
Quoted 91 lines · Benjamin Coddington
Simplify these defines for consistency, readability, and clarity.
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfsctl.c | 2 +- include/uapi/linux/nfsd/export.h | 38 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 20 deletions(-)fs/nfsd/nfsctl.c +1
-1@@ -169,7 +169,7 @@ static const struct file_operations exports_nfsd_operations = {static int export_features_show (struct seq_file *m,void *v) { -seq_printf (m,"0x%x 0x%x\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS ); +seq_printf (m,"0x%lx 0x%lx\n" ,NFSEXP_ALLFLAGS ,NFSEXP_SECINFO_FLAGS );return 0 ; }include/uapi/linux/nfsd/export.h +0
-12@@ -26,22 +26,22 @@* Please update the expflags[] array in fs/nfsd/export.c when adding * a new flag. */ -#define NFSEXP_READONLY 0x0001 -#define NFSEXP_INSECURE_PORT 0x0002 -#define NFSEXP_ROOTSQUASH 0x0004 -#define NFSEXP_ALLSQUASH 0x0008 -#define NFSEXP_ASYNC 0x0010 -#define NFSEXP_GATHERED_WRITES 0x0020 -#define NFSEXP_NOREADDIRPLUS 0x0040 -#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ -#define NFSEXP_NOHIDE 0x0200 -#define NFSEXP_NOSUBTREECHECK 0x0400 -#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
-#defineNFSEXP_MSNFS0x1000/* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +0
-3-#define NFSEXP_FSID 0x2000 -#define NFSEXP_CROSSMOUNT 0x4000 -#define NFSEXP_NOACL 0x8000/* reserved for possible ACL related use */
include/uapi/linux/nfsd/export.h +12
-0+#define NFSEXP_READONLY BIT(0) +#define NFSEXP_INSECURE_PORT BIT(1) +#define NFSEXP_ROOTSQUASH BIT(2) +#define NFSEXP_ALLSQUASH BIT(3) +#define NFSEXP_ASYNC BIT(4) +#define NFSEXP_GATHERED_WRITES BIT(5) +#define NFSEXP_NOREADDIRPLUS BIT(6) +#define NFSEXP_SECURITY_LABEL BIT(7) +/* BIT(8) currently unused */ +#define NFSEXP_NOHIDE BIT(9) +#define NFSEXP_NOSUBTREECHECK BIT(10) +#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
+#defineNFSEXP_MSNFSBIT(12) /* do silly things that MS clients
expect; no longer supported */include/uapi/linux/nfsd/export.h +3
-0+#define NFSEXP_FSID BIT(13) +#define NFSEXP_CROSSMOUNT BIT(14) +#define NFSEXP_NOACL BIT(15)/* reserved for possible ACL related use */
/*
* TheNFSEXP_V4ROOTflag causes the kernel to give access only to
NFSv4
* clients, and only to the single directory that is the root of theinclude/uapi/linux/nfsd/export.h +0
-0@@ -51,11 +51,11 @@* pseudofilesystem, which provides access only to paths leading to each
* exported filesystem.
*/include/uapi/linux/nfsd/export.h +3
-3-#define NFSEXP_V4ROOT 0x10000 -#define NFSEXP_PNFS 0x20000 +#define NFSEXP_V4ROOT BIT(16) +#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \Signature · 1 line
2.50.1This might constitute a breaking user space API change.
BIT()is a kernel convention. What definesBIT()for user space consumers of this header?
Quoted 6 lines
Quoted 5 lines
Doh - good catch. I can drop this, or maybe add:
#ifndef BIT
#define BIT(n) (1UL << (n))
#endifI'm happy leaving these as hex constants.
Got it. I'll drop this on the next version.
Ben
PATCH 2/4 nfsd: Add a key for signing filehandles
In reply to Benjamin CoddingtonExpand the nfsd_net to hold a siphash_key_t value "fh_key".
Expand the netlink server interface to allow the setting of the 128-bit fh_key value to be used as a signing key for filehandles.
Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)
Documentation/netlink/specs/nfsd.yaml +12 -0
@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:
@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do: + request: + attributes: + - fh-key
fs/nfsd/netlink.c +15 -0
@@ -47,6 +47,14 @@ static const struct nla_policy nfsd_pool_mode_set_nl_policy[NFSD_A_POOL_MODE_MOD [NFSD_A_POOL_MODE_MODE] = { .type = NLA_NUL_STRING, }, }; +/* NFSD_CMD_FH_KEY_SET - do */ +static const struct nla_policy nfsd_fh_key_set_nl_policy[NFSD_A_SERVER_FH_KEY +1 ] = { + [NFSD_A_SERVER_FH_KEY ] = { + .type =NLA_BINARY , + .len =16 + }, +}; +/* Ops table for nfsd */ static const struct genl_split_ops nfsd_nl_ops[] = { {
@@ -102,6 +110,13 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .doit = nfsd_nl_pool_mode_get_doit, .flags = GENL_CMD_CAP_DO, }, + { + .cmd = NFSD_CMD_FH_KEY_SET, + .doit = nfsd_nl_fh_key_set_doit, + .policy = nfsd_fh_key_set_nl_policy, + .maxattr = NFSD_A_SERVER_FH_KEY, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, };struct genl_family nfsd_nl_family__ro_after_init = {
fs/nfsd/netlink.h +1 -0
@@ -26,6 +26,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info);int nfsd_nl_listener_get_doit (struct sk_buff *skb,struct genl_info *info);int nfsd_nl_pool_mode_set_doit (struct sk_buff *skb,struct genl_info *info);int nfsd_nl_pool_mode_get_doit (struct sk_buff *skb,struct genl_info *info); +int nfsd_nl_fh_key_set_doit (struct sk_buff *skb,struct genl_info *info);extern struct genl_family nfsd_nl_family;
fs/nfsd/netns.h +2 -0
@@ -16,6 +16,7 @@#include <linux/percpu-refcount.h> #include <linux/siphash.h> #include <linux/sunrpc/stats.h> +#include <linux/siphash.h> /* Hash tables for nfs4_clientid state */ #define CLIENT_HASH_BITS 4
@@ -224,6 +225,7 @@ struct nfsd_net {spinlock_t local_clients_lock;struct list_head local_clients;#endif +siphash_key_t *fh_key; };/* Simple check to find out if a given net was properly initialized */
fs/nfsd/nfsctl.c +85 -0
133 lines not on this page · raw message
fs/nfsd/trace.h +19 -0
@@ -2240,6 +2240,25 @@ TRACE_EVENT(nfsd_end_grace, ) ); +TRACE_EVENT (nfsd_ctl_fh_key_set, +TP_PROTO ( + const char *key, + int result + ), +TP_ARGS (key, result), +TP_STRUCT__entry ( + __array(unsigned char, key, 16) + __field(unsigned long, result) + ), +TP_fast_assign ( + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" ,__print_hex (__entry->key,16 ), + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copy
include/uapi/linux/nfsd_netlink.h +2 -0
@@ -36,6 +36,7 @@ enum {NFSD_A_SERVER_LEASETIME ,NFSD_A_SERVER_SCOPE ,NFSD_A_SERVER_MIN_THREADS , +NFSD_A_SERVER_FH_KEY , __NFSD_A_SERVER_MAX,NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX -1 )
@@ -90,6 +91,7 @@ enum {NFSD_CMD_LISTENER_GET ,NFSD_CMD_POOL_MODE_SET ,NFSD_CMD_POOL_MODE_GET , +NFSD_CMD_FH_KEY_SET , __NFSD_CMD_MAX,NFSD_CMD_MAX = (__NFSD_CMD_MAX -1 )
Signature · 1 line
2.50.1
Jeff Layton
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 36 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +3
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 10 lines
Documentation/netlink/specs/nfsd.yaml +9
-0- npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do: + request: + attributes: + - fh-key
Rather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:
Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
What feels a little strange though is where to store the key? I was thinking in /etc/exports, but that would make the FH key per-export rather than per-server instance.
That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
Signature · 1 line
Chuck Lever
Re: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 70 lines · Chuck Lever
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
What feels a little strange though is where to store the key? I was thinking in
/etc/exports, but that would make the FH key per-export rather than per-server instance.
Quoted 7 lines
That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
I'm hacking out a problem in the nfs-utils patches, should be coming soon - but if the key is already set, the utils don't halt or report an error.
The patches have the key generated by hashing the contents of a file set via nfs.conf: fh_key_file=/etc/nfs_fh.key for example.
No additional crypto benefit for having a different key per-export, because the filehandles start with fsid the siphash naturally computes differently for each export of a different filesystem.
By all means attack these points on the userspace patches.
Ben
Jeff Layton
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
In reply to Chuck LeverQuoted 59 lines · Chuck Lever
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Quoted 8 lines
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
I'm not convinced. We could easily vet that the key doesn't change when changing the thread count, and either return an error or throw some sort of warning and ignore the change.
My main thinking here is that you'd want to set up the key at startup time and never change it, so if the server is already running you probably want to reject key changes -- otherwise you may have already given out some unencrypted handles.
If that's the case, then now you have to ensure you run the op to set the key before issuing "threads".
Why deal with an ordering constraint like that? Optionally passing down the key with "threads" means we handle it all in one shot.
Quoted 4 lines
What feels a little strange though is where to store the key? I was thinking in
/etc/exports, but that would make the FH key per-export rather than per-server instance.
Quoted 8 lines
That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
Yeah, there are a lot of possibilities here. I like the idea of scraping this out of the TPM, but that's not going to be possible everywhere. We'll also need some alternate method of storing the key in a secure way on the fs so that nfsdctl can get to it for hosts that don't have a TPM.
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 78 lines · Jeff Layton
Quoted 59 lines · Chuck Lever
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Quoted 8 lines
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
I'm not convinced. We could easily vet that the key doesn't change when changing the thread count, and either return an error or throw some sort of warning and ignore the change.
My main thinking here is that you'd want to set up the key at startup time and never change it, so if the server is already running you probably want to reject key changes -- otherwise you may have already given out some unencrypted handles.
Quoted 5 lines
If that's the case, then now you have to ensure you run the op to set the key before issuing "threads".
Why deal with an ordering constraint like that? Optionally passing down the key with "threads" means we handle it all in one shot.
We already configure listeners and threads in separate operations. The ordering is managed. It's reasonable for the kernel to block fh_key changes while the NFS server is in operation.
I'd much rather set a precedent of several small ops rather than one or two Swiss army knives.
Quoted 13 lines
Quoted 4 lines
What feels a little strange though is where to store the key? I was thinking in
/etc/exports, but that would make the FH key per-export rather than per-server instance.Quoted 8 lines
That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
Quoted 5 lines
Yeah, there are a lot of possibilities here. I like the idea of scraping this out of the TPM, but that's not going to be possible everywhere. We'll also need some alternate method of storing the key in a secure way on the fs so that nfsdctl can get to it for hosts that don't have a TPM.
My point is none of this has anything to do with thread count. Setting the fh_key needs to be a distinct UI element.
Signature · 1 line
Chuck Lever
Jeff Layton
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 86 lines · Chuck Lever
Quoted 78 lines · Jeff Layton
Quoted 59 lines · Chuck Lever
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Quoted 8 lines
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
I'm not convinced. We could easily vet that the key doesn't change when changing the thread count, and either return an error or throw some sort of warning and ignore the change.
My main thinking here is that you'd want to set up the key at startup time and never change it, so if the server is already running you probably want to reject key changes -- otherwise you may have already given out some unencrypted handles.
Quoted 5 lines
If that's the case, then now you have to ensure you run the op to set the key before issuing "threads".
Why deal with an ordering constraint like that? Optionally passing down the key with "threads" means we handle it all in one shot.
Quoted 8 lines
We already configure listeners and threads in separate operations. The ordering is managed. It's reasonable for the kernel to block
fh_keychanges while the NFS server is in operation.I'd much rather set a precedent of several small ops rather than one or two Swiss army knives.
I disagree. Having all of the server settings as discrete elements was part of the problem with the old nfsdfs-based interface. We had this set of discrete knobs that needed to all be twiddled in the correct order.
The fact that we can send down server parameters in a single block is a strength of the netlink interface, IMO. We can easily add optional parameters and I think that's what we should do here.
Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
What feels a little strange though is where to store the key? I was thinking in
/etc/exports, but that would make the FH key per-export rather than per-server instance.That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
Quoted 8 lines
Quoted 5 lines
Yeah, there are a lot of possibilities here. I like the idea of scraping this out of the TPM, but that's not going to be possible everywhere. We'll also need some alternate method of storing the key in a secure way on the fs so that nfsdctl can get to it for hosts that don't have a TPM.
My point is none of this has anything to do with thread count. Setting the
fh_keyneeds to be a distinct UI element.
"threads" should probably have been named "service", since it's basically the method that we use to create the actual service, culminating with starting threads.
Signature · 1 line
Jeff Layton <[email protected]>
Chuck Lever
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 125 lines · Jeff Layton
Quoted 86 lines · Chuck Lever
Quoted 78 lines · Jeff Layton
Quoted 59 lines · Chuck Lever
Quoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Setting the FH key in the threads op seems awkward to me. Setting a key is optional, but you always set the thread count to start the server.
Quoted 8 lines
Key setting is done once; whereas setting the thread count can be done many times during operation. It seems like it would be easy to mistakenly change the key when setting the thread count.
From a "UI safety" perspective, a separate op makes sense to me.
I'm not convinced. We could easily vet that the key doesn't change when changing the thread count, and either return an error or throw some sort of warning and ignore the change.
My main thinking here is that you'd want to set up the key at startup time and never change it, so if the server is already running you probably want to reject key changes -- otherwise you may have already given out some unencrypted handles.
Quoted 5 lines
If that's the case, then now you have to ensure you run the op to set the key before issuing "threads".
Why deal with an ordering constraint like that? Optionally passing down the key with "threads" means we handle it all in one shot.
Quoted 8 lines
We already configure listeners and threads in separate operations. The ordering is managed. It's reasonable for the kernel to block
fh_keychanges while the NFS server is in operation.I'd much rather set a precedent of several small ops rather than one or two Swiss army knives.
I disagree. Having all of the server settings as discrete elements was part of the problem with the old nfsdfs-based interface. We had this set of discrete knobs that needed to all be twiddled in the correct order.
The fact that we can send down server parameters in a single block is a strength of the netlink interface, IMO. We can easily add optional parameters and I think that's what we should do here.
Quoted 19 lines
Quoted 18 lines
Quoted 4 lines
What feels a little strange though is where to store the key? I was thinking in
/etc/exports, but that would make the FH key per-export rather than per-server instance.Quoted 8 lines
That gives a cryptographic benefit, as there would be more keying material. But maybe it doesn't make a lot of sense from a UX perspective.
On the other hand, some might like to manage the key by storing it in a trusted compute module -- systemd has a facility to extract keys from a TCM.
Yeah, there are a lot of possibilities here. I like the idea of scraping this out of the TPM, but that's not going to be possible everywhere. We'll also need some alternate method of storing the key in a secure way on the fs so that nfsdctl can get to it for hosts that don't have a TPM.
Quoted 6 lines
Quoted 2 lines
My point is none of this has anything to do with thread count. Setting the
fh_keyneeds to be a distinct UI element."threads" should probably have been named "service", since it's basically the method that we use to create the actual service, culminating with starting threads.
OK, you rode herd on getting the netlink stuff in, so it's your call here.
It might be nice to create a new op called "service" and leave the legacy "threads" op in place for now.
Signature · 1 line
Chuck Lever
Re: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
In reply to Jeff LaytonQuoted 44 lines · Jeff Layton
Quoted 43 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +9
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles + attribute-set: server + flags: [admin-perm] + do:
Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 241
Quoted 3 lines
Documentation/netlink/specs/nfsd.yaml +3
-0+ request: + attributes: + - fh-keyRather than a new netlink operation, I think we might be better served with just sending the fh-key down as an optional attribute in the "threads" op. It's a per-netns attribute anyway, and the threads setting is handled similarly.
Ok - will do on v2.
Ben
Chuck Lever
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 7 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Generally I like to see more "why" in the commit message. This message just repeats what the diff says.
Since the actual rationale will be lengthy, I would say this is one of those rare occasions when including a Link: tag that refers the reader to the cover letter in the lore archive might be helpful.
Quoted 27 lines
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +3
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:
Documentation/netlink/specs/nfsd.yaml operations: · line 235
Quoted 6 lines
Documentation/netlink/specs/nfsd.yaml +3
-0@@ -227,3 +230,12 @@ operations: attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandles
Nit: "set signing key for filehandles"
Quoted 132 lines
Documentation/netlink/specs/nfsd.yaml +6
-0+ attribute-set: server + flags: [admin-perm] + do: + request: + attributes: + - fh-keyfs/nfsd/netlink.c +0
-0@@ -47,6 +47,14 @@ static const struct nla_policynfsd_pool_mode_set_nl_policy[NFSD_A_POOL_MODE_MOD
fs/nfsd/netlink.c +2
-0[NFSD_A_POOL_MODE_MODE] = { .type = NLA_NUL_STRING, }, }; +/* NFSD_CMD_FH_KEY_SET - do */ +static const struct nla_policynfsd_fh_key_set_nl_policy[NFSD_A_SERVER_FH_KEY + 1] = {
fs/nfsd/netlink.c +13
-0+ [NFSD_A_SERVER_FH_KEY ] = { + .type =NLA_BINARY , + .len =16 + }, +}; +/* Ops table for nfsd */ static const struct genl_split_ops nfsd_nl_ops[] = { {@@ -102,6 +110,13 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .doit = nfsd_nl_pool_mode_get_doit, .flags = GENL_CMD_CAP_DO, }, + { + .cmd = NFSD_CMD_FH_KEY_SET, + .doit = nfsd_nl_fh_key_set_doit, + .policy = nfsd_fh_key_set_nl_policy, + .maxattr = NFSD_A_SERVER_FH_KEY, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, };struct
genl_familynfsd_nl_family__ro_after_init= {fs/nfsd/netlink.h +0
-0@@ -26,6 +26,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info);int nfsd_nl_listener_get_doit (struct sk_buff *skb,struct genl_info *info);int nfsd_nl_pool_mode_set_doit (struct sk_buff *skb,struct genl_info *info); int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info*info);
+intnfsd_nl_fh_key_set_doit(struct sk_buff *skb, struct genl_info *info);extern struct
genl_familynfsd_nl_family;fs/nfsd/netns.h +2
-0@@ -16,6 +16,7 @@#include <linux/percpu-refcount.h> #include <linux/siphash.h> #include <linux/sunrpc/stats.h> +#include <linux/siphash.h> /* Hash tables for nfs4_clientid state */ #define CLIENT_HASH_BITS 4@@ -224,6 +225,7 @@ struct nfsd_net {spinlock_t local_clients_lock;struct list_head local_clients;#endif +siphash_key_t *fh_key; };/* Simple check to find out if a given net was properly initialized */ fs/nfsd/nfsctl.c +3
-0@@ -19,6 +19,7 @@#include <linux/module.h> #include <linux/fsnotify.h> #include <linux/nfslocalio.h> +#include <crypto/skcipher.h> #include "idmap.h" #include "nfsd.h" @@ -49,6 +50,7 @@ enum { NFSD_Ports, NFSD_MaxBlkSize, NFSD_MinThreads, + NFSD_Fh_Key, NFSD_Filecache, NFSD_Leasetime, NFSD_Gracetime,@@ -69,6 +71,7 @@ static ssize_t write_versions(struct file *file, char *buf, size_t size);static ssize_t write_ports (struct file *file,char *buf,size_t size);static ssize_t write_maxblksize (struct file *file,char *buf,size_t size);static ssize_t write_minthreads (struct file *file,char *buf,size_t size); +static ssize_t write_fh_key (struct file *file,char *buf,size_t size);#ifdef
CONFIG_NFSD_V4
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -88,6 +91,7 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = { [NFSD_Ports] = write_ports, [NFSD_MaxBlkSize] = write_maxblksize, [NFSD_MinThreads] = write_minthreads, + [NFSD_Fh_Key] = write_fh_key, #ifdef CONFIG_NFSD_V4 [NFSD_Leasetime] = write_leasetime,fs/nfsd/nfsctl.c +6
-0[NFSD_Gracetime] = write_gracetime,@@ -950,6 +954,54 @@ static ssize_t write_minthreads(struct file *file, char *buf, size_t size) return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%u\n", minthreads); } +/* +* write_fh_key - Set or report the current NFS filehandle key +* +* Input: +* buf: ignored +* size: zero
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, · line 973
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+* OR +* +* Input: +* buf: C string containing a parseable UUID +* size: non-zero length of C string in @buf +* Output: +* On success: passed-in buffer filled with '\n'-terminated C string +* containing the standard UUID format of the server's fh_key +* return code is the size in bytes of the string +* On error: return code is zero or a negative errno value
Nit: it would be nice to explain (briefly) why fh_key rotation during server operation is prohibited.
Quoted 13 lines
fs/nfsd/nfsctl.c +13
-0+*/ +static ssize_t write_fh_key (struct file *file,char *buf,size_t size) +{ +struct nfsd_net *nn =net_generic (netns (file), nfsd_net_id); + +if (size >35 && size <38 ) { +siphash_key_t *sip_fh_key; +uuid_t uuid_fh_key; +int ret; + +/* Is the key already set? */ +if (nn->fh_key) +return -EEXIST ;
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 995
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+ + ret =uuid_parse (buf, &uuid_fh_key); +if (ret) +return ret; + + sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) +return -ENOMEM ; + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t));
What protects updates of nn->fh_key from concurrent writers? A race might result in leaking a previous fh_key buffer, when it should return EEXIST. So you'll need some mutual exclusion here -- probably nfsd_mutex.
Ditto for the netlink interface.
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 1000
Quoted 5 lines
fs/nfsd/nfsctl.c +5
-0+ nn->fh_key = sip_fh_key; + +trace_nfsd_ctl_fh_key_set ((const char *)sip_fh_key, ret); + } +
If user space reads the key before it has been set, would nn->fh_key be NULL here?
Quoted 45 lines
fs/nfsd/nfsctl.c +4
-0+return scnprintf (buf,SIMPLE_TRANSACTION_LIMIT ,"%pUb\n" , + nn->fh_key); +} +#ifdef CONFIG_NFSD_V4 static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_tsize,
fs/nfsd/nfsctl.c +1
-0time64_t *time, struct nfsd_net *nn)@@ -1343,6 +1395,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc) [NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MaxBlkSize] = {"max_block_size" , &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MinThreads] = {"min_threads" , &transaction_ops, S_IWUSR|S_IRUGO}, + [NFSD_Fh_Key] = {"fh_key", &transaction_ops, S_IWUSR|S_IRUSR},[
NFSD_Filecache] = {"filecache",&nfsd_file_cache_stats_fops,S_IRUGO},
#ifdefCONFIG_NFSD_V4
[NFSD_Leasetime] = {"nfsv4leasetime",&transaction_ops,
S_IWUSR|S_IRUSR},fs/nfsd/nfsctl.c +18
-0@@ -2199,6 +2252,37 @@ int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info)return err; } +int nfsd_nl_fh_key_set_doit (struct sk_buff *skb,struct genl_info *info) +{ +siphash_key_t *fh_key; +struct nfsd_net *nn; +int fh_key_len; +int ret; + +if (GENL_REQ_ATTR_CHECK (info,NFSD_A_SERVER_FH_KEY )) +return -EINVAL ; + + fh_key_len =nla_len (info->attrs[NFSD_A_SERVER_FH_KEY ]); +if (fh_key_len !=sizeof (siphash_key_t)) +return -EINVAL ; + +/* Is the key already set? */ + nn =net_generic (genl_info_net (info), nfsd_net_id); +if (nn->fh_key) +return -EEXIST ;
fs/nfsd/nfsctl.c int nfsd_nl_pool_mode_get_doit(struct sk_buff · line 2284
Quoted 10 lines
fs/nfsd/nfsctl.c +9
-0+ + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + +memcpy (fh_key,nla_data (info->attrs[NFSD_A_SERVER_FH_KEY ]),sizeof (siphash_key_t)); + nn =net_generic (genl_info_net (info), nfsd_net_id); + nn->fh_key = fh_key; +
On success, "ret" hasn't been initialized. And maybe you want the error flows above to exit through here to get those return codes captured by the trace point.
fs/nfsd/nfsctl.c static __net_exit void nfsd_net_exit(struct net · line 2371
Quoted 12 lines
fs/nfsd/nfsctl.c +4
-0+trace_nfsd_ctl_fh_key_set ((const char *)fh_key, ret); +return ret; +} +/**
*nfsd_net_init- Prepare thenfsd_netportion of a new net namespace
* @net: a freshly-created network namespacefs/nfsd/nfsctl.c +0
-0@@ -2284,6 +2368,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id);
To save an extra pointer dereference on the hotter paths, maybe fh_key should be the actual key rather than a pointer. You can use kfree_sensitive() when freeing the whole struct nfsd_net, or just memset the fh_key field before freeing nfsd_net.
Just a random thought.
Quoted 57 lines
fs/nfsd/nfsctl.c +1
-0+kfree_sensitive (nn->fh_key);nfsd_proc_stat_shutdown (net);percpu_counter_destroy_many (nn->counter,NFSD_STATS_COUNTERS_NUM );fs/nfsd/nfsctl.c +0
-0nfsd_idmap_shutdown (net);fs/nfsd/trace.h +19
-0@@ -2240,6 +2240,25 @@ TRACE_EVENT(nfsd_end_grace, ) ); +TRACE_EVENT (nfsd_ctl_fh_key_set, +TP_PROTO ( + const char *key, + int result + ), +TP_ARGS (key, result), +TP_STRUCT__entry ( + __array(unsigned char, key, 16) + __field(unsigned long, result) + ), +TP_fast_assign ( + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" ,__print_hex (__entry->key,16 ), + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copyinclude/uapi/linux/nfsd_netlink.h +2
-0@@ -36,6 +36,7 @@ enum {NFSD_A_SERVER_LEASETIME ,NFSD_A_SERVER_SCOPE ,NFSD_A_SERVER_MIN_THREADS , +NFSD_A_SERVER_FH_KEY , __NFSD_A_SERVER_MAX,NFSD_A_SERVER_MAX = (__NFSD_A_SERVER_MAX -1 )@@ -90,6 +91,7 @@ enum {NFSD_CMD_LISTENER_GET ,NFSD_CMD_POOL_MODE_SET ,NFSD_CMD_POOL_MODE_GET , +NFSD_CMD_FH_KEY_SET , __NFSD_CMD_MAX,NFSD_CMD_MAX = (__NFSD_CMD_MAX -1 )Signature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
Re: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 9 lines · Chuck Lever
Quoted 7 lines · Benjamin Coddington
Expand the
nfsd_netto hold asiphash_key_tvalue "fh_key".Expand the netlink server interface to allow the setting of the 128-bit
fh_keyvalue to be used as a signing key for filehandles.Add a file to the nfsd filesystem to set and read the 128-bit key, formatted as a uuid.
Quoted 6 lines
Generally I like to see more "why" in the commit message. This message just repeats what the diff says.
Since the actual rationale will be lengthy, I would say this is one of those rare occasions when including a Link: tag that refers the reader to the cover letter in the lore archive might be helpful.
I'll add a bit more context, and the link.
Quoted 27 lines
Quoted 27 lines
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 85 +++++++++++++++++++++++++++ fs/nfsd/trace.h | 19 ++++++ include/uapi/linux/nfsd_netlink.h | 2 + 7 files changed, 136 insertions(+)Documentation/netlink/specs/nfsd.yaml +3
-0@@ -81,6 +81,9 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary - name: version attributes:@@ -227,3 +230,12 @@ operations:
Quoted 8 lines
Documentation/netlink/specs/nfsd.yaml operations: · line 235
Quoted 6 lines
Documentation/netlink/specs/nfsd.yaml +3
-0attributes: - mode - npools + - + name: fh-key-set + doc: set encryption key for filehandlesNit: "set signing key for filehandles"
Got it - there's encryption artifacts in 4/4 too..
Quoted 137 lines
Quoted 137 lines
Documentation/netlink/specs/nfsd.yaml +6
-0+ attribute-set: server + flags: [admin-perm] + do: + request: + attributes: + - fh-keyfs/nfsd/netlink.c +0
-0@@ -47,6 +47,14 @@ static const struct nla_policynfsd_pool_mode_set_nl_policy[NFSD_A_POOL_MODE_MOD
fs/nfsd/netlink.c +2
-0[NFSD_A_POOL_MODE_MODE] = { .type = NLA_NUL_STRING, }, }; +/* NFSD_CMD_FH_KEY_SET - do */ +static const struct nla_policynfsd_fh_key_set_nl_policy[NFSD_A_SERVER_FH_KEY + 1] = {
fs/nfsd/netlink.c +13
-0+ [NFSD_A_SERVER_FH_KEY ] = { + .type =NLA_BINARY , + .len =16 + }, +}; +/* Ops table for nfsd */ static const struct genl_split_ops nfsd_nl_ops[] = { {@@ -102,6 +110,13 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .doit = nfsd_nl_pool_mode_get_doit, .flags = GENL_CMD_CAP_DO, }, + { + .cmd = NFSD_CMD_FH_KEY_SET, + .doit = nfsd_nl_fh_key_set_doit, + .policy = nfsd_fh_key_set_nl_policy, + .maxattr = NFSD_A_SERVER_FH_KEY, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + }, };struct
genl_familynfsd_nl_family__ro_after_init= {fs/nfsd/netlink.h +0
-0@@ -26,6 +26,7 @@ int nfsd_nl_listener_set_doit(struct sk_buff *skb, struct genl_info *info);int nfsd_nl_listener_get_doit (struct sk_buff *skb,struct genl_info *info);int nfsd_nl_pool_mode_set_doit (struct sk_buff *skb,struct genl_info *info); int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info*info);
+intnfsd_nl_fh_key_set_doit(struct sk_buff *skb, struct genl_info *info);extern struct
genl_familynfsd_nl_family;fs/nfsd/netns.h +2
-0@@ -16,6 +16,7 @@#include <linux/percpu-refcount.h> #include <linux/siphash.h> #include <linux/sunrpc/stats.h> +#include <linux/siphash.h> /* Hash tables for nfs4_clientid state */ #define CLIENT_HASH_BITS 4@@ -224,6 +225,7 @@ struct nfsd_net {spinlock_t local_clients_lock;struct list_head local_clients;#endif +siphash_key_t *fh_key; };/* Simple check to find out if a given net was properly initialized */ fs/nfsd/nfsctl.c +3
-0@@ -19,6 +19,7 @@#include <linux/module.h> #include <linux/fsnotify.h> #include <linux/nfslocalio.h> +#include <crypto/skcipher.h> #include "idmap.h" #include "nfsd.h" @@ -49,6 +50,7 @@ enum { NFSD_Ports, NFSD_MaxBlkSize, NFSD_MinThreads, + NFSD_Fh_Key, NFSD_Filecache, NFSD_Leasetime, NFSD_Gracetime,@@ -69,6 +71,7 @@ static ssize_t write_versions(struct file *file, char *buf, size_t size);static ssize_t write_ports (struct file *file,char *buf,size_t size);static ssize_t write_maxblksize (struct file *file,char *buf,size_t size);static ssize_t write_minthreads (struct file *file,char *buf,size_t size); +static ssize_t write_fh_key (struct file *file,char *buf,size_t size);#ifdef
CONFIG_NFSD_V4
staticssize_twrite_leasetime(struct file *file, char *buf,size_t
size);
staticssize_twrite_gracetime(struct file *file, char *buf,size_t
size);fs/nfsd/nfsctl.c +1
-0@@ -88,6 +91,7 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = { [NFSD_Ports] = write_ports, [NFSD_MaxBlkSize] = write_maxblksize, [NFSD_MinThreads] = write_minthreads, + [NFSD_Fh_Key] = write_fh_key, #ifdef CONFIG_NFSD_V4 [NFSD_Leasetime] = write_leasetime,fs/nfsd/nfsctl.c +11
-0[NFSD_Gracetime] = write_gracetime,@@ -950,6 +954,54 @@ static ssize_t write_minthreads(struct file *file, char *buf, size_t size) return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%u\n", minthreads); } +/* +* write_fh_key - Set or report the current NFS filehandle key +* +* Input: +* buf: ignored +* size: zero +* OR +* +* Input: +* buf: C string containing a parseable UUID +* size: non-zero length of C string in @buf
Quoted 8 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 972
Quoted 5 lines
fs/nfsd/nfsctl.c +5
-0+* Output: +* On success: passed-in buffer filled with '\n'-terminated C string +* containing the standard UUID format of the server's fh_key +* return code is the size in bytes of the string +* On error: return code is zero or a negative errno value Nit: it would be nice to explain (briefly) why
fh_keyrotation during server operation is prohibited.
Ok, will do.
Quoted 17 lines
Quoted 17 lines
fs/nfsd/nfsctl.c +17
-0+*/ +static ssize_t write_fh_key (struct file *file,char *buf,size_t size) +{ +struct nfsd_net *nn =net_generic (netns (file), nfsd_net_id); + +if (size >35 && size <38 ) { +siphash_key_t *sip_fh_key; +uuid_t uuid_fh_key; +int ret; + +/* Is the key already set? */ +if (nn->fh_key) +return -EEXIST ; + + ret =uuid_parse (buf, &uuid_fh_key); +if (ret) +return ret;
Quoted 8 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 995
Quoted 6 lines
fs/nfsd/nfsctl.c +6
-0+ + sip_fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!sip_fh_key) +return -ENOMEM ; + +memcpy (sip_fh_key, &uuid_fh_key,sizeof (siphash_key_t));What protects updates of
nn->fh_keyfrom concurrent writers?
Just the check of the null pointer -- but looks like I forgot to check it here. It has a TOCTOU anyway, so yes needs and will add the mutex.
Quoted 5 lines
A race might result in leaking a previous
fh_keybuffer, when it should return EEXIST. So you'll need some mutual exclusion here -- probablynfsd_mutex.Ditto for the netlink interface.
Ditto ack.
Quoted 8 lines
fs/nfsd/nfsctl.c static ssize_t write_minthreads(struct file *file, char *buf, size_t size) · line 1000
Quoted 5 lines
fs/nfsd/nfsctl.c +5
-0+ nn->fh_key = sip_fh_key; + +trace_nfsd_ctl_fh_key_set ((const char *)sip_fh_key, ret); + } +If user space reads the key before it has been set, would
nn->fh_keybe NULL here?
Yes, but scnprintf is smart enough to print "(null)"
Quoted 51 lines
Quoted 51 lines
fs/nfsd/nfsctl.c +4
-0+return scnprintf (buf,SIMPLE_TRANSACTION_LIMIT ,"%pUb\n" , + nn->fh_key); +} +#ifdef CONFIG_NFSD_V4 static ssize_t __nfsd4_write_time(struct file *file, char *buf, size_tsize,
fs/nfsd/nfsctl.c +1
-0time64_t *time, struct nfsd_net *nn)@@ -1343,6 +1395,7 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc) [NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MaxBlkSize] = {"max_block_size" , &transaction_ops, S_IWUSR|S_IRUGO}, [NFSD_MinThreads] = {"min_threads" , &transaction_ops, S_IWUSR|S_IRUGO}, + [NFSD_Fh_Key] = {"fh_key", &transaction_ops, S_IWUSR|S_IRUSR},[
NFSD_Filecache] = {"filecache",&nfsd_file_cache_stats_fops,S_IRUGO},
#ifdefCONFIG_NFSD_V4
[NFSD_Leasetime] = {"nfsv4leasetime",&transaction_ops,
S_IWUSR|S_IRUSR},fs/nfsd/nfsctl.c +24
-0@@ -2199,6 +2252,37 @@ int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info)return err; } +int nfsd_nl_fh_key_set_doit (struct sk_buff *skb,struct genl_info *info) +{ +siphash_key_t *fh_key; +struct nfsd_net *nn; +int fh_key_len; +int ret; + +if (GENL_REQ_ATTR_CHECK (info,NFSD_A_SERVER_FH_KEY )) +return -EINVAL ; + + fh_key_len =nla_len (info->attrs[NFSD_A_SERVER_FH_KEY ]); +if (fh_key_len !=sizeof (siphash_key_t)) +return -EINVAL ; + +/* Is the key already set? */ + nn =net_generic (genl_info_net (info), nfsd_net_id); +if (nn->fh_key) +return -EEXIST ; + + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + + memcpy(fh_key, nla_data(info->attrs[NFSD_A_SERVER_FH_KEY]),
Quoted 8 lines
fs/nfsd/nfsctl.c int nfsd_nl_pool_mode_get_doit(struct sk_buff *skb, struct genl_info *info) · line 2281
Quoted 4 lines
fs/nfsd/nfsctl.c +3
-0sizeof(siphash_key_t)); + nn =net_generic (genl_info_net (info), nfsd_net_id); + nn->fh_key = fh_key; +On success, "ret" hasn't been initialized. And maybe you want the error flows above to exit through here to get those return codes captured by the trace point.
Gah, thanks!
Quoted 19 lines
fs/nfsd/nfsctl.c static __net_exit void nfsd_net_exit(struct net · line 2371
Quoted 13 lines
fs/nfsd/nfsctl.c +4
-0+trace_nfsd_ctl_fh_key_set ((const char *)fh_key, ret); +return ret; +} +/**
*nfsd_net_init- Prepare thenfsd_netportion of a new net namespace
* @net: a freshly-created network namespacefs/nfsd/nfsctl.c +0
-0@@ -2284,6 +2368,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id);To save an extra pointer dereference on the hotter paths, maybe
fh_keyshould be the actual key rather than a pointer. You can usekfree_sensitive()when freeing the whole structnfsd_net, or just memset thefh_keyfield before freeingnfsd_net.
Quoted 1 line
Just a random thought.
Could do! ..but its easier to check if a pointer is NULL than to check two u64's being unset. That said, having half the key be zero is probably insane odds. I also figured to minimize the size of nfsd_net when this is not used. I think I like the pointer better, if that's acceptable.
Thank you for your review here.
Ben
Chuck Lever
maintainerRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
Quoted 6 lines · Benjamin Coddington
Quoted 5 lines · Chuck Lever
To save an extra pointer dereference on the hotter paths, maybe
fh_keyshould be the actual key rather than a pointer. You can usekfree_sensitive()when freeing the whole structnfsd_net, or just memset thefh_keyfield before freeingnfsd_net.
Quoted 6 lines
Quoted 1 line
Just a random thought.
Could do! ..but its easier to check if a pointer is NULL than to check two u64's being unset. That said, having half the key be zero is probably insane odds. I also figured to minimize the size of
nfsd_netwhen this is not used. I think I like the pointer better, if that's acceptable.
Agreed, the pointer has the benefit of showing whether the fh_key has been set or not.
Signature · 1 line
Chuck Lever
kernel test robot
build botRe: [PATCH v1 2/4] nfsd: Add a key for signing filehandles
In reply to Benjamin Coddingtonkernel test robot noticed the following build warnings:
Hi Benjamin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bfd453acb5637b5df881cef4b21803344aa9e7ac]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Coddington/nfsd-Convert-export-flags-to-use-BIT-macro/20260116-223927 base: bfd453acb5637b5df881cef4b21803344aa9e7ac patch link: https://lore.kernel.org/r/c49d28aade36c044f0533d03b564ff65e00d9e05.1768573690.git.bcodding%40hammerspace.com patch subject: [PATCH v1 2/4] nfsd: Add a key for signing filehandles config: x86_64-kexec (https://download.01.org/0day-ci/archive/20260117/[email protected]/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260117/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> fs/nfsd/nfsctl.c:2282:50: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
2282 | trace_nfsd_ctl_fh_key_set((const char *)fh_key, ret);
| ^~~
fs/nfsd/nfsctl.c:2260:9: note: initialize the variable 'ret' to silence this warning
2260 | int ret;
| ^
| = 01 warning generated.
vim +/ret +2282 fs/nfsd/nfsctl.c
2254
2255 int nfsd_nl_fh_key_set_doit(struct sk_buff *skb, struct genl_info *info)
2256 {
2257 siphash_key_t *fh_key;
2258 struct nfsd_net *nn;
2259 int fh_key_len;
2260 int ret;
2261
2262 if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_FH_KEY))
2263 return -EINVAL;
2264
2265 fh_key_len = nla_len(info->attrs[NFSD_A_SERVER_FH_KEY]);
2266 if (fh_key_len != sizeof(siphash_key_t))
2267 return -EINVAL;
2268
2269 /* Is the key already set? */
2270 nn = net_generic(genl_info_net(info), nfsd_net_id);
2271 if (nn->fh_key)
2272 return -EEXIST;
2273
2274 fh_key = kmalloc(sizeof(siphash_key_t), GFP_KERNEL);
2275 if (!fh_key)
2276 return -ENOMEM;
2277
2278 memcpy(fh_key, nla_data(info->attrs[NFSD_A_SERVER_FH_KEY]), sizeof(siphash_key_t));
2279 nn = net_generic(genl_info_net(info), nfsd_net_id);
2280 nn->fh_key = fh_key;
2281
> 2282 trace_nfsd_ctl_fh_key_set((const char *)fh_key, ret);
2283 return ret;
2284 }
2285
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH 3/4 NFSD/export: Add sign_fh export option
In reply to Benjamin CoddingtonSetting the "sign_fh" export option sets NFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.
--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)
fs/nfsd/export.c +3 -2
@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };
include/uapi/linux/nfsd/export.h +2 -2
@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES BIT(5)#define NFSEXP_NOREADDIRPLUS BIT(6)#define NFSEXP_SECURITY_LABEL BIT(7) -/* BIT(8) currently unused */ +#define NFSEXP_SIGN_FH BIT(8)#define NFSEXP_NOHIDE BIT(9)#define NFSEXP_NOSUBTREECHECK BIT(10)#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
@@ -55,7 +55,7 @@#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1 +#define NFSEXP_ALLFLAGS BIT(18) - 1/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v1 3/4] NFSD/export: Add sign_fh export option
Quoted 2 lines · Benjamin Coddington
Setting the "
sign_fh" export option setsNFSEXP_SIGN_FH. In a future patch NFSD uses this signal to append a MAC onto filehandles for that export.
Same comment on missing "why" and the use of a Link: tag
Also, let's see some motivation for re-ordering the export flag table.
Quoted 53 lines
Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/export.c | 5 +++-- include/uapi/linux/nfsd/export.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)fs/nfsd/export.c +3
-2@@ -1349,13 +1349,14 @@ static struct flags { { NFSEXP_ASYNC, {"async", "sync"}}, { NFSEXP_GATHERED_WRITES, {"wdelay", "no_wdelay"}}, { NFSEXP_NOREADDIRPLUS, {"nordirplus", ""}}, + { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, + { NFSEXP_SIGN_FH, {"sign_fh", ""}}, { NFSEXP_NOHIDE, {"nohide", ""}}, - { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_NOSUBTREECHECK, {"no_subtree_check", ""}}, { NFSEXP_NOAUTHNLM, {"insecure_locks", ""}}, + { NFSEXP_CROSSMOUNT, {"crossmnt", ""}}, { NFSEXP_V4ROOT, {"v4root", ""}}, { NFSEXP_PNFS, {"pnfs", ""}}, - { NFSEXP_SECURITY_LABEL, {"security_label", ""}}, { 0, {"", ""}} };include/uapi/linux/nfsd/export.h +1
-1@@ -34,7 +34,7 @@#define NFSEXP_GATHERED_WRITES BIT(5)#define NFSEXP_NOREADDIRPLUS BIT(6)#define NFSEXP_SECURITY_LABEL BIT(7) -/* BIT(8) currently unused */ +#define NFSEXP_SIGN_FH BIT(8)#define NFSEXP_NOHIDE BIT(9)#define NFSEXP_NOSUBTREECHECK BIT(10)#define NFSEXP_NOAUTHNLM BIT(11)/* Don't authenticate NLM requests - just trust */
include/uapi/linux/nfsd/export.h +1
-1@@ -55,7 +55,7 @@#define NFSEXP_PNFS BIT(17)/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS BIT(18) - BIT(8) - 1 +#define NFSEXP_ALLFLAGS BIT(18) - 1/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \Signature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
PATCH 4/4 NFSD: Sign filehandles
In reply to Benjamin CoddingtonNFS clients may bypass restrictive directory permissions by using open_by_handle() (or other available OS system call) to guess the filehandles for files below that directory.
In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "sign_fh" export option. The filehandle's fh_auth_type is set to FH_AT_MAC(1) to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error _BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error _BADHANDLE.
--- fs/nfsd/nfs3xdr.c | 20 +++++++---- fs/nfsd/nfs4xdr.c | 12 ++++--- fs/nfsd/nfsfh.c | 72 ++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 22 ++++++++++++ include/linux/sunrpc/svc.h | 1 + 5 files changed, 113 insertions(+), 14 deletions(-)
fs/nfsd/nfs3xdr.c +13 -7
@@ -120,11 +120,16 @@ svcxdr_encode_nfsstat3(struct xdr_stream *xdr, __be32 status) }static bool -svcxdr_encode_nfs_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_nfs_fh3 (struct svc_rqst *rqstp,struct xdr_stream *xdr, +struct svc_fh *fhp) { -u32 size = fhp->fh_handle.fh_size; +u32 size;__be32 *p; +if (fh_append_mac (fhp,SVC_NET (rqstp))) +return false; + size = fhp->fh_handle.fh_size; + p =xdr_reserve_space (xdr,XDR_UNIT + size);if (!p)return false;
@@ -137,11 +142,12 @@ svcxdr_encode_nfs_fh3(struct xdr_stream *xdr, const struct svc_fh *fhp) }static bool -svcxdr_encode_post_op_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_post_op_fh3 (struct svc_rqst *rqstp,struct xdr_stream *xdr, +struct svc_fh *fhp) {if (xdr_stream_encode_item_present (xdr) <0 )return false; -if (!svcxdr_encode_nfs_fh3 (xdr, fhp)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, fhp))return false;return true;
@@ -772,7 +778,7 @@ nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_nfs_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))return false;
@@ -908,7 +914,7 @@ nfs3svc_encode_createres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_post_op_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_post_op_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))return false;
@@ -1117,7 +1123,7 @@ svcxdr_encode_entry3_plus(struct nfsd3_readdirres *resp, const char *name,if (!svcxdr_encode_post_op_attr (resp->rqstp, xdr, fhp)) goto out; -if (!svcxdr_encode_post_op_fh3 (xdr, fhp)) +if (!svcxdr_encode_post_op_fh3 (resp->rqstp, xdr, fhp)) goto out; result = true;
fs/nfsd/nfs4xdr.c +8 -4
@@ -2701,9 +2701,13 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp) }static __be32 nfsd4_encode_nfs_fh4 (struct xdr_stream *xdr, -struct knfsd_fh *fh_handle) +struct svc_fh *fhp) { -return nfsd4_encode_opaque (xdr, fh_handle->fh_raw, fh_handle->fh_size); +if (fh_append_mac (fhp,SVC_NET (RESSTRM_RQST (xdr)))) +return nfserr_resource; + +return nfsd4_encode_opaque (xdr, fhp->fh_handle.fh_raw, + fhp->fh_handle.fh_size); }/* This is a frequently-encoded type; open-coded for speed */
@@ -3359,7 +3363,7 @@ static __be32 nfsd4_encode_fattr4_acl(struct xdr_stream *xdr,static __be32 nfsd4_encode_fattr4_filehandle (struct xdr_stream *xdr,const struct nfsd4_fattr_args *args) { -return nfsd4_encode_nfs_fh4 (xdr, &args->fhp->fh_handle); +return nfsd4_encode_nfs_fh4 (xdr, args->fhp); } static __be32 nfsd4_encode_fattr4_fileid(struct xdr_stream *xdr,
@@ -4460,7 +4464,7 @@ nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr,struct svc_fh *fhp = u->getfh;/* object */ -return nfsd4_encode_nfs_fh4 (xdr, &fhp->fh_handle); +return nfsd4_encode_nfs_fh4 (xdr, fhp); } static __be32
fs/nfsd/nfsfh.c +69 -3
98 lines not on this page · raw message
fs/nfsd/nfsfh.h +22 -0
@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1 +static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];
@@ -226,6 +229,7 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat);__be32 fh_compose (struct svc_fh *,struct svc_export *,struct dentry *,struct svc_fh *);__be32 fh_update (struct svc_fh *);void fh_put (struct svc_fh *); +int fh_append_mac (struct svc_fh *,struct net *net);static __inline__struct svc_fh *fh_copy (struct svc_fh *dst,const struct svc_fh *src)
@@ -274,6 +278,24 @@ static inline bool fh_fsid_match(const struct knfsd_fh *fh1,return true; } +static inline size_t fh_fileid_offset (const struct knfsd_fh *fh) +{ +return key_len (fh->fh_fsid_type) +4 ; +} + +static inline size_t fh_fileid_len (const struct knfsd_fh *fh) +{ +switch (fh->fh_auth_type) { +case FH_AT_NONE : +return fh->fh_size -fh_fileid_offset (fh); +break ; +case FH_AT_MAC : +return fh->fh_size -8 -fh_fileid_offset (fh); +break ; + } +return 0 ; +} +/** * fh_want_write - Get write access to an export * @fhp: File handle of file to be written
include/linux/sunrpc/svc.h +1 -0
@@ -267,6 +267,7 @@ enum { };#define SVC_NET (rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net) +#define RESSTRM_RQST (xdr_stream) (container_of(xdr_stream, struct svc_rqst, rq_res_stream))/* * Rigorous type checking on sockaddr type conversions
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v1 4/4] NFSD: Sign filehandles
Quoted 38 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfs3xdr.c | 20 +++++++---- fs/nfsd/nfs4xdr.c | 12 ++++--- fs/nfsd/nfsfh.c | 72 ++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 22 ++++++++++++ include/linux/sunrpc/svc.h | 1 + 5 files changed, 113 insertions(+), 14 deletions(-)fs/nfsd/nfs3xdr.c +1
-1@@ -120,11 +120,16 @@ svcxdr_encode_nfsstat3(struct xdr_stream *xdr, __be32 status) } static bool -svcxdr_encode_nfs_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_nfs_fh3(struct svc_rqst *rqstp, struct xdr_stream *xdr,
fs/nfsd/nfs3xdr.c svcxdr_encode_nfsstat3(struct xdr_stream *xdr, · line 133
Quoted 10 lines
fs/nfsd/nfs3xdr.c +6
-1+ struct svc_fh *fhp) { -u32 size = fhp->fh_handle.fh_size; +u32 size;__be32 *p; +if (fh_append_mac (fhp,SVC_NET (rqstp))) +return false; + size = fhp->fh_handle.fh_size; +
This is a layering violation. XDR encoding never alters the content in the local data structures, and this will be impossible to convert to xdrgen down the line. All of the NFS version-specific code should not know or care about FH signing, IMO.
Why can't the new signing logic be contained in fh_compose() and fh_verify() ?
Quoted 94 lines
fs/nfsd/nfs3xdr.c +0
-0p =xdr_reserve_space (xdr,XDR_UNIT + size);if (!p)fs/nfsd/nfs3xdr.c +3
-2return false;@@ -137,11 +142,12 @@ svcxdr_encode_nfs_fh3(struct xdr_stream *xdr, const struct svc_fh *fhp) }static bool -svcxdr_encode_post_op_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_post_op_fh3 (struct svc_rqst *rqstp,struct xdr_stream *xdr, +struct svc_fh *fhp) {if (xdr_stream_encode_item_present (xdr) <0 )return false; -if (!svcxdr_encode_nfs_fh3 (xdr, fhp)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, fhp))return false;fs/nfsd/nfs3xdr.c +1
-1return true;@@ -772,7 +778,7 @@ nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_nfs_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))fs/nfsd/nfs3xdr.c +1
-1return false;@@ -908,7 +914,7 @@ nfs3svc_encode_createres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_post_op_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_post_op_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))fs/nfsd/nfs3xdr.c +1
-1return false;@@ -1117,7 +1123,7 @@ svcxdr_encode_entry3_plus(struct nfsd3_readdirres *resp, const char *name, if (!svcxdr_encode_post_op_attr(resp->rqstp, xdr, fhp)) goto out; -if (!svcxdr_encode_post_op_fh3 (xdr, fhp)) +if (!svcxdr_encode_post_op_fh3 (resp->rqstp, xdr, fhp)) goto out; result = true;fs/nfsd/nfs4xdr.c +7
-3@@ -2701,9 +2701,13 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp) }static __be32 nfsd4_encode_nfs_fh4 (struct xdr_stream *xdr, -struct knfsd_fh *fh_handle) +struct svc_fh *fhp) { -return nfsd4_encode_opaque (xdr, fh_handle->fh_raw, fh_handle->fh_size); +if (fh_append_mac (fhp,SVC_NET (RESSTRM_RQST (xdr)))) +return nfserr_resource; + +return nfsd4_encode_opaque (xdr, fhp->fh_handle.fh_raw, + fhp->fh_handle.fh_size); }/* This is a frequently-encoded type; open-coded for speed */ @@ -3359,7 +3363,7 @@ static __be32 nfsd4_encode_fattr4_acl(structxdr_stream *xdr, static __be32 nfsd4_encode_fattr4_filehandle(struct xdr_stream *xdr, const struct nfsd4_fattr_args *args) { - return nfsd4_encode_nfs_fh4(xdr, &args->fhp->fh_handle); + return nfsd4_encode_nfs_fh4(xdr, args->fhp); }static
__be32nfsd4_encode_fattr4_fileid(structxdr_stream*xdr,fs/nfsd/nfs4xdr.c +1
-1@@ -4460,7 +4464,7 @@ nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr, struct svc_fh *fhp = u->getfh;/* object */ -return nfsd4_encode_nfs_fh4 (xdr, &fhp->fh_handle); +return nfsd4_encode_nfs_fh4 (xdr, fhp); }static
__be32fs/nfsd/nfsfh.c +0
-0
fs/nfsd/nfsfh.c line 14
Quoted 4 lines
fs/nfsd/nfsfh.c +1
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h> #include <linux/sunrpc/svcauth_gss.h> +#include <crypto/skcipher.h>
Is this header still needed?
Quoted 14 lines
fs/nfsd/nfsfh.c +6
-0#include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,62 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Intended to be called when encoding, appends an 8-byte MAC +* to the filehandle hashed from the server's fh_key: +*/ +int fh_append_mac (struct svc_fh *fhp,struct net *net) +{
fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct · line 157
Quoted 10 lines
fs/nfsd/nfsfh.c +10
-0+struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { +pr_warn ("NFSD: unable to sign filehandles, fh_key not set.\n" );
Use pr_warn_ratelimited() instead
Quoted 8 lines
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn("NFSD: unable to sign filehandles, fh_size %lu would begreater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", fh->fh_size + sizeof(hash),
fhp->fh_maxsize);
fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 169
Quoted 7 lines
fs/nfsd/nfsfh.c +7
-0+return -EINVAL ; + } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash);
What prevents appending a MAC to the same FH multiple times?
Quoted 26 lines
fs/nfsd/nfsfh.c +20
-0+ +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn("NFSD: unable to verify signed filehandles, fh_key notset.\n");
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return memcmp(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,
Quoted 1 line
sizeof(hash));
Let's use crypto_memneq() for this purpose, to avoid timing attacks.
Quoted 70 lines
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +223,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +8
-2return error;@@ -237,9 +297,15 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +/* Root filehandle always unsigned because rpc.mountd has no key */ +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +0
-0nfsd_acceptable, exp);fs/nfsd/nfsfh.h +4
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1 +static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];@@ -226,6 +229,7 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat);__be32 fh_compose (struct svc_fh *,struct svc_export *,struct dentry *,struct svc_fh *);__be32 fh_update (struct svc_fh *);void fh_put (struct svc_fh *); +int fh_append_mac (struct svc_fh *,struct net *net);static
__inline__structsvc_fh*fh_copy(struct svc_fh *dst, const struct svc_fh *src)fs/nfsd/nfsfh.h +3
-0@@ -274,6 +278,24 @@ static inline bool fh_fsid_match(const structknfsd_fh *fh1, return true; } +static inline size_t fh_fileid_offset (const struct knfsd_fh *fh) +{ +return key_len (fh->fh_fsid_type) +4 ;
fs/nfsd/nfsfh.h static inline bool fh_fsid_match(const struct · line 294
Quoted 10 lines
fs/nfsd/nfsfh.h +10
-0+} + +static inline size_t fh_fileid_len (const struct knfsd_fh *fh) +{ +switch (fh->fh_auth_type) { +case FH_AT_NONE : +return fh->fh_size -fh_fileid_offset (fh); +break ; +case FH_AT_MAC : +return fh->fh_size -8 -fh_fileid_offset (fh);
Let's define/use symbolic constants here (8) and just above (4).
fs/nfsd/nfsfh.h static inline bool fh_fsid_match(const struct · line 295
Quoted 1 line
fs/nfsd/nfsfh.h +1
-0+break ;
The break; statements are unreachable.
Quoted 23 lines
fs/nfsd/nfsfh.h +4
-0+ } +return 0 ; +} +/** * fh_want_write - Get write access to an export * @fhp: File handle of file to be written
include/linux/sunrpc/svc.h +1
-0@@ -267,6 +267,7 @@ enum { };#define SVC_NET (rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net) +#define RESSTRM_RQST (xdr_stream) (container_of(xdr_stream, struct svc_rqst, rq_res_stream))/*
* Rigorous type checking on sockaddr type conversionsSignature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
Re: [PATCH v1 4/4] NFSD: Sign filehandles
Quoted 55 lines · Chuck Lever
Quoted 38 lines · Benjamin Coddington
NFS clients may bypass restrictive directory permissions by using
open_by_handle()(or other available OS system call) to guess the filehandles for files below that directory.In order to harden knfsd servers against this attack, create a method to sign and verify filehandles using siphash as a MAC (Message Authentication Code). Filehandles that have been signed cannot be tampered with, nor can clients reasonably guess correct filehandles and hashes that may exist in parts of the filesystem they cannot access due to directory permissions.
Append the 8 byte siphash to encoded filehandles for exports that have set the "
sign_fh" export option. The filehandle'sfh_auth_typeis set toFH_AT_MAC(1)to indicate the filehandle is signed. Filehandles received from clients are verified by comparing the appended hash to the expected hash. If the MAC does not match the server responds with NFS error_BADHANDLE. If unsigned filehandles are received for an export with "sign_fh" they are rejected with NFS error_BADHANDLE.Signed-off-by Benjamin Coddington <[email protected]>--- fs/nfsd/nfs3xdr.c | 20 +++++++---- fs/nfsd/nfs4xdr.c | 12 ++++--- fs/nfsd/nfsfh.c | 72 ++++++++++++++++++++++++++++++++++++-- fs/nfsd/nfsfh.h | 22 ++++++++++++ include/linux/sunrpc/svc.h | 1 + 5 files changed, 113 insertions(+), 14 deletions(-)fs/nfsd/nfs3xdr.c +1
-1@@ -120,11 +120,16 @@ svcxdr_encode_nfsstat3(struct xdr_stream *xdr, __be32 status) } static bool -svcxdr_encode_nfs_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_nfs_fh3(struct svc_rqst *rqstp, struct xdr_stream *xdr,fs/nfsd/nfs3xdr.c svcxdr_encode_nfsstat3(struct xdr_stream *xdr, · line 133
Quoted 10 lines
fs/nfsd/nfs3xdr.c +6
-1+ struct svc_fh *fhp) { -u32 size = fhp->fh_handle.fh_size; +u32 size;__be32 *p; +if (fh_append_mac (fhp,SVC_NET (rqstp))) +return false; + size = fhp->fh_handle.fh_size; +This is a layering violation. XDR encoding never alters the content in the local data structures, and this will be impossible to convert to xdrgen down the line. All of the NFS version-specific code should not know or care about FH signing, IMO.
Quoted 2 lines
Why can't the new signing logic be contained in
fh_compose()andfh_verify()?
It can - I originally had it there. I moved it after finding that fh_compose() creates a partial handle requiring fh_update() to fill it in after file creation, and IIRC there are a couple of other fh_match() gotcha's buried in the server when you're not signing last thing before the wire. Totally doable to move it, I'll work on that for next version - but I think it will mean that some other parts of the server will need to become "signed filehandle"-aware.
Quoted 96 lines
Quoted 96 lines
fs/nfsd/nfs3xdr.c +0
-0p =xdr_reserve_space (xdr,XDR_UNIT + size);if (!p)fs/nfsd/nfs3xdr.c +3
-2return false;@@ -137,11 +142,12 @@ svcxdr_encode_nfs_fh3(struct xdr_stream *xdr, const struct svc_fh *fhp) }static bool -svcxdr_encode_post_op_fh3 (struct xdr_stream *xdr,const struct svc_fh *fhp) +svcxdr_encode_post_op_fh3 (struct svc_rqst *rqstp,struct xdr_stream *xdr, +struct svc_fh *fhp) {if (xdr_stream_encode_item_present (xdr) <0 )return false; -if (!svcxdr_encode_nfs_fh3 (xdr, fhp)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, fhp))return false;fs/nfsd/nfs3xdr.c +1
-1return true;@@ -772,7 +778,7 @@ nfs3svc_encode_lookupres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_nfs_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_nfs_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))fs/nfsd/nfs3xdr.c +1
-1return false;@@ -908,7 +914,7 @@ nfs3svc_encode_createres(struct svc_rqst *rqstp, struct xdr_stream *xdr)return false;switch (resp->status) {case nfs_ok: -if (!svcxdr_encode_post_op_fh3 (xdr, &resp->fh)) +if (!svcxdr_encode_post_op_fh3 (rqstp, xdr, &resp->fh))return false;if (!svcxdr_encode_post_op_attr (rqstp, xdr, &resp->fh))fs/nfsd/nfs3xdr.c +1
-1return false;@@ -1117,7 +1123,7 @@ svcxdr_encode_entry3_plus(struct nfsd3_readdirres *resp, const char *name, if (!svcxdr_encode_post_op_attr(resp->rqstp, xdr, fhp)) goto out; -if (!svcxdr_encode_post_op_fh3 (xdr, fhp)) +if (!svcxdr_encode_post_op_fh3 (resp->rqstp, xdr, fhp)) goto out; result = true;fs/nfsd/nfs4xdr.c +7
-3@@ -2701,9 +2701,13 @@ nfsd4_decode_compound(struct nfsd4_compoundargs *argp) }static __be32 nfsd4_encode_nfs_fh4 (struct xdr_stream *xdr, -struct knfsd_fh *fh_handle) +struct svc_fh *fhp) { -return nfsd4_encode_opaque (xdr, fh_handle->fh_raw, fh_handle->fh_size); +if (fh_append_mac (fhp,SVC_NET (RESSTRM_RQST (xdr)))) +return nfserr_resource; + +return nfsd4_encode_opaque (xdr, fhp->fh_handle.fh_raw, + fhp->fh_handle.fh_size); }/* This is a frequently-encoded type; open-coded for speed */ @@ -3359,7 +3363,7 @@ static __be32 nfsd4_encode_fattr4_acl(structxdr_stream *xdr, static __be32 nfsd4_encode_fattr4_filehandle(struct xdr_stream *xdr, const struct nfsd4_fattr_args *args) { - return nfsd4_encode_nfs_fh4(xdr, &args->fhp->fh_handle); + return nfsd4_encode_nfs_fh4(xdr, args->fhp); }static
__be32nfsd4_encode_fattr4_fileid(structxdr_stream*xdr,fs/nfsd/nfs4xdr.c +1
-1@@ -4460,7 +4464,7 @@ nfsd4_encode_getfh(struct nfsd4_compoundres *resp, __be32 nfserr, struct svc_fh *fhp = u->getfh;/* object */ -return nfsd4_encode_nfs_fh4 (xdr, &fhp->fh_handle); +return nfsd4_encode_nfs_fh4 (xdr, fhp); }static
__be32fs/nfsd/nfsfh.c +0
-0@@ -11,6 +11,7 @@#include <linux/exportfs.h>
Quoted 4 lines
fs/nfsd/nfsfh.c line 14
Quoted 2 lines
fs/nfsd/nfsfh.c +1
-0#include <linux/sunrpc/svcauth_gss.h> +#include <crypto/skcipher.h> Is this header still needed?
Nope.
Quoted 18 lines
Quoted 18 lines
fs/nfsd/nfsfh.c +10
-0#include "nfsd.h" #include "vfs.h" #include "auth.h" @@ -137,6 +138,62 @@ static inline __be32 check_pseudo_root(structdentry *dentry, return nfs_ok; } +/* +* Intended to be called when encoding, appends an 8-byte MAC +* to the filehandle hashed from the server's fh_key: +*/ +int fh_append_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash;
Quoted 8 lines
fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 156
Quoted 6 lines
fs/nfsd/nfsfh.c +6
-0+ +if (!(fhp->fh_export->ex_flags &NFSEXP_SIGN_FH )) +return 0 ; + +if (!fh_key) { +pr_warn ("NFSD: unable to sign filehandles, fh_key not set.\n" );Use
pr_warn_ratelimited()instead
Yessir!
Quoted 9 lines
Quoted 9 lines
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + +if (fh->fh_size +sizeof (hash) > fhp->fh_maxsize) { + pr_warn("NFSD: unable to sign filehandles, fh_size %lu would begreater"
fs/nfsd/nfsfh.c +1
-0+ " than fh_maxsize %d.\n", fh->fh_size + sizeof(hash),
fhp->fh_maxsize);fs/nfsd/nfsfh.c +1
-0+return -EINVAL ;
Quoted 8 lines
fs/nfsd/nfsfh.c static inline __be32 check_pseudo_root(struct dentry *dentry, · line 169
Quoted 6 lines
fs/nfsd/nfsfh.c +6
-0+ } + + fh->fh_auth_type =FH_AT_MAC ; + hash =siphash (&fh->fh_raw, fh->fh_size, fh_key); +memcpy (&fh->fh_raw[fh->fh_size], &hash,sizeof (hash)); + fh->fh_size +=sizeof (hash);What prevents appending a MAC to the same FH multiple times?
The fact that it's the last thing that happens during encoding. We're going to change this now, so it will need to be guarded against. Can use a check of fh_auth_type.
Quoted 21 lines
Quoted 21 lines
fs/nfsd/nfsfh.c +20
-0+ +return 0 ; +} + +/* +* Verify that the the filehandle's MAC was hashed from this filehandle +* given the server's fh_key: +*/ +static int fh_verify_mac (struct svc_fh *fhp,struct net *net) +{ +struct nfsd_net *nn =net_generic (net, nfsd_net_id); +struct knfsd_fh *fh = &fhp->fh_handle; +siphash_key_t *fh_key = nn->fh_key; +u64 hash; + +if (fhp->fh_handle.fh_auth_type !=FH_AT_MAC ) +return -EINVAL ; + +if (!fh_key) { + pr_warn("NFSD: unable to verify signed filehandles, fh_key notset.\n");
Quoted 8 lines
Quoted 6 lines
fs/nfsd/nfsfh.c +5
-0+return -EINVAL ; + } + + hash =siphash (&fh->fh_raw, fh->fh_size -sizeof (hash), fh_key); + return memcmp(&fh->fh_raw[fh->fh_size - sizeof(hash)], &hash,sizeof(hash));
Let's use
crypto_memneq()for this purpose, to avoid timing attacks.
Ok.
Quoted 67 lines
Quoted 67 lines
fs/nfsd/nfsfh.c +6
-1+} +/* * Use the given filehandle to look up the corresponding export and * dentry. On success, the results are used to set fh_export and @@ -166,8 +223,11 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, if (--data_left < 0)return error; -if (fh->fh_auth_type !=0 ) + +/* either FH_AT_NONE or FH_AT_MAC */ +if (fh->fh_auth_type >1 )return error; + len =key_len (fh->fh_fsid_type) /4 ;if (len ==0 )fs/nfsd/nfsfh.c +8
-2return error;@@ -237,9 +297,15 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct net *net, fileid_type = fh->fh_fileid_type; -if (fileid_type ==FILEID_ROOT ) +if (fileid_type ==FILEID_ROOT ) { dentry =dget (exp->ex_path.dentry); -else { + }else { +/* Root filehandle always unsigned because rpc.mountd has no key */ +if (exp->ex_flags &NFSEXP_SIGN_FH &&fh_verify_mac (fhp, net)) { +trace_nfsd_set_fh_dentry_badhandle (rqstp, fhp, -EKEYREJECTED ); + goto out; + } + dentry = exportfs_decode_fh_raw(exp->ex_path.mnt, fid, data_left, fileid_type, 0,fs/nfsd/nfsfh.c +0
-0nfsd_acceptable, exp);fs/nfsd/nfsfh.h +4
-0@@ -59,6 +59,9 @@ struct knfsd_fh {#define fh_fsid_type fh_raw[2]#define fh_fileid_type fh_raw[3] +#define FH_AT_NONE 0 +#define FH_AT_MAC 1 +static inline u32 *fh_fsid (const struct knfsd_fh *fh) {return (u32 *)&fh->fh_raw[4 ];@@ -226,6 +229,7 @@ __be32 fh_getattr(const struct svc_fh *fhp, struct kstat *stat);__be32 fh_compose (struct svc_fh *,struct svc_export *,struct dentry *,struct svc_fh *);__be32 fh_update (struct svc_fh *);void fh_put (struct svc_fh *); +int fh_append_mac (struct svc_fh *,struct net *net);static
__inline__structsvc_fh*fh_copy(struct svc_fh *dst, const struct svc_fh *src)fs/nfsd/nfsfh.h +0
-0@@ -274,6 +278,24 @@ static inline bool fh_fsid_match(const structknfsd_fh *fh1, return true; }
Quoted 15 lines
fs/nfsd/nfsfh.h static inline bool fh_fsid_match(const struct knfsd_fh *fh1, · line 293
Quoted 13 lines
fs/nfsd/nfsfh.h +13
-0+static inline size_t fh_fileid_offset (const struct knfsd_fh *fh) +{ +return key_len (fh->fh_fsid_type) +4 ; +} + +static inline size_t fh_fileid_len (const struct knfsd_fh *fh) +{ +switch (fh->fh_auth_type) { +case FH_AT_NONE : +return fh->fh_size -fh_fileid_offset (fh); +break ; +case FH_AT_MAC : +return fh->fh_size -8 -fh_fileid_offset (fh);Let's define/use symbolic constants here (8) and just above (4).
Actually, think I can drop this hunk now - its no longer used after a refactor.
Quoted 3 lines
fs/nfsd/nfsfh.h static inline bool fh_fsid_match(const struct · line 295
Quoted 1 line
fs/nfsd/nfsfh.h +1
-0+break ;The break; statements are unreachable.
Yep. Thanks Chuck!
Ben
kernel test robot
build botRe: [PATCH v1 4/4] NFSD: Sign filehandles
In reply to Benjamin Coddingtonkernel test robot noticed the following build warnings:
Hi Benjamin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bfd453acb5637b5df881cef4b21803344aa9e7ac]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Coddington/nfsd-Convert-export-flags-to-use-BIT-macro/20260116-223927 base: bfd453acb5637b5df881cef4b21803344aa9e7ac patch link: https://lore.kernel.org/r/9c9cd6131574a45f2603c9248ba205a79b00fea3.1768573690.git.bcodding%40hammerspace.com patch subject: [PATCH v1 4/4] NFSD: Sign filehandles config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20260117/[email protected]/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260117/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> fs/nfsd/nfsfh.c:162:30: warning: format specifies type 'unsigned long' but the argument has type 'unsigned int' [-Wformat]
161 | pr_warn("NFSD: unable to sign filehandles, fh_size %lu would be greater"
| ~~~
| %u
162 | " than fh_maxsize %d.\n", fh->fh_size + sizeof(hash), fhp->fh_maxsize);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:565:37: note: expanded from macro 'pr_warn'
565 | printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:512:60: note: expanded from macro 'printk'
512 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
| ~~~ ^~~~~~~~~~~
include/linux/printk.h:484:19: note: expanded from macro 'printk_index_wrap'
484 | _p_func(_fmt, ##__VA_ARGS__); \
| ~~~~ ^~~~~~~~~~~1 warning generated.
vim +162 fs/nfsd/nfsfh.c
140
141 /*
142 * Intended to be called when encoding, appends an 8-byte MAC
143 * to the filehandle hashed from the server's fh_key:
144 */
145 int fh_append_mac(struct svc_fh *fhp, struct net *net)
146 {
147 struct nfsd_net *nn = net_generic(net, nfsd_net_id);
148 struct knfsd_fh *fh = &fhp->fh_handle;
149 siphash_key_t *fh_key = nn->fh_key;
150 u64 hash;
151
152 if (!(fhp->fh_export->ex_flags & NFSEXP_SIGN_FH))
153 return 0;
154
155 if (!fh_key) {
156 pr_warn("NFSD: unable to sign filehandles, fh_key not set.\n");
157 return -EINVAL;
158 }
159
160 if (fh->fh_size + sizeof(hash) > fhp->fh_maxsize) {
161 pr_warn("NFSD: unable to sign filehandles, fh_size %lu would be greater"
> 162 " than fh_maxsize %d.\n", fh->fh_size + sizeof(hash), fhp->fh_maxsize);
163 return -EINVAL;
164 }
165
166 fh->fh_auth_type = FH_AT_MAC;
167 hash = siphash(&fh->fh_raw, fh->fh_size, fh_key);
168 memcpy(&fh->fh_raw[fh->fh_size], &hash, sizeof(hash));
169 fh->fh_size += sizeof(hash);
170
171 return 0;
172 }
173
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Chuck Lever
maintainerQuoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
"open, read, or modify the contents of the file"
Btw, referring to "open" here is a little confusing, since NFSv3 does not have an on-the-wire OPEN operation. I'm not sure how to clarify.
Quoted 7 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Instead: "an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP."
Quoted 20 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Quoted 10 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
OK, I guess this is where I got the idea this would be an export option.
But I'm unconvinced that this provides any real security. There are other ways of obtaining a filehandle besides guessing, and nothing here suggests that guessing is the premier attack methodology.
The fundamental issue is there is no authorization check done by NFS READ or WRITE. And in the case of root_squash with AUTH_SYS, maybe even an authorization check at I/O time isn't enough. Note this is the classic NFS AUTH_SYS security model; it assumes we're all best of friends.
Quoted 5 lines
Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
There are some problems here.
- The requirement is: File handles must remain stable while the inode generation number remains unchanged (Chinner).
- There's nothing in your implementation that prevents user space from providing a different key after a system reboot or server restart. In fact, destroying the net namespace removes the ability for the server to remember (and thus check) the previously set fh_key.
- An fh_key change is safe to do once all exported file systems are unexported and unmounted. NFS clients generally don't preserve filehandles after they unmount file systems. I say this because I think rekeying might become important as the time to break a key decreases.
Quoted 13 lines
I had a previous attempt to solve this problem by encrypting filehandles,
which turned out to be a problematic, poor solution. The discussion on that previous attempt is here:
https://lore.kernel.org/linux-nfs/[email protected]/T/#tThere are some changes from that version:
- sign filehandles instead of encrypt them (Eric Biggers)
- fix theNFSEXP_macros, specificallyNFSEXP_ALLFLAGS(NeilBrown)
- rebase onto cel/nfsd-next (Chuck Lever)
- condensed/clarified problem explantion (thanks Chuck Lever)
- add nfsctl file "fh_key" for rpc.nfsd to also set the key
Quoted 6 lines
I plan on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "
sign_fh" is requested.
Noted, and we might require all that to be in place before merging.
Quoted 28 lines
Thanks for any comments and critique.
Benjamin Coddington (4):
nfsd: Convert export flags to useBIT()macro
nfsd: Add a key for signing filehandles
NFSD/export: Addsign_fhexport option
NFSD: Sign filehandlesDocumentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfs3xdr.c | 20 +++--- fs/nfsd/nfs4xdr.c | 12 ++-- fs/nfsd/nfsctl.c | 87 ++++++++++++++++++++++++++- fs/nfsd/nfsfh.c | 72 +++++++++++++++++++++- fs/nfsd/nfsfh.h | 22 +++++++ fs/nfsd/trace.h | 19 ++++++ include/linux/sunrpc/svc.h | 1 + include/uapi/linux/nfsd/export.h | 38 ++++++------ include/uapi/linux/nfsd_netlink.h | 2 + 14 files changed, 272 insertions(+), 36 deletions(-)base-commit:
bfd453acb5637b5df881cef4b21803344aa9e7acSignature · 1 line
2.50.1
Signature · 1 line
Chuck Lever
Quoted 62 lines · Chuck Lever
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle."open, read, or modify the contents of the file"
Btw, referring to "open" here is a little confusing, since NFSv3 does not have an on-the-wire OPEN operation. I'm not sure how to clarify.
Quoted 7 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Instead: "an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP."
Quoted 20 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.Quoted 10 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
Quoted 5 lines
OK, I guess this is where I got the idea this would be an export option.
But I'm unconvinced that this provides any real security. There are other ways of obtaining a filehandle besides guessing, and nothing here suggests that guessing is the premier attack methodology.
Help me understand you - you're unconvinced that having the server sign filehandles and verify filehandles prevents clients from fabricating valid ones?
Quoted 5 lines
The fundamental issue is there is no authorization check done by NFS READ or WRITE. And in the case of
root_squashwithAUTH_SYS, maybe even an authorization check at I/O time isn't enough. Note this is the classic NFSAUTH_SYSsecurity model; it assumes we're all best of friends.
I'm not quite following, READ and WRITE will have their filehandles validated.. the case you might be talking about is when a file moves or its access changes after the client has the valid filehandle. I'm not attempting to protect against those cases.
Quoted 8 lines
Quoted 5 lines
Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
There are some problems here.
Quoted 8 lines
- The requirement is: File handles must remain stable while the inode generation number remains unchanged (Chinner).
- There's nothing in your implementation that prevents user space from providing a different key after a system reboot or server restart. In fact, destroying the net namespace removes the ability for the server to remember (and thus check) the previously set
fh_key.
Yes - suggestions on how to solve this are welcome.
Quoted 5 lines
- An
fh_keychange is safe to do once all exported file systems are unexported and unmounted. NFS clients generally don't preserve filehandles after they unmount file systems. I say this because I think rekeying might become important as the time to break a key decreases.
Absolutely.
Ben
Chuck Lever
maintainerQuoted 53 lines · Benjamin Coddington
Quoted 51 lines · Chuck Lever
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle."open, read, or modify the contents of the file"
Btw, referring to "open" here is a little confusing, since NFSv3 does not have an on-the-wire OPEN operation. I'm not sure how to clarify.
Quoted 7 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Instead: "an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP."
Quoted 20 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access.
Btw, "allow ... clients to open by filehandle" is another confusion.
NFSv4 OPEN does do access checking and authorization.
Again, it's NFS READ and WRITE that are not blocked.
NFSv3 READ and WRITE do an intrinsic open.
NFSv4 READ and WRITE permit the use of a special stateid so that an OPEN isn't necessary to do the I/O (IIRC).
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Therefore additional
protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.OK, I guess this is where I got the idea this would be an export option.
Quoted 7 lines
Quoted 3 lines
But I'm unconvinced that this provides any real security. There are other ways of obtaining a filehandle besides guessing, and nothing here suggests that guessing is the premier attack methodology.
Help me understand you - you're unconvinced that having the server sign filehandles and verify filehandles prevents clients from fabricating valid ones?
The rationale provided here doesn't convince me that fabrication is the biggest threat and will give us the biggest bang for our buck if it is mitigated.
In order to carry out this attack, the attacker has to have access to the filehandles on an NFS client to examine them. She has to have access to a valid client IP address to send NFS requests from. Maybe you can bridge the gap by explaining how a /non-root/ user on an NFS client might leverage FH fabrication to gain access to another user's files. I think only the root user has this ability.
I've also tried to convince myself that cryptographic FH validation could mitigate misdirected WRITEs or READs. An attacker could replace the FH in a valid NFS request, for example, or a client might send garbage in an FH. I'm not sure those are real problems, though.
I am keeping in mind that everything here is using AUTH_SYS anyway, so maybe I'm just an old man yelling at a cloud. Nothing here is blocking yet, but I want the feature to provide meaningful value (as I think you do).
Thank you for bearing with me.
Signature · 1 line
Chuck Lever
Trond Myklebust
maintainerQuoted 154 lines · Chuck Lever
Quoted 78 lines · Benjamin Coddington
Quoted 76 lines · Chuck Lever
Quoted 8 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message
Authentication Code (MAC) to the filehandles it gives to clients. This
provides additional protection to the exported filesystem against filehandle
guessing attacks.Quoted 9 lines
Filesystems generate their own filehandles through the
export_operation
"encode_fh" and a filehandle provides sufficient access to open a file
without needing to perform a lookup. An NFS client holding a valid
filehandle can remotely open and read the contents of the file referred to
by the filehandle."open, read, or modify the contents of the file"
Btw, referring to "open" here is a little confusing, since NFSv3 does
not have an on-the-wire OPEN operation. I'm not sure how to clarify.Quoted 5 lines
In order to acquire a filehandle, you must perform lookup operations on the
parent directory(ies), and the permissions on those directories may
prohibit you from walking into them to find the files within.Quoted 8 lines
This would
normally be considered sufficient protection on a local filesystem to
prohibit users from accessing those files, however when the filesystem is
exported via NFS those files can still be accessed by guessing the correct,
valid filehandles.Instead: "an exported file can be accessed whenever the NFS server is
presented with the correct filehandle, which can be guessed or acquired
by means other than LOOKUP."Quoted 28 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's
an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^
^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation
number. Because the handle is well-formed, its easy to fabricate
filehandles that match other files within the same filesystem. You can
simply insert inode numbers and iterate on the generation number.
Eventually you'll be able to access the file usingopen_by_handle_at(2).
For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which
protects against guessing attacks by unprivileged users.Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
In contrast to a local user using
open_by_handle(2), the NFS server must
permissively allow remote clients to open by filehandle without being able
to check or trust the remote caller's access.Btw, "allow ... clients to open by filehandle" is another confusion.
NFSv4 OPEN does do access checking and authorization.
Again, it's NFS READ and WRITE that are not blocked.
NFSv3 READ and WRITE do an intrinsic open.
NFSv4 READ and WRITE permit the use of a special stateid so that an OPEN
isn't necessary to do the I/O (IIRC).Quoted 24 lines
Quoted 15 lines
Quoted 6 lines
Therefore additional
protection against this attack is needed for NFS case. We propose to sign
filehandles by appending an 8-byte MAC which is the siphash of the
filehandle from a key set from the nfs-utilities. NFS serverQuoted 8 lines
can then
ensure that guessing a valid filehandle+MAC is practically impossible
without knowledge of the MAC's key. The NFS server performs optional
signing by possessing a key set from userspace and having the "sign_fh"
export option.Quoted 8 lines
OK, I guess this is where I got the idea this would be an export option.
But I'm unconvinced that this provides any real security. There are
other ways of obtaining a filehandle besides guessing, and nothing
here suggests that guessing is the premier attack methodology.Quoted 5 lines
Help me understand you - you're unconvinced that having the server sign
filehandles and verify filehandles prevents clients from fabricating valid
ones?The rationale provided here doesn't convince me that fabrication is the
biggest threat and will give us the biggest bang for our buck if it is
mitigated.In order to carry out this attack, the attacker has to have access to the filehandles on an NFS client to examine them. She has to have access to a valid client IP address to send NFS requests from. Maybe you can bridge the gap by explaining how a /non-root/ user on an NFS client might leverage FH fabrication to gain access to another user's files. I think only the root user has this ability.
I've also tried to convince myself that cryptographic FH validation could mitigate misdirected WRITEs or READs. An attacker could replace the FH in a valid NFS request, for example, or a client might send garbage in an FH. I'm not sure those are real problems, though.
I am keeping in mind that everything here is using
AUTH_SYSanyway, so
maybe I'm just an old man yelling at a cloud. Nothing here is blocking
yet, but I want the feature to provide meaningful value (as I think you
do).
Quoted 2 lines
Thank you for bearing with me.
Yes, there may be a root squash option that prevents synthesis of root credentials, but that's not necessarily an impediment:
We assume that the attacker does have access to a userspace NFS client on the authorised client and that they can synthesize calls. If so, then GETATTR is unprivileged and will happily give up knowledge of which owner/group combination to synthesize in an OPEN, READ or WRITE call.
Signature · 3 lines
Trond Myklebust Linux NFS client maintainer, Hammerspace [email protected], [email protected]
Quoted 58 lines · Chuck Lever
Quoted 53 lines · Benjamin Coddington
Quoted 51 lines · Chuck Lever
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle."open, read, or modify the contents of the file"
Btw, referring to "open" here is a little confusing, since NFSv3 does not have an on-the-wire OPEN operation. I'm not sure how to clarify.
Quoted 7 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Instead: "an exported file can be accessed whenever the NFS server is presented with the correct filehandle, which can be guessed or acquired by means other than LOOKUP."
Quoted 20 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access.
Quoted 5 lines
Btw, "allow ... clients to open by filehandle" is another confusion.
NFSv4 OPEN does do access checking and authorization.
Again, it's NFS READ and WRITE that are not blocked.
Ok - I will try to be more precise next time.. something like:
.. the NFS server allows operations against the file ..
Quoted 22 lines
NFSv3 READ and WRITE do an intrinsic open.
NFSv4 READ and WRITE permit the use of a special stateid so that an OPEN isn't necessary to do the I/O (IIRC).
Quoted 15 lines
Quoted 14 lines
Quoted 8 lines
Therefore additional
protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.OK, I guess this is where I got the idea this would be an export option.
But I'm unconvinced that this provides any real security. There are other ways of obtaining a filehandle besides guessing, and nothing here suggests that guessing is the premier attack methodology.
Quoted 7 lines
Quoted 3 lines
Help me understand you - you're unconvinced that having the server sign filehandles and verify filehandles prevents clients from fabricating valid ones?
The rationale provided here doesn't convince me that fabrication is the biggest threat and will give us the biggest bang for our buck if it is mitigated.
Ahh - ok. I'm not trying to be convincing that its the biggest threat. For us, its a threat that doesn't have any counter measure yet. For snooping filehandles, we can guard against that with transport encryption (two options there). We're assuming that network security is a solved problem.
Quoted 2 lines
In order to carry out this attack, the attacker has to have access to the filehandles on an NFS client to examine them.
Maybe - you can also fab them up from reading code or do it in the lab. Maybe I should stop saying "after looking, easy to fab", and instead say "easy to fab".
Quoted 4 lines
She has to have access to a valid client IP address to send NFS requests from. Maybe you can bridge the gap by explaining how a /non-root/ user on an NFS client might leverage FH fabrication to gain access to another user's files. I think only the root user has this ability.
Let's not assume we have a perfect linux client. We don't know what the client implementation allows, or what bugs might be present there, or if that implementation has been compromised in some way. Whatever the client is, we assume it might be able to send guessed filehandles because the client implementation is out of the server's control and scope of trust.
That said, let's say we /do/ have a linux client - there's no reason to say it can't be the root user there where root is squashed to that client, but the root user can still use the path_to_filehandle_at() interfaces (as root after all) to discover some filehandles and try to open by handle.
Quoted 4 lines
I've also tried to convince myself that cryptographic FH validation could mitigate misdirected WRITEs or READs. An attacker could replace the FH in a valid NFS request, for example, or a client might send garbage in an FH. I'm not sure those are real problems, though.
For what we're trying to fix, those aren't in scope. This work really isn't interested in protecting against modifications on the wire or intercepting other client's traffic on the wire and using that information.
Quoted 6 lines
I am keeping in mind that everything here is using
AUTH_SYSanyway, so maybe I'm just an old man yelling at a cloud. Nothing here is blocking yet, but I want the feature to provide meaningful value (as I think you do).Thank you for bearing with me.
Of course, I very much appreciate the improvements only these iterations can produce. Thanks very much, Chuck.
Ben
Re: [PATCH v1 4/4] NFSD: Sign filehandles
In reply to Benjamin CoddingtonReview
Quoted 7 lines · Benjamin Coddington
(quoted diff) +3
-2- if (fileid_type == FILEID_ROOT) + if (fileid_type == FILEID_ROOT) { dentry = dget(exp->ex_path.dentry); - else { + } else { + /* Root filehandle always unsigned because rpc.mountd has no key */
I don't think this is correct.
rpc.mountd always asks the kernel for a filehandle, so it doesn't need a key.
However signing the root filehandle would be pointless as the client can "PUT_ROOTFH" without needing to provide a signature.
So I'm happy with the root not being signed, I'm not happy with the justification.
NeilBrown
Re: [PATCH v1 4/4] NFSD: Sign filehandles
Quoted 13 lines · NeilBrown
Review
Quoted 7 lines · Benjamin Coddington
(quoted diff) +3
-2- if (fileid_type == FILEID_ROOT) + if (fileid_type == FILEID_ROOT) { dentry = dget(exp->ex_path.dentry); - else { + } else { + /* Root filehandle always unsigned because rpc.mountd has no key */I don't think this is correct.
rpc.mountd always asks the kernel for a filehandle, so it doesn't need a key.
Quoted 4 lines
However signing the root filehandle would be pointless as the client can "
PUT_ROOTFH" without needing to provide a signature.
So I'm happy with the root not being signed, I'm not happy with the justification.
ah yes - I will just drop the comment.
Ben
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
A *trusted* NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.
I think that last part is key to understanding what you are trying to do. You are trying to enforce path-based restriction in NFS. And due to the various ways that a path and be traversed - e.g. different principles for different components, or some traversal on server, some on client - this effectively means you need a capability framework.
So you are enhancing the filehandle to act as a capability by adding a MAC.
Quoted 21 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^
Quoted 7 lines
This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Iterating a 32 bit generation number would be expected to take a long time to succeed - except that they tend to cluster early. Though in your example the msb is 1!
Do you have exploit code which demonstrates unauthorised access to a given inode number? What runtime? Could attack-detection in the server be a simple effective counter-measure? Should we do that anyway?
Supporting and encouraging the use of 64-bit generation numbers, and starting at a random offset would do a lot to make guessing harder. A crypto key could be part of making this number hard to guess.
Quoted 4 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access.
I find the above sufficiently vague and confusing that I cannot tell if I agree...
open_by_handle_at(2) requires CAP_DAC_READ_SEARCH precisely because no path-based restrictions are imposed.
The NFS server cannot require CAP_DAC_READ_SEARCH and assumes the trusted client performs the necessary path-based access checks.
When the client cannot be completely trusted, and path-based access controls are depended on, extra support is needed for NFS.
Quoted 8 lines
Therefore additional
protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
NeilBrown
Quoted 12 lines · NeilBrown
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
Quoted 3 lines
A *trusted* NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.
Mind if I use your words next time? I'm thinking that most of this cover-letter should end up in the docs.
Quoted 8 lines
I think that last part is key to understanding what you are trying to do. You are trying to enforce path-based restriction in NFS. And due to the various ways that a path and be traversed - e.g. different principles for different components, or some traversal on server, some on client - this effectively means you need a capability framework.
So you are enhancing the filehandle to act as a capability by adding a MAC.
Yes, that's a great way to think about it.
Quoted 28 lines
Quoted 20 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^Quoted 7 lines
This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Quoted 3 lines
Iterating a 32 bit generation number would be expected to take a long time to succeed - except that they tend to cluster early. Though in your example the msb is 1!
Trond posited that with a 1ms round-trip and 50 parallel GETATTRs it only takes one day.
Quoted 3 lines
Do you have exploit code which demonstrates unauthorised access to a given inode number? What runtime? Could attack-detection in the server be a simple effective counter-measure? Should we do that anyway?
Yes, its a modification of t_open_by_handle_at.c example program in the open_by_handle_at(2) man page. On my single system NFS client and server with a local mount, I averaged 16usec per open, and discovered my target filehandle in less than an hour. I didn't have any network latency to worry about, but I think it still shows its possible and a determined attacker can do it.
The server could be modified to notice elevated counts of error returns for
a client and then try to notify about it. But, I don't think it will be
simple - I imagine it would need a lot of tunable (how many failed fh, at what rate.. etc) because you need to tune the system to make a signal from the noise of regular operations and returns. That tuning can be worked around by a very determined attacker. You end up in a behavior detection/modification feedback loop and the server's not guaranteed to catch everything. Still it would be another layer of defense-in-depth that would have value.
Quoted 12 lines
Supporting and encouraging the use of 64-bit generation numbers, and starting at a random offset would do a lot to make guessing harder. A crypto key could be part of making this number hard to guess.
Quoted 4 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access.I find the above sufficiently vague and confusing that I cannot tell if I agree...
Quoted 6 lines
open_by_handle_at(2)requiresCAP_DAC_READ_SEARCHprecisely because no path-based restrictions are imposed.
The NFS server cannot requireCAP_DAC_READ_SEARCHand assumes the trusted client performs the necessary path-based access checks.
When the client cannot be completely trusted, and path-based access controls are depended on, extra support is needed for NFS.
Much better than mine, I'll use it. Thanks for looking Neil.
Ben
Quoted 11 lines
Quoted 8 lines
Therefore additional
protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.NeilBrown
Quoted 14 lines · Benjamin Coddington
Quoted 12 lines · NeilBrown
Quoted 10 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
Quoted 6 lines
Quoted 3 lines
A *trusted* NFS client holding a valid filehandle can remotely access the corresponding file without reference to access-path restrictions that might be imposed by the ancestor directories or the server exports.
Mind if I use your words next time? I'm thinking that most of this cover-letter should end up in the docs.
Please do!
Quoted 12 lines
Quoted 4 lines
Iterating a 32 bit generation number would be expected to take a long time to succeed - except that they tend to cluster early. Though in your example the msb is 1!
Trond posited that with a 1ms round-trip and 50 parallel GETATTRs it only takes one day.
Quoted 3 lines
Do you have exploit code which demonstrates unauthorised access to a given inode number? What runtime? Could attack-detection in the server be a simple effective counter-measure? Should we do that anyway?
Quoted 6 lines
Yes, its a modification of
t_open_by_handle_at.cexample program in theopen_by_handle_at(2)man page. On my single system NFS client and server with a local mount, I averaged 16usec per open, and discovered my target filehandle in less than an hour. I didn't have any network latency to worry about, but I think it still shows its possible and a determined attacker can do it.
This information would be useful to include in the cover letter/documentation.
Quoted 10 lines
The server could be modified to notice elevated counts of error returns for
a client and then try to notify about it. But, I don't think it will be
simple - I imagine it would need a lot of tunable (how many failed fh, at what rate.. etc) because you need to tune the system to make a signal from the noise of regular operations and returns. That tuning can be worked around by a very determined attacker. You end up in a behavior detection/modification feedback loop and the server's not guaranteed to catch everything. Still it would be another layer of defense-in-depth that would have value.
I would like to explore this, at least for the defense-in-depth rationale. Maybe it would also supply some protection wehn sign_fh isn't enabled.
We would need to monitor the result of exportfs_decode_fh_raw() for stale vs non-stale, and if the proportion of stale filehandles (on a given export) exceeds some low threshold (1%?) over a modest time period (5 minutes?) then .... what?
I don't think a hard fail would be a good idea, but maybe serialise future requests from that auth_domain and impose a delay on any stale filehandles until the proportion drops below some lower threshold??
Does anyone else thinks this would be worth pursuing?
Thanks,
NeilBrown
Christian Brauner
maintainerQuoted 11 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.
Quoted 8 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
I really like this concept (I think Lennart has talked about support for this before?).
Fwiw, I would really like if nsfs and pidfs file handles were signed specifically because they're not supposed to persist across reboot. But we can't do that in a backward compatbile way because userspace accesses the file handle directly to get e.g., the inode number out of it.
But for new types of file handles for such pseudo-fses I think this would be very lovely to have. It would probably mean generating a per-boot key that is used to sign such file handles.
For nfs I understand that you probably outsource the problem to userspace how to generate and share the key.
Quoted 12 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^
Quoted 7 lines
This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Fyi, it's not so simple. For some filesystems like pidfs and nsfs they have custom permission checks and are available to unprivileged users. But they also don't do any path lookup ofc.
Quoted 12 lines
In contrast to a local user using
open_by_handle(2), the NFS server must permissively allow remote clients to open by filehandle without being able to check or trust the remote caller's access. Therefore additional protection against this attack is needed for NFS case. We propose to sign filehandles by appending an 8-byte MAC which is the siphash of the filehandle from a key set from the nfs-utilities. NFS server can then ensure that guessing a valid filehandle+MAC is practically impossible without knowledge of the MAC's key. The NFS server performs optional signing by possessing a key set from userspace and having the "sign_fh" export option.
Quoted 3 lines
Because filehandles are long-lived, and there's no method for expiring them, the server's key should be set once and not changed. It also should be persisted across restarts. The methods to set the key allow only setting it
Right, otherwise you break a ton of stuff that relies on stable file handles across reboots.
Quoted 51 lines
once, afterward it cannot be changed. A separate patchset for nfs-utils contains the userspace changes required to set the server's key.
I had a previous attempt to solve this problem by encrypting filehandles, which turned out to be a problematic, poor solution. The discussion on that previous attempt is here:
https://lore.kernel.org/linux-nfs/[email protected]/T/#tThere are some changes from that version:
- sign filehandles instead of encrypt them (Eric Biggers)
- fix theNFSEXP_macros, specificallyNFSEXP_ALLFLAGS(NeilBrown)
- rebase onto cel/nfsd-next (Chuck Lever)
- condensed/clarified problem explantion (thanks Chuck Lever)
- add nfsctl file "fh_key" for rpc.nfsd to also set the keyI plan on adding additional work to enable the server to check whether the 8-byte MAC will overflow maximum filehandle length for the protocol at export time. There could be some filesystems with 40-byte fileid and 24-byte fsid which would break NFSv3's 64-byte filehandle maximum with an 8-byte MAC appended. The server should refuse to export those filesystems when "
sign_fh" is requested.Thanks for any comments and critique.
Benjamin Coddington (4):
nfsd: Convert export flags to useBIT()macro
nfsd: Add a key for signing filehandles
NFSD/export: Addsign_fhexport option
NFSD: Sign filehandlesDocumentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 15 +++++ fs/nfsd/netlink.h | 1 + fs/nfsd/netns.h | 2 + fs/nfsd/nfs3xdr.c | 20 +++--- fs/nfsd/nfs4xdr.c | 12 ++-- fs/nfsd/nfsctl.c | 87 ++++++++++++++++++++++++++- fs/nfsd/nfsfh.c | 72 +++++++++++++++++++++- fs/nfsd/nfsfh.h | 22 +++++++ fs/nfsd/trace.h | 19 ++++++ include/linux/sunrpc/svc.h | 1 + include/uapi/linux/nfsd/export.h | 38 ++++++------ include/uapi/linux/nfsd_netlink.h | 2 + 14 files changed, 272 insertions(+), 36 deletions(-)base-commit:
bfd453acb5637b5df881cef4b21803344aa9e7acSignature · 1 line
2.50.1
Quoted 21 lines · Christian Brauner
Quoted 11 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
Filesystems generate their own filehandles through the
export_operation"encode_fh" and a filehandle provides sufficient access to open a file without needing to perform a lookup. An NFS client holding a valid filehandle can remotely open and read the contents of the file referred to by the filehandle.Quoted 8 lines
In order to acquire a filehandle, you must perform lookup operations on the parent directory(ies), and the permissions on those directories may prohibit you from walking into them to find the files within. This would normally be considered sufficient protection on a local filesystem to prohibit users from accessing those files, however when the filesystem is exported via NFS those files can still be accessed by guessing the correct, valid filehandles.
Quoted 7 lines
I really like this concept (I think Lennart has talked about support for this before?).
Fwiw, I would really like if nsfs and pidfs file handles were signed specifically because they're not supposed to persist across reboot. But we can't do that in a backward compatbile way because userspace accesses the file handle directly to get e.g., the inode number out of it.
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
The only reason we need signing is because filesystems only provide 32bits of generation number. If a filesystem stored 64 bits, and used a crypto-safe random number for the generation number, then we wouldn't need signing or a key.
We need a key, effectively, to turn a 32bit number that can be iterated into a 64bit number which cannot, in a non-reversible way.
Does userspace refuse the extract the inode number if the filehandle size changes? It it can cope with size change, then adding a random number to the end of the filehandle should not be a problem.
Quoted 4 lines
But for new types of file handles for such pseudo-fses I think this would be very lovely to have. It would probably mean generating a per-boot key that is used to sign such file handles.
For new fses I recommend including a 64bit random number. No signing.
Quoted 24 lines
For nfs I understand that you probably outsource the problem to userspace how to generate and share the key.
Quoted 12 lines
Filehandles are easy to guess because they are well-formed. The
open_by_handle_at(2)man page contains an example C program (t_name_to_handle_at.c) that can display a filehandle given a path. Here's an example filehandle from a fairly modern XFS:# ./t_name_to_handle_at /exports/foo 57 12 129 99 00 00 00 00 00 00 00 b4 10 0b 8c^--------- filehandle ----------^ ^------- inode -------^ ^-- gen --^Quoted 7 lines
This filehandle consists of a 64-bit inode number and 32-bit generation number. Because the handle is well-formed, its easy to fabricate filehandles that match other files within the same filesystem. You can simply insert inode numbers and iterate on the generation number. Eventually you'll be able to access the file using
open_by_handle_at(2). For a local system,open_by_handle_at(2)requiresCAP_DAC_READ_SEARCH, which protects against guessing attacks by unprivileged users.
Quoted 3 lines
Fyi, it's not so simple. For some filesystems like pidfs and nsfs they have custom permission checks and are available to unprivileged users. But they also don't do any path lookup ofc.
I didn't know that.....
Oh, there is a "permission" operation now:
* permission:
* Allow filesystems to specify a custom permission function.
Not the most useful documentation I have ever read. Not documented in Documentation/filesystems/exporting.rst
Not used in fs/exportfs/.
Ahhh.. used in fs/fhandle.c to bypass may_decode_fh()
Fair enough - seems sane for a special-purpose filesystem to give away different access.
Thanks for the info.
I wonder if nfsd should refuse to export filesystems which have a .permission function, as they clearly are something special.
NeilBrown
Jeff Layton
maintainerQuoted 8 lines · NeilBrown
Thanks for the info.
I wonder if nfsd should refuse to export filesystems which have a .permission function, as they clearly are something special.
That would exclude a lot of filesystems that are currently exportable (including NFS). I don't think we want to do that.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 5 lines · Jeff Layton
Quoted 4 lines · NeilBrown
Thanks for the info.
Quoted 7 lines
Quoted 4 lines
I wonder if nfsd should refuse to export filesystems which have a .permission function, as they clearly are something special.
That would exclude a lot of filesystems that are currently exportable (including NFS). I don't think we want to do that.
I was referring to the .permission function in export_operations, not the one in inode_operations (and no, it isn't confusing at all that both structs have the ops with the same name but different function....).
NFS doesn't define .permission in export_operations. Only pidfs and nsfs do.
NeilBrown
Quoted 4 lines
Signature · 2 lines
-- Jeff Layton <[email protected]>
Christian Brauner
maintainerQuoted 3 lines
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
For pidfs I went with the 64-bit inode number. But I dislike the generation number thing. If I would have to freedom to completely redo it I would probably assign a uuid to the pidfs sb and then use that in the file handles alongside the inode number. That would be enough for sure as the uuid would change on each boot.
Quoted 11 lines
The only reason we need signing is because filesystems only provide 32bits of generation number. If a filesystem stored 64 bits, and used a crypto-safe random number for the generation number, then we wouldn't need signing or a key.
We need a key, effectively, to turn a 32bit number that can be iterated into a 64bit number which cannot, in a non-reversible way.
Does userspace refuse the extract the inode number if the filehandle size changes? It it can cope with size change, then adding a random number to the end of the filehandle should not be a problem.
At least nsfs file handles are public api and may grow in size.
Quoted 12 lines
I didn't know that.....
Oh, there is a "permission" operation now:* permission:
* Allow filesystems to specify a custom permission function.Not the most useful documentation I have ever read. Not documented in
Documentation/filesystems/exporting.rstNot used in
fs/exportfs/.
Ahhh.. used infs/fhandle.cto bypassmay_decode_fh()
Quoted 7 lines
Fair enough - seems sane for a special-purpose filesystem to give away different access.
Thanks for the info.
I wonder if nfsd should refuse to export filesystems which have a .permission function, as they clearly are something special.
No problem. pidfs and nsfs have custom permission and open because they both don't need to reconstruct any paths and aren't subject to the same permission checking.
Quoted 9 lines · Christian Brauner
Quoted 3 lines
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
For pidfs I went with the 64-bit inode number. But I dislike the generation number thing. If I would have to freedom to completely redo it I would probably assign a uuid to the pidfs sb and then use that in the file handles alongside the inode number. That would be enough for sure as the uuid would change on each boot.
What you are calling a "uuid" in "the pidfs sb" is exactly what I am calling a "generation number" - for pidfs it would be a "generation number" for the whole filesystem, while for ext4 etc it is a generation number of the inode number.
So we are substantially in agreement.
Why do you not have freedom to add a uuid to the pidfs sb and to the filehandles now?
Thanks,
NeilBrown
Quoted 39 lines
Quoted 6 lines
The only reason we need signing is because filesystems only provide 32bits of generation number. If a filesystem stored 64 bits, and used a crypto-safe random number for the generation number, then we wouldn't need signing or a key.
Quoted 6 lines
We need a key, effectively, to turn a 32bit number that can be iterated into a 64bit number which cannot, in a non-reversible way.
Does userspace refuse the extract the inode number if the filehandle size changes? It it can cope with size change, then adding a random number to the end of the filehandle should not be a problem.
At least nsfs file handles are public api and may grow in size.
Quoted 12 lines
I didn't know that.....
Oh, there is a "permission" operation now:* permission:
* Allow filesystems to specify a custom permission function.Not the most useful documentation I have ever read. Not documented in
Documentation/filesystems/exporting.rstNot used in
fs/exportfs/.
Ahhh.. used infs/fhandle.cto bypassmay_decode_fh()Quoted 7 lines
Fair enough - seems sane for a special-purpose filesystem to give away different access.
Thanks for the info.
I wonder if nfsd should refuse to export filesystems which have a .permission function, as they clearly are something special.
No problem. pidfs and nsfs have custom permission and open because they both don't need to reconstruct any paths and aren't subject to the same permission checking.
Christian Brauner
maintainerQuoted 5 lines · NeilBrown
Quoted 4 lines · Christian Brauner
Quoted 3 lines
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
Quoted 8 lines
Quoted 5 lines
For pidfs I went with the 64-bit inode number. But I dislike the generation number thing. If I would have to freedom to completely redo it I would probably assign a uuid to the pidfs sb and then use that in the file handles alongside the inode number. That would be enough for sure as the uuid would change on each boot.
What you are calling a "uuid" in "the pidfs sb" is exactly what I am calling a "generation number" - for pidfs it would be a "generation
"generation number" just evokes the 32-bit identifier in struct inode that's overall somewhat useless. And a UUID has much stronger guarantees.
Quoted 4 lines
number" for the whole filesystem, while for ext4 etc it is a generation number of the inode number.
So we are substantially in agreement.
Great!
Quoted 3 lines
Why do you not have freedom to add a uuid to the pidfs sb and to the filehandles now?
Userspace relies on the current format to get the inode number from the file handle:
https://github.com/systemd/systemd/blob/main/src/basic/pidfd-util.c#L233-L281
And they often also construct them in userspace. That needs to continue to work. I also don't think it's that critical.
Quoted 27 lines · Christian Brauner
Quoted 5 lines · NeilBrown
Quoted 4 lines · Christian Brauner
Quoted 3 lines
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
Quoted 8 lines
Quoted 5 lines
For pidfs I went with the 64-bit inode number. But I dislike the generation number thing. If I would have to freedom to completely redo it I would probably assign a uuid to the pidfs sb and then use that in the file handles alongside the inode number. That would be enough for sure as the uuid would change on each boot.
What you are calling a "uuid" in "the pidfs sb" is exactly what I am calling a "generation number" - for pidfs it would be a "generation
"generation number" just evokes the 32-bit identifier in struct inode that's overall somewhat useless. And a UUID has much stronger guarantees.
Quoted 4 lines
number" for the whole filesystem, while for ext4 etc it is a generation number of the inode number.
So we are substantially in agreement.
Great!
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
Why do you not have freedom to add a uuid to the pidfs sb and to the filehandles now?
Userspace relies on the current format to get the inode number from the file handle:
https://github.com/systemd/systemd/blob/main/src/basic/pidfd-util.c#L233-L281
The
assert(r != -EOVERFLOW);
means you cannot extend it
Quoted 4 lines
And they often also construct them in userspace. That needs to continue to work. I also don't think it's that critical.
as does this.
OK, thanks.
NeilBrown
Christian Brauner
maintainerQuoted 40 lines · NeilBrown
Quoted 27 lines · Christian Brauner
Quoted 5 lines · NeilBrown
Quoted 4 lines · Christian Brauner
Quoted 3 lines
You don't need signing to ensure a filehandle doesn't persist across reboot. For that you just need a generation number. Storing a random number generated at boot time in the filehandle would be a good solution.
Quoted 8 lines
Quoted 5 lines
For pidfs I went with the 64-bit inode number. But I dislike the generation number thing. If I would have to freedom to completely redo it I would probably assign a uuid to the pidfs sb and then use that in the file handles alongside the inode number. That would be enough for sure as the uuid would change on each boot.
What you are calling a "uuid" in "the pidfs sb" is exactly what I am calling a "generation number" - for pidfs it would be a "generation
"generation number" just evokes the 32-bit identifier in struct inode that's overall somewhat useless. And a UUID has much stronger guarantees.
Quoted 4 lines
number" for the whole filesystem, while for ext4 etc it is a generation number of the inode number.
So we are substantially in agreement.
Great!
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
Why do you not have freedom to add a uuid to the pidfs sb and to the filehandles now?
Userspace relies on the current format to get the inode number from the file handle:
https://github.com/systemd/systemd/blob/main/src/basic/pidfd-util.c#L233-L281The
assert(r !=-EOVERFLOW);
means you cannot extend itQuoted 1 line
Quoted 5 lines
Quoted 3 lines
And they often also construct them in userspace. That needs to continue to work. I also don't think it's that critical.
as does this.
Well, we could add a new version and we could make this work. I just don't think we need to right now.
Btw, the manpage for name_to_handle_at() and friends mandates that file handles are to be treated as opaque and should not be messed with directly. That ship has sailed a long time ago. Userspace very much relies on file handle layout. That's at least true for cgroupfs and pidfs.
Quoted 4 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
I've pondering this some more and I think we can get the same result without any API change, though it would require internal changes to each filesystem of interest.
The problem can be rephrased as saying that 32 bits is too small for a generation number as it allows guessing in a modest time. A 64bit generation number would provide sufficient protection (though admittedly not quite as much as a 32bit gen number with a 64 bit MAC).
If a filesystem had 64 bits or more of random bits in the superblock which are stable and not visible over NFS, it could combine those with the 32bit gen number stored in the inode and present a 64 bit generation number in the filehandle which would, in practice, be unguessable.
ext4 has s_journal_uuid which is stable and (I think) is not visible. xfs has sb_meta_uuid which seems to fill a vaguely similar role. I haven't looked further afield but it is credible that filesystems either have some suitable bits, or could add them.
We could add a generic_encode_ino32_gen64_fh() which takes a uuid or similar, and some corresponding support for verifying the gen64 on decode.
This would remove the need to change nfs-utils at all.
This would mean the file handles change when you upgrade to a new kernel (though a CONFIG option would could give distros control of when to make the change), but nfsd it able to handle that if we are careful. Lookups from an "old" style filehandle will always produce an "old" style filehandle. But when the client remounts they get a new style for everything.
This is a lot more work in the kernel and requires buy-in from each relevant fs maintainer, but saves us any API change and so is largely transparent.
Is it worth it? I don't know.
NeilBrown
Quoted 25 lines · NeilBrown
Quoted 4 lines · Benjamin Coddington
The following series enables the linux NFS server to add a Message Authentication Code (MAC) to the filehandles it gives to clients. This provides additional protection to the exported filesystem against filehandle guessing attacks.
I've pondering this some more and I think we can get the same result without any API change, though it would require internal changes to each filesystem of interest.
The problem can be rephrased as saying that 32 bits is too small for a generation number as it allows guessing in a modest time. A 64bit generation number would provide sufficient protection (though admittedly not quite as much as a 32bit gen number with a 64 bit MAC).
If a filesystem had 64 bits or more of random bits in the superblock which are stable and not visible over NFS, it could combine those with the 32bit gen number stored in the inode and present a 64 bit generation number in the filehandle which would, in practice, be unguessable.
ext4 has
s_journal_uuidwhich is stable and (I think) is not visible. xfs hassb_meta_uuidwhich seems to fill a vaguely similar role. I haven't looked further afield but it is credible that filesystems either have some suitable bits, or could add them.
Quoted 5 lines
We could add a
generic_encode_ino32_gen64_fh()which takes a uuid or similar, and some corresponding support for verifying the gen64 on decode.This would remove the need to change nfs-utils at all.
If so, it would be nice to find a way to unify setting/changing the per-fs unique value because cloning and "golden image" approaches are often used in deployments. I think there are steps taken to "uniquify" cloned systems, and setting/resetting this key would need to be one of those.
Quoted 7 lines
This would mean the file handles change when you upgrade to a new kernel (though a CONFIG option would could give distros control of when to make the change), but nfsd it able to handle that if we are careful. Lookups from an "old" style filehandle will always produce an "old" style filehandle. But when the client remounts they get a new style for everything.
Quoted 5 lines
This is a lot more work in the kernel and requires buy-in from each relevant fs maintainer, but saves us any API change and so is largely transparent.
Is it worth it? I don't know.
Good pondering, I'm hoping this discussion continues a bit and we can figure it out - I'll still iterate on this design in the meantime.
Ben
Quoted 2 lines · Benjamin Coddington
Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +-
Would this make sense as a generic utility (i.e. in fs/exportfs/)?
The ultimate use case for me would be unprivileged open_by_handle_at(2).
Thanks,
Miklos
Quoted 9 lines · Miklos Szeredi
Quoted 2 lines · Benjamin Coddington
Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +-Would this make sense as a generic utility (i.e. in
fs/exportfs/)?The ultimate use case for me would be unprivileged
open_by_handle_at(2).
It might - I admit I don't know if signed filehandles would sufficiently protect everything that CAP_DAC_READ_SEARCH has. I've been focused on the NFS (and pNFS/flexfiles) cases.
Would open_by_handle_at(2) need the ability to set/change the confidential key used, and how can it be persisted? Neil has some ideas about using a per-fs unique value.
Ben
Quoted 9 lines · Benjamin Coddington
Quoted 6 lines · Miklos Szeredi
Quoted 2 lines · Benjamin Coddington
Documentation/netlink/specs/nfsd.yaml | 12 ++++ fs/nfsd/export.c | 5 +-
Quoted 7 lines
Quoted 3 lines
Would this make sense as a generic utility (i.e. in
fs/exportfs/)?The ultimate use case for me would be unprivileged
open_by_handle_at(2).It might - I admit I don't know if signed filehandles would sufficiently protect everything that
CAP_DAC_READ_SEARCHhas. I've been focused on the NFS (and pNFS/flexfiles) cases.
Problem is that keeping access to a file through an open file descriptor can be better controlled than through a file handle, which is just a cookie in memory. Otherwise the two are equivalent, AFAICS.
I guess this is something that can be decided by the admin by weighing the pros and cons.
Quoted 3 lines
Would
open_by_handle_at(2)need the ability to set/change the confidential key used, and how can it be persisted? Neil has some ideas about using a per-fs unique value.
I think an automatically generated per-userns key would work in some situations, for example.
An unprivileged userspace NFS server that can survive a reboot would need a persistent key, but that would have to be managed by a privileged entity.
Thanks,
Miklos