kNFSD Signed Filehandles
Subsystems (8)
v4 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. 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.
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 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.
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.
Simple testing confirms that the correct generation number can be found within ~1200 minutes using open_by_handle_at() over NFS on a local system and it is estimated that adding network delay with genuine NFS calls may only increase this to around 24 hours.
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 planned 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. However, the way the export caches work (the server may not even be running when a user sets up the export) its impossible to do this check at export time. Instead, the server will refuse to give out filehandles at mount time and emit a pr_warn().
Thanks for any comments and critique.
Changes from encrypt_fh posting:
https://lore.kernel.org/linux-nfs/[email protected]
- 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
Changes from v1 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- remove fh_fileid_offset() (Chuck Lever)
- fix pr_warns, fix memcmp (Chuck Lever)
- remove incorrect rootfh comment (NeilBrown)
- make fh_key setting an optional attr to threads verb (Jeff Layton)
- drop BIT() EXP_ flag conversion
- cover-letter tune-ups (NeilBrown, Chuck Lever)
- fix NFSEXP_ALLFLAGS on 2/3
- cast fh->fh_size + sizeof(hash) result to int (avoid x86_64 WARNING)
- move MAC signing into __fh_update() (Chuck Lever)
Changes from v2 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- more cover-letter detail (NeilBrown)
- Documentation/filesystems/nfs/exporting.rst section (Jeff Layton)
- fix key copy (Eric Biggers)
- use NFSD_A_SERVER_MAX (NeilBrown)
- remove procfs fh_key interface (Chuck Lever)
- remove FH_AT_MAC (Chuck Lever)
- allow fh_key change when server is not running (Chuck/Jeff)
- accept fh_key as netlink attribute instead of command (Jeff Layton)
Changes from v3 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- /actually/ fix up endianness problems (Eric Biggers)
- comment typo
- fix Documentation underline warnings
- fix possible uninitialized fh_key var
Benjamin Coddington (3):
NFSD: Add a key for signing filehandles
NFSD/export: Add sign_fh export option
NFSD: Sign filehandles
Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++ Documentation/netlink/specs/nfsd.yaml | 6 ++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 5 +- fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 37 ++++++++- fs/nfsd/nfsfh.c | 64 +++++++++++++++- fs/nfsd/trace.h | 25 ++++++ include/uapi/linux/nfsd/export.h | 4 +- include/uapi/linux/nfsd_netlink.h | 1 + 10 files changed, 225 insertions(+), 9 deletions(-)
base-commit: e3934bbd57c73b3835a77562ca47b5fbc6f34287
Signature · 1 line
2.50.1
PATCH v4 1/3 NFSD: Add a key for signing filehandles
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
--- Documentation/netlink/specs/nfsd.yaml | 6 +++++ fs/nfsd/netlink.c | 5 ++-- fs/nfsd/netns.h | 2 ++ fs/nfsd/nfsctl.c | 37 ++++++++++++++++++++++++++- fs/nfsd/trace.h | 25 ++++++++++++++++++ include/uapi/linux/nfsd_netlink.h | 1 + 6 files changed, 73 insertions(+), 3 deletions(-)
Documentation/netlink/specs/nfsd.yaml +6 -0
@@ -81,6 +81,11 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary + checks: + exact-len: 16 - name: version attributes:
@@ -163,6 +168,7 @@ operations: - leasetime - scope - min-threads + - fh-key - name: threads-get doc: get the maximum number of running threads
fs/nfsd/netlink.c +3 -2
@@ -24,12 +24,13 @@ const struct nla_policy nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = { };/* NFSD_CMD_THREADS_SET - do */ -static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS +1 ] = { +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY +1 ] = { [NFSD_A_SERVER_THREADS ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_GRACETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_LEASETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_SCOPE ] = { .type =NLA_NUL_STRING , }, [NFSD_A_SERVER_MIN_THREADS ] = { .type =NLA_U32 , }, + [NFSD_A_SERVER_FH_KEY ] =NLA_POLICY_EXACT_LEN (16 ), };/* NFSD_CMD_VERSION_SET - do */
@@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .cmd = NFSD_CMD_THREADS_SET, .doit = nfsd_nl_threads_set_doit, .policy = nfsd_threads_set_nl_policy, - .maxattr = NFSD_A_SERVER_MIN_THREADS, + .maxattr = NFSD_A_SERVER_MAX, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, {
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 +36 -1
@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno. +*/ +static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) +{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t)) +return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +return 0 ; +} +/** * nfsd_nl_threads_set_doit - set the number of running threads * @skb: reply buffer
@@ -1612,7 +1637,8 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)if (info->attrs[NFSD_A_SERVER_GRACETIME ] || info->attrs[NFSD_A_SERVER_LEASETIME ] || - info->attrs[NFSD_A_SERVER_SCOPE ]) { + info->attrs[NFSD_A_SERVER_SCOPE ] || + info->attrs[NFSD_A_SERVER_FH_KEY ]) { ret = -EBUSY ;if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads) goto out_unlock;
@@ -1641,6 +1667,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info) attr = info->attrs[NFSD_A_SERVER_SCOPE ];if (attr) scope =nla_data (attr); + + attr = info->attrs[NFSD_A_SERVER_FH_KEY ]; +if (attr) { + ret =nfsd_nl_fh_key_set (attr, nn); +trace_nfsd_ctl_fh_key_set ((const char *)nn->fh_key, ret); +if (ret) + goto out_unlock; + } } attr = info->attrs[NFSD_A_SERVER_MIN_THREADS ];
@@ -2240,6 +2274,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id); +kfree_sensitive (nn->fh_key);nfsd_proc_stat_shutdown (net);percpu_counter_destroy_many (nn->counter,NFSD_STATS_COUNTERS_NUM );nfsd_idmap_shutdown (net);
fs/nfsd/trace.h +25 -0
@@ -2240,6 +2240,31 @@ 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) +__field (bool, key_set) + ), +TP_fast_assign ( + __entry->key_set = true; + if (!key) + __entry->key_set = false; + else + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" , + __entry->key_set ?__print_hex_str (__entry->key,16 ) :"(null)" , + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copy
include/uapi/linux/nfsd_netlink.h +1 -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 )
Signature · 1 line
2.50.1
Re: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
Quoted 109 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 6 +++++ fs/nfsd/netlink.c | 5 ++-- fs/nfsd/netns.h | 2 ++ fs/nfsd/nfsctl.c | 37 ++++++++++++++++++++++++++- fs/nfsd/trace.h | 25 ++++++++++++++++++ include/uapi/linux/nfsd_netlink.h | 1 + 6 files changed, 73 insertions(+), 3 deletions(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -81,6 +81,11 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary + checks: + exact-len: 16 - name: version attributes:@@ -163,6 +168,7 @@ operations: - leasetime - scope - min-threads + - fh-key - name: threads-get doc: get the maximum number of running threadsfs/nfsd/netlink.c +3
-2@@ -24,12 +24,13 @@ const struct nla_policy nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = { };/* NFSD_CMD_THREADS_SET - do */ -static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS +1 ] = { +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY +1 ] = { [NFSD_A_SERVER_THREADS ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_GRACETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_LEASETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_SCOPE ] = { .type =NLA_NUL_STRING , }, [NFSD_A_SERVER_MIN_THREADS ] = { .type =NLA_U32 , }, + [NFSD_A_SERVER_FH_KEY ] =NLA_POLICY_EXACT_LEN (16 ), };/* NFSD_CMD_VERSION_SET - do */ @@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .cmd = NFSD_CMD_THREADS_SET, .doit = nfsd_nl_threads_set_doit, .policy = nfsd_threads_set_nl_policy, - .maxattr = NFSD_A_SERVER_MIN_THREADS, + .maxattr = NFSD_A_SERVER_MAX, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, {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 +12
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno. +*/ +static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) +{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))
fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1595
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
Another error here ^^, I somehow failed :wq to increment to the next u64. I will send the fix in a much shorter timeframe.
Ben
Chuck Lever
maintainerRe: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 33 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>---
fs/nfsd/nfsctl.c +6
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static intnfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)fs/nfsd/nfsctl.c +4
-0+{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))
fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1595
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
put_unaligned_le64() takes a value as its first argument and a destination pointer as its second. These two lines write the contents of fh_key->key[0] into the nlattr buffer rather than reading userspace data into the key.
On the first call, fh_key was just kmalloc'd and contains uninitialized heap data, so the key is never populated from userspace input.
Additionally, both lines reference key[0] -- the second should reference key[1] and write to an offset of nla_data(attr).
The correct form, following the pattern in fscrypt_derive_siphash_key(), would be something like:
fh_key->key[0] = get_unaligned_le64(nla_data(attr));
fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);Quoted 6 lines
fs/nfsd/nfsctl.c +3
-0+return 0 ; +} +/** * nfsd_nl_threads_set_doit - set the number of running threads * @skb: reply buffer
Signature · 1 line
Chuck Lever
Re: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
Quoted 54 lines · Chuck Lever
Quoted 33 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>---
fs/nfsd/nfsctl.c +6
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static intnfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)fs/nfsd/nfsctl.c +4
-0+{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1595
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
put_unaligned_le64()takes a value as its first argument and a destination pointer as its second. These two lines write the contents of fh_key->key[0] into the nlattr buffer rather than reading userspace data into the key.On the first call,
fh_keywas just kmalloc'd and contains uninitialized heap data, so the key is never populated from userspace input.
Quoted 8 lines
Additionally, both lines reference key[0] -- the second should reference key[1] and write to an offset of
nla_data(attr).The correct form, following the pattern in
fscrypt_derive_siphash_key(), would be something like:fh_key->key[0] = get_unaligned_le64(nla_data(attr)); fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);
Yes- thanks Chuck, I really messed this one up.. somehow sending out the wrong version.
Ben
Re: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
Quoted 59 lines · Benjamin Coddington
Quoted 57 lines · Chuck Lever
Quoted 33 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>---
fs/nfsd/nfsctl.c +6
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static intnfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)fs/nfsd/nfsctl.c +4
-0+{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))fs/nfsd/nfsctl.c
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
put_unaligned_le64()takes a value as its first argument and a destination pointer as its second. These two lines write the contents of fh_key->key[0] into the nlattr buffer rather than reading userspace data into the key.On the first call,
fh_keywas just kmalloc'd and contains uninitialized heap data, so the key is never populated from userspace input.Additionally, both lines reference key[0] -- the second should reference key[1] and write to an offset of
nla_data(attr).
Quoted 8 lines
Quoted 5 lines
The correct form, following the pattern in
fscrypt_derive_siphash_key(), would be something like:fh_key->key[0] = get_unaligned_le64(nla_data(attr)); fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);Yes- thanks Chuck, I really messed this one up.. somehow sending out the wrong version.
I think nla_data() returns void* - and we want to ensure that void* + 8 moves 8 bytes. Isn't this a GCC extension that assumes void* + 1 moves one byte?
ISTR other conversations about this with you, is it maybe safer to do something like this?
fs/nfsd/nfsctl.c +5 -2
@@ -1581,6 +1581,7 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) {siphash_key_t *fh_key = nn->fh_key; +u8 *data;if (nla_len (attr) !=sizeof (siphash_key_t))return -EINVAL ;
@@ -1591,8 +1592,10 @@ static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)return -ENOMEM ; nn->fh_key = fh_key; } -put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); -put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); + + data =nla_data (attr); + fh_key->key[0 ] =get_unaligned_le64 (data); + fh_key->key[1 ] =get_unaligned_le64 (data +8 );return 0 ; }
Ben
Chuck Lever
maintainerRe: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
Quoted 103 lines · Benjamin Coddington
Quoted 59 lines · Benjamin Coddington
Quoted 57 lines · Chuck Lever
Quoted 33 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Link:
https://lore.kernel.org/linux-nfs/[email protected]Signed-off-by Benjamin Coddington <[email protected]>---
fs/nfsd/nfsctl.c +6
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno.
+ */
+static intnfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn)fs/nfsd/nfsctl.c +4
-0+{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))fs/nfsd/nfsctl.c
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
put_unaligned_le64()takes a value as its first argument and a destination pointer as its second. These two lines write the contents of fh_key->key[0] into the nlattr buffer rather than reading userspace data into the key.On the first call,
fh_keywas just kmalloc'd and contains uninitialized heap data, so the key is never populated from userspace input.Additionally, both lines reference key[0] -- the second should reference key[1] and write to an offset of
nla_data(attr).Quoted 8 lines
Quoted 5 lines
The correct form, following the pattern in
fscrypt_derive_siphash_key(), would be something like:fh_key->key[0] = get_unaligned_le64(nla_data(attr)); fh_key->key[1] = get_unaligned_le64(nla_data(attr) + 8);Yes- thanks Chuck, I really messed this one up.. somehow sending out the wrong version.
I think
nla_data()returns void* - and we want to ensure that void* + 8 moves 8 bytes. Isn't this a GCC extension that assumes void* + 1 moves one byte?ISTR other conversations about this with you, is it maybe safer to do something like this?
fs/nfsd/nfsctl.c +5
-2@@ -1581,6 +1581,7 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) {siphash_key_t *fh_key = nn->fh_key; +u8 *data;if (nla_len (attr) !=sizeof (siphash_key_t))return -EINVAL ;@@ -1591,8 +1592,10 @@ static int nfsd_nl_fh_key_set(const struct nlattr *attr, struct nfsd_net *nn) return -ENOMEM; nn->fh_key = fh_key; } -put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); -put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); + + data =nla_data (attr); + fh_key->key[0 ] =get_unaligned_le64 (data); + fh_key->key[1 ] =get_unaligned_le64 (data +8 );return 0 ; }
Quoted 1 line
Ben
I did a little research. Void pointer arithmetic is explicitly sanctioned in the kernel code base. Documentation/kernel-hacking/hacking.rst (line 690) lists "Arithmetic on void pointers" as a standard accepted GNU extension. The kernel compiles with -std=gnu11. -Wpointer-arith is only enabled at W=3, not by default.
Additionally, Linus seems to prefer "void *" and I see pointer arithmetic in other contexts under fs/nfsd/ that use a void * cast.
So "get_unaligned_le64(nla_data(attr) + 8)" is fine.
Signature · 1 line
Chuck Lever
PATCH v4 2/3 NFSD/export: Add sign_fh export option
In reply to Benjamin CoddingtonIn order to signal that filehandles on this export should be signed, add a "sign_fh" export option. Filehandle signing can help the server defend against certain filehandle guessing attacks.
Setting 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.
While we're in here, tidy a few stray expflags to more closely align to the export flag order.
--- 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 0x0020#define NFSEXP_NOREADDIRPLUS 0x0040#define NFSEXP_SECURITY_LABEL 0x0080 -/* 0x100 currently unused */ +#define NFSEXP_SIGN_FH 0x0100#define NFSEXP_NOHIDE 0x0200#define NFSEXP_NOSUBTREECHECK 0x0400#define NFSEXP_NOAUTHNLM 0x0800/* Don't authenticate NLM requests - just trust */
@@ -55,7 +55,7 @@#define NFSEXP_PNFS 0x20000/* All flags that we claim to support. (Note we don't support NOACL.) */ -#define NFSEXP_ALLFLAGS 0x3FEFF +#define NFSEXP_ALLFLAGS 0x3FFFF/* The flags that may vary depending on security flavor: */ #define NFSEXP_SECINFO_FLAGS (NFSEXP_READONLY | NFSEXP_ROOTSQUASH \
Signature · 1 line
2.50.1
PATCH v4 3/3 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. 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.
--- Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++ fs/nfsd/nfsfh.c | 64 +++++++++++++++- 2 files changed, 147 insertions(+), 2 deletions(-)
Documentation/filesystems/nfs/exporting.rst +85 -0
@@ -238,3 +238,88 @@ following flags are defined: all of an inode's dirty data on last close. Exports that behave this way should set EXPORT_OP_FLUSH_ON_CLOSE so that NFSD knows to skip waiting for writeback when closing such files. + +Signed Filehandles +------------------ + +To protect against filehandle guessing attacks, the Linux NFS server can be +configured to sign filehandles with a Message Authentication Code (MAC). + +Standard NFS filehandles are often predictable. If an attacker can guess +a valid filehandle for a file they do not have permission to access via +directory traversal, they may be able to bypass path-based permissions +(though they still remain subject to inode-level permissions). + +Signed filehandles prevent this by appending a MAC to the filehandle +before it is sent to the client. Upon receiving a filehandle back from a +client, the server re-calculates the MAC using its internal key and +verifies it against the one provided. If the signatures do not match, +the server treats the filehandle as invalid (returning NFS[34]ERR_STALE). + +Note that signing filehandles provides integrity and authenticity but +not confidentiality. The contents of the filehandle remain visible to +the client; they simply cannot be forged or modified. + +Configuration +~~~~~~~~~~~~~ + +To enable signed filehandles, the administrator must provide a signing +key to the kernel and enable the "sign_fh" export option. + +1. Providing a Key + The signing key is managed via the nfsd netlink interface. This key + is per-network-namespace and must be set before any exports using + "sign_fh" become active. + +2. Export Options + The feature is controlled on a per-export basis in /etc/exports: + + sign_fh + Enables signing for all filehandles generated under this export. + + no_sign_fh + (Default) Disables signing. + +Key Management and Rotation +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The security of this mechanism relies entirely on the secrecy of the +signing key. + +Initial Setup: + The key should be generated using a high-quality random source and + loaded early in the boot process or during the nfs-server startup + sequence. + +Changing Keys: + If a key is changed while clients have active mounts, existing + filehandles held by those clients will become invalid, resulting in + "Stale file handle" errors on the client side. + +Safe Rotation: + Currently, there is no mechanism for "graceful" key rotation + (maintaining multiple valid keys). Changing the key is an atomic + operation that immediately invalidates all previous signatures. + +Transitioning Exports +~~~~~~~~~~~~~~~~~~~~~ + +When adding or removing the "sign_fh" flag from an active export, the +following behaviors should be expected: + ++-------------------+---------------------------------------------------+ +| Change | Result for Existing Clients | ++===================+===================================================+ +| Adding sign_fh | Clients holding unsigned filehandles will find | +| | them rejected, as the server now expects a | +| | signature. | ++-------------------+---------------------------------------------------+ +| Removing sign_fh | Clients holding signed filehandles will find them | +| | rejected, as the server now expects the | +| | filehandle to end at its traditional boundary | +| | without a MAC. | ++-------------------+---------------------------------------------------+ + +Because filehandles are often cached persistently by clients, adding or +removing this option should generally be done during a scheduled maintenance +window involving a NFS client unmount/remount.
fs/nfsd/nfsfh.c +62 -2
89 lines not on this page · raw message
Signature · 1 line
2.50.1
Chuck Lever
maintainerRe: [PATCH v4 3/3] NFSD: Sign filehandles
Quoted 54 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. 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]>--- Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++ fs/nfsd/nfsfh.c | 64 +++++++++++++++- 2 files changed, 147 insertions(+), 2 deletions(-)Documentation/filesystems/nfs/exporting.rst +0
-0@@ -238,3 +238,88 @@ following flags are defined: all of an inode's dirty data on last close. Exports that behavethis
way should setEXPORT_OP_FLUSH_ON_CLOSEso that NFSD knows to skip waiting for writeback when closing such files.Documentation/filesystems/nfs/exporting.rst +5
-0+ +Signed Filehandles +------------------ + +To protect against filehandle guessing attacks, the Linux NFS servercan be
+configured to sign filehandles with a Message Authentication Code (MAC).Documentation/filesystems/nfs/exporting.rst +2
-0+ +Standard NFS filehandles are often predictable. If an attacker canguess
Documentation/filesystems/nfs/exporting.rst +6
-0+a valid filehandle for a file they do not have permission to access via +directory traversal, they may be able to bypass path-based permissions +(though they still remain subject to inode-level permissions). + +Signed filehandles prevent this by appending a MAC to the filehandle +before it is sent to the client. Upon receiving a filehandle back froma
Documentation/filesystems/nfs/exporting.rst +3
-0+client, the server re-calculates the MAC using its internal key and +verifies it against the one provided. If the signatures do not match, +the server treats the filehandle as invalid (returning
Quoted 1 line
NFS[34]ERR_STALE).
The documentation says NFS[34]ERR_STALE, but the code in nfsd_set_fh_dentry() returns nfserr_badhandle on MAC failure. The commit message also says _BADHANDLE.
Should the code be returning nfserr_stale here to match the documentation, or should the documentation say BADHANDLE?
IMHO STALE is the right answer for this purpose.
Quoted 99 lines
Documentation/filesystems/nfs/exporting.rst +60
-0+ +Note that signing filehandles provides integrity and authenticity but +not confidentiality. The contents of the filehandle remain visible to +the client; they simply cannot be forged or modified. + +Configuration +~~~~~~~~~~~~~ + +To enable signed filehandles, the administrator must provide a signing +key to the kernel and enable the "sign_fh" export option. + +1. Providing a Key + The signing key is managed via the nfsd netlink interface. This key + is per-network-namespace and must be set before any exports using + "sign_fh" become active. + +2. Export Options + The feature is controlled on a per-export basis in /etc/exports: + + sign_fh + Enables signing for all filehandles generated under this export. + + no_sign_fh + (Default) Disables signing. + +Key Management and Rotation +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The security of this mechanism relies entirely on the secrecy of the +signing key. + +Initial Setup: + The key should be generated using a high-quality random source and + loaded early in the boot process or during the nfs-server startup + sequence. + +Changing Keys: + If a key is changed while clients have active mounts, existing + filehandles held by those clients will become invalid, resulting in + "Stale file handle" errors on the client side. + +Safe Rotation: + Currently, there is no mechanism for "graceful" key rotation + (maintaining multiple valid keys). Changing the key is an atomic + operation that immediately invalidates all previous signatures. + +Transitioning Exports +~~~~~~~~~~~~~~~~~~~~~ + +When adding or removing the "sign_fh" flag from an active export, the +following behaviors should be expected: + ++-------------------+---------------------------------------------------+ +| Change | Result for Existing Clients | ++===================+===================================================+ +| Adding sign_fh | Clients holding unsigned filehandles will find | +| | them rejected, as the server now expects a | +| | signature. | ++-------------------+---------------------------------------------------+ +| Removing sign_fh | Clients holding signed filehandles will findthem |
+| | rejected, as the server now expects the | +| | filehandle to end at its traditional boundary | +| | without a MAC. |++-------------------+---------------------------------------------------+
Documentation/filesystems/nfs/exporting.rst +3
-0+ +Because filehandles are often cached persistently by clients, adding or +removing this option should generally be done during a scheduledmaintenance
Documentation/filesystems/nfs/exporting.rst +1
-0+window involving a NFS client unmount/remount.fs/nfsd/nfsfh.c +7
-2@@ -240,9 +292,14 @@ 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 { +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,
Quoted 1 line
nfsd_acceptable, exp);
When a signed filehandle arrives from a client, fh->fh_size includes the 8-byte MAC. data_left is computed earlier as fh->fh_size / 4, so it includes 2 extra u32 words from the MAC.
After fh_verify_mac() succeeds, data_left is passed unchanged to exportfs_decode_fh_raw(). The filesystem's fh_to_dentry callback receives an fh_len that is 2 words larger than the actual file ID data.
Current filesystem implementations only check minimum fh_len, so the extra words are harmless in practice. Does data_left need to be reduced by sizeof(u64) / 4 after MAC verification so that exportfs_decode_fh_raw() receives the correct file ID length?
Signature · 1 line
Chuck Lever
Re: [PATCH v4 3/3] NFSD: Sign filehandles
Quoted 178 lines · Chuck Lever
Quoted 54 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. 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]>--- Documentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++ fs/nfsd/nfsfh.c | 64 +++++++++++++++- 2 files changed, 147 insertions(+), 2 deletions(-)Documentation/filesystems/nfs/exporting.rst +0
-0@@ -238,3 +238,88 @@ following flags are defined: all of an inode's dirty data on last close. Exports that behavethis
way should setEXPORT_OP_FLUSH_ON_CLOSEso that NFSD knows to skip waiting for writeback when closing such files.Documentation/filesystems/nfs/exporting.rst +5
-0+ +Signed Filehandles +------------------ + +To protect against filehandle guessing attacks, the Linux NFS servercan be
+configured to sign filehandles with a Message Authentication Code (MAC).Documentation/filesystems/nfs/exporting.rst +2
-0+ +Standard NFS filehandles are often predictable. If an attacker canguess
Documentation/filesystems/nfs/exporting.rst +6
-0+a valid filehandle for a file they do not have permission to access via +directory traversal, they may be able to bypass path-based permissions +(though they still remain subject to inode-level permissions). + +Signed filehandles prevent this by appending a MAC to the filehandle +before it is sent to the client. Upon receiving a filehandle back froma
Documentation/filesystems/nfs/exporting.rst +3
-0+client, the server re-calculates the MAC using its internal key and +verifies it against the one provided. If the signatures do not match, +the server treats the filehandle as invalid (returningQuoted 1 line
NFS[34]ERR_STALE).
The documentation says NFS[34]ERR_STALE, but the code in
nfsd_set_fh_dentry()returnsnfserr_badhandleon MAC failure. The commit message also says_BADHANDLE.Should the code be returning
nfserr_stalehere to match the documentation, or should the documentation say BADHANDLE?IMHO STALE is the right answer for this purpose.
Quoted 99 lines
Documentation/filesystems/nfs/exporting.rst +60
-0+ +Note that signing filehandles provides integrity and authenticity but +not confidentiality. The contents of the filehandle remain visible to +the client; they simply cannot be forged or modified. + +Configuration +~~~~~~~~~~~~~ + +To enable signed filehandles, the administrator must provide a signing +key to the kernel and enable the "sign_fh" export option. + +1. Providing a Key + The signing key is managed via the nfsd netlink interface. This key + is per-network-namespace and must be set before any exports using + "sign_fh" become active. + +2. Export Options + The feature is controlled on a per-export basis in /etc/exports: + + sign_fh + Enables signing for all filehandles generated under this export. + + no_sign_fh + (Default) Disables signing. + +Key Management and Rotation +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The security of this mechanism relies entirely on the secrecy of the +signing key. + +Initial Setup: + The key should be generated using a high-quality random source and + loaded early in the boot process or during the nfs-server startup + sequence. + +Changing Keys: + If a key is changed while clients have active mounts, existing + filehandles held by those clients will become invalid, resulting in + "Stale file handle" errors on the client side. + +Safe Rotation: + Currently, there is no mechanism for "graceful" key rotation + (maintaining multiple valid keys). Changing the key is an atomic + operation that immediately invalidates all previous signatures. + +Transitioning Exports +~~~~~~~~~~~~~~~~~~~~~ + +When adding or removing the "sign_fh" flag from an active export, the +following behaviors should be expected: + ++-------------------+---------------------------------------------------+ +| Change | Result for Existing Clients | ++===================+===================================================+ +| Adding sign_fh | Clients holding unsigned filehandles will find | +| | them rejected, as the server now expects a | +| | signature. | ++-------------------+---------------------------------------------------+ +| Removing sign_fh | Clients holding signed filehandles will findthem |
+| | rejected, as the server now expects the | +| | filehandle to end at its traditional boundary | +| | without a MAC. |++-------------------+---------------------------------------------------+
Documentation/filesystems/nfs/exporting.rst +3
-0+ +Because filehandles are often cached persistently by clients, adding or +removing this option should generally be done during a scheduledmaintenance
Documentation/filesystems/nfs/exporting.rst +1
-0+window involving a NFS client unmount/remount.fs/nfsd/nfsfh.c +7
-2@@ -240,9 +292,14 @@ 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 { +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,Quoted 1 line
nfsd_acceptable, exp);When a signed filehandle arrives from a client,
fh->fh_sizeincludes the 8-byte MAC.data_leftis computed earlier asfh->fh_size/ 4, so it includes 2 extra u32 words from the MAC.After
fh_verify_mac()succeeds,data_leftis passed unchanged toexportfs_decode_fh_raw(). The filesystem'sfh_to_dentrycallback receives anfh_lenthat is 2 words larger than the actual file ID data.
Quoted 5 lines
Current filesystem implementations only check minimum
fh_len, so the extra words are harmless in practice. Doesdata_leftneed to be reduced by sizeof(u64) / 4 after MAC verification so thatexportfs_decode_fh_raw()receives the correct file ID length?
Yes, it sure does -- great catch. Thanks Chuck.
Ben
kernel test robot
build botRe: [PATCH v4 3/3] 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 e3934bbd57c73b3835a77562ca47b5fbc6f34287]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Coddington/NFSD-Add-a-key-for-signing-filehandles/20260206-231407 base: e3934bbd57c73b3835a77562ca47b5fbc6f34287 patch link: https://lore.kernel.org/r/d34d4f79a7d4c6b77ad260f925cb51c34fd53ce5.1770390036.git.bcodding%40hammerspace.com patch subject: [PATCH v4 3/3] NFSD: Sign filehandles config: x86_64-randconfig-121-20260207 (https://download.01.org/0day-ci/archive/20260207/[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/20260207/[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]/
sparse warnings: (new ones prefixed by >>)
>> fs/nfsd/nfsfh.c:168:14: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned long long [usertype] hash @@ got restricted __le64 [usertype] @@
fs/nfsd/nfsfh.c:168:14: sparse: expected unsigned long long [usertype] hash
fs/nfsd/nfsfh.c:168:14: sparse: got restricted __le64 [usertype]
fs/nfsd/nfsfh.c:191:14: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned long long [usertype] hash @@ got restricted __le64 [usertype] @@
fs/nfsd/nfsfh.c:191:14: sparse: expected unsigned long long [usertype] hash
fs/nfsd/nfsfh.c:191:14: sparse: got restricted __le64 [usertype]
vim +168 fs/nfsd/nfsfh.c
143
144 /*
145 * Append an 8-byte MAC to the filehandle hashed from the server's fh_key:
146 */
147 static int fh_append_mac(struct svc_fh *fhp, struct net *net)
148 {
149 struct nfsd_net *nn = net_generic(net, nfsd_net_id);
150 struct knfsd_fh *fh = &fhp->fh_handle;
151 siphash_key_t *fh_key = nn->fh_key;
152 u64 hash;
153
154 if (!(fhp->fh_export->ex_flags & NFSEXP_SIGN_FH))
155 return 0;
156
157 if (!fh_key) {
158 pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_key not set.\n");
159 return -EINVAL;
160 }
161
162 if (fh->fh_size + sizeof(hash) > fhp->fh_maxsize) {
163 pr_warn_ratelimited("NFSD: unable to sign filehandles, fh_size %d would be greater"
164 " than fh_maxsize %d.\n", (int)(fh->fh_size + sizeof(hash)), fhp->fh_maxsize);
165 return -EINVAL;
166 }
167
> 168 hash = cpu_to_le64(siphash(&fh->fh_raw, fh->fh_size, fh_key));
169 memcpy(&fh->fh_raw[fh->fh_size], &hash, sizeof(hash));
170 fh->fh_size += sizeof(hash);
171
172 return 0;
173 }
174
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Quoted 131 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. 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.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 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.
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.Simple testing confirms that the correct generation number can be found within ~1200 minutes using
open_by_handle_at()over NFS on a local system and it is estimated that adding network delay with genuine NFS calls may only increase this to around 24 hours.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 planned 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. However, the way the export caches work (the server may not even be running when a user sets up the export) its impossible to do this check at export time. Instead, the server will refuse to give out filehandles at mount time and emit apr_warn().Thanks for any comments and critique.
Changes from
encrypt_fhposting:
https://lore.kernel.org/linux-nfs/[email protected]
- 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 keyChanges from v1 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- removefh_fileid_offset()(Chuck Lever)
- fixpr_warns, fix memcmp (Chuck Lever)
- remove incorrect rootfh comment (NeilBrown)
- makefh_keysetting an optional attr to threads verb (Jeff Layton)
- dropBIT()EXP_flag conversion
- cover-letter tune-ups (NeilBrown, Chuck Lever)
- fixNFSEXP_ALLFLAGSon 2/3
- castfh->fh_size+ sizeof(hash) result to int (avoidx86_64WARNING)
- move MAC signing into__fh_update()(Chuck Lever)Changes from v2 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- more cover-letter detail (NeilBrown)
-Documentation/filesystems/nfs/exporting.rstsection (Jeff Layton)
- fix key copy (Eric Biggers)
- useNFSD_A_SERVER_MAX(NeilBrown)
- remove procfsfh_keyinterface (Chuck Lever)
- removeFH_AT_MAC(Chuck Lever)
- allowfh_keychange when server is not running (Chuck/Jeff)
- acceptfh_keyas netlink attribute instead of command (Jeff Layton)Changes from v3 posting:
https://lore.kernel.org/linux-nfs/[email protected]
- /actually/ fix up endianness problems (Eric Biggers)
- comment typo
- fix Documentation underline warnings
- fix possible uninitializedfh_keyvarBenjamin Coddington (3):
NFSD: Add a key for signing filehandles
NFSD/export: Addsign_fhexport option
NFSD: Sign filehandlesDocumentation/filesystems/nfs/exporting.rst | 85 +++++++++++++++++++++ Documentation/netlink/specs/nfsd.yaml | 6 ++ fs/nfsd/export.c | 5 +- fs/nfsd/netlink.c | 5 +- fs/nfsd/netns.h | 2 + fs/nfsd/nfsctl.c | 37 ++++++++- fs/nfsd/nfsfh.c | 64 +++++++++++++++- fs/nfsd/trace.h | 25 ++++++ include/uapi/linux/nfsd/export.h | 4 +- include/uapi/linux/nfsd_netlink.h | 1 + 10 files changed, 225 insertions(+), 9 deletions(-)
Quoted 1 line
base-commit:
e3934bbd57c73b3835a77562ca47b5fbc6f34287
Nice work, Ben. This looks good to me!
Re: [PATCH v4 1/3] NFSD: Add a key for signing filehandles
In reply to Benjamin CoddingtonQuoted 109 lines · Benjamin Coddington
A future patch will enable NFSD to sign filehandles by appending a Message Authentication Code(MAC). To do this, NFSD requires a secret 128-bit key that can persist across reboots. A persisted key allows the server to accept filehandles after a restart. Enable NFSD to be configured with this key the netlink interface.
Signed-off-by Benjamin Coddington <[email protected]>--- Documentation/netlink/specs/nfsd.yaml | 6 +++++ fs/nfsd/netlink.c | 5 ++-- fs/nfsd/netns.h | 2 ++ fs/nfsd/nfsctl.c | 37 ++++++++++++++++++++++++++- fs/nfsd/trace.h | 25 ++++++++++++++++++ include/uapi/linux/nfsd_netlink.h | 1 + 6 files changed, 73 insertions(+), 3 deletions(-)Documentation/netlink/specs/nfsd.yaml +6
-0@@ -81,6 +81,11 @@ attribute-sets: - name: min-threads type: u32 + - + name: fh-key + type: binary + checks: + exact-len: 16 - name: version attributes:@@ -163,6 +168,7 @@ operations: - leasetime - scope - min-threads + - fh-key - name: threads-get doc: get the maximum number of running threadsfs/nfsd/netlink.c +3
-2@@ -24,12 +24,13 @@ const struct nla_policy nfsd_version_nl_policy[NFSD_A_VERSION_ENABLED + 1] = { };/* NFSD_CMD_THREADS_SET - do */ -static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_MIN_THREADS +1 ] = { +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_FH_KEY +1 ] = { [NFSD_A_SERVER_THREADS ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_GRACETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_LEASETIME ] = { .type =NLA_U32 , }, [NFSD_A_SERVER_SCOPE ] = { .type =NLA_NUL_STRING , }, [NFSD_A_SERVER_MIN_THREADS ] = { .type =NLA_U32 , }, + [NFSD_A_SERVER_FH_KEY ] =NLA_POLICY_EXACT_LEN (16 ), };/* NFSD_CMD_VERSION_SET - do */ @@ -58,7 +59,7 @@ static const struct genl_split_ops nfsd_nl_ops[] = { .cmd = NFSD_CMD_THREADS_SET, .doit = nfsd_nl_threads_set_doit, .policy = nfsd_threads_set_nl_policy, - .maxattr = NFSD_A_SERVER_MIN_THREADS, + .maxattr = NFSD_A_SERVER_MAX, .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, }, {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 +12
-0@@ -1571,6 +1571,31 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,return ret; } +/** +* nfsd_nl_fh_key_set - helper to copy fh_key from userspace +* @attr: nlattr NFSD_A_SERVER_FH_KEY +* @nn: nfsd_net +* +* Callers should hold nfsd_mutex, returns 0 on success or negative errno. +*/ +static int nfsd_nl_fh_key_set (const struct nlattr *attr,struct nfsd_net *nn) +{ +siphash_key_t *fh_key = nn->fh_key; + +if (nla_len (attr) !=sizeof (siphash_key_t))
fs/nfsd/nfsctl.c int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, · line 1595
Quoted 10 lines
fs/nfsd/nfsctl.c +10
-0+return -EINVAL ; + +if (!fh_key) { + fh_key =kmalloc (sizeof (siphash_key_t),GFP_KERNEL ); +if (!fh_key) +return -ENOMEM ; + nn->fh_key = fh_key; + } +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr)); +put_unaligned_le64 (fh_key->key[0 ],nla_data (attr));
Should the second one be [1], or is a comment needs to explain some subtlety?
Otherwise this looks sensible.
NeilBrown
Quoted 91 lines
fs/nfsd/nfsctl.c +14
-1+return 0 ; +} +/** * nfsd_nl_threads_set_doit - set the number of running threads * @skb: reply buffer @@ -1612,7 +1637,8 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)if (info->attrs[NFSD_A_SERVER_GRACETIME ] || info->attrs[NFSD_A_SERVER_LEASETIME ] || - info->attrs[NFSD_A_SERVER_SCOPE ]) { + info->attrs[NFSD_A_SERVER_SCOPE ] || + info->attrs[NFSD_A_SERVER_FH_KEY ]) { ret = -EBUSY ;if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads) goto out_unlock;@@ -1641,6 +1667,14 @@ int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info) attr = info->attrs[NFSD_A_SERVER_SCOPE ];if (attr) scope =nla_data (attr); + + attr = info->attrs[NFSD_A_SERVER_FH_KEY ]; +if (attr) { + ret =nfsd_nl_fh_key_set (attr, nn); +trace_nfsd_ctl_fh_key_set ((const char *)nn->fh_key, ret); +if (ret) + goto out_unlock; + } } attr = info->attrs[NFSD_A_SERVER_MIN_THREADS ];@@ -2240,6 +2274,7 @@ static __net_exit void nfsd_net_exit(struct net *net) {struct nfsd_net *nn =net_generic (net, nfsd_net_id); +kfree_sensitive (nn->fh_key);nfsd_proc_stat_shutdown (net);percpu_counter_destroy_many (nn->counter,NFSD_STATS_COUNTERS_NUM );nfsd_idmap_shutdown (net);fs/nfsd/trace.h +25
-0@@ -2240,6 +2240,31 @@ 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) +__field (bool, key_set) + ), +TP_fast_assign ( + __entry->key_set = true; + if (!key) + __entry->key_set = false; + else + memcpy(__entry->key, key, 16); + __entry->result = result; + ), +TP_printk ("key=%s result=%ld" , + __entry->key_set ?__print_hex_str (__entry->key,16 ) :"(null)" , + __entry->result + ) +); + DECLARE_EVENT_CLASS(nfsd_copy_class, TP_PROTO( const struct nfsd4_copy *copyinclude/uapi/linux/nfsd_netlink.h +1
-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 )Signature · 1 line
2.50.1