* [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares @ 2021-04-30 22:19 Steve French 2021-05-01 4:00 ` ronnie sahlberg 2021-05-07 0:17 ` Stefan Metzmacher 0 siblings, 2 replies; 16+ messages in thread From: Steve French @ 2021-04-30 22:19 UTC (permalink / raw) To: CIFS; +Cc: samba-technical, linux-cifsd-devel [-- Attachment #1: Type: text/plain, Size: 3737 bytes --] Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo keys" e.g.) to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted shares. But with the addition of GCM-256 support, we have to be able to dump 32 byte instead of 16 byte keys which requires adding an additional ioctl for that. Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h index f262c64516bc..9f2ed9cccb08 100644 --- a/fs/cifs/cifs_ioctl.h +++ b/fs/cifs/cifs_ioctl.h @@ -57,6 +57,12 @@ struct smb_query_info { /* char buffer[]; */ } __packed; +/* + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported + * for backlevel compatibility, but is not sufficient for dumping the less + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" + * ioctl for dumping decryption info for GCM256 mounts) + */ struct smb3_key_debug_info { __u64 Suid; __u16 cipher_type; @@ -65,6 +71,18 @@ struct smb3_key_debug_info { __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; } __packed; +/* + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) + * is needed if GCM256 (stronger encryption) negotiated + */ +struct smb3_full_key_debug_info { + __u64 Suid; + __u16 cipher_type; + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; +} __packed; + struct smb3_notify { __u32 completion_filter; bool watch_tree; @@ -78,6 +96,7 @@ struct smb3_notify { #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct smb3_full_key_debug_info) #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) /* diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c index ef41fa878793..e4321e2a27d2 100644 --- a/fs/cifs/ioctl.c +++ b/fs/cifs/ioctl.c @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) { struct inode *inode = file_inode(filep); struct smb3_key_debug_info pkey_inf; + struct smb3_full_key_debug_info pfull_key_inf; int rc = -ENOTTY; /* strange error - but the precedent */ unsigned int xid; struct cifsFileInfo *pSMBFile = filep->private_data; @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) else rc = 0; break; + /* + * Dump full key (32 bytes instead of 16 bytes) is + * needed if GCM256 (stronger encryption) negotiated + */ + case CIFS_DUMP_FULL_KEY: + if (pSMBFile == NULL) + break; + if (!capable(CAP_SYS_ADMIN)) { + rc = -EACCES; + break; + } + + tcon = tlink_tcon(pSMBFile->tlink); + if (!smb3_encryption_required(tcon)) { + rc = -EOPNOTSUPP; + break; + } + pfull_key_inf.cipher_type = + le16_to_cpu(tcon->ses->server->cipher_type); + pfull_key_inf.Suid = tcon->ses->Suid; + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); + memcpy(pfull_key_inf.smb3decryptionkey, + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); + memcpy(pfull_key_inf.smb3encryptionkey, + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); + if (copy_to_user((void __user *)arg, &pfull_key_inf, + sizeof(struct smb3_full_key_debug_info))) + rc = -EFAULT; + else + rc = 0; + break; case CIFS_IOC_NOTIFY: if (!S_ISDIR(inode->i_mode)) { /* Notify can only be done on directories */ -- Thanks, Steve [-- Attachment #2: 0001-smb3.1.1-allow-dumping-GCM256-keys-to-improve-debugg.patch --] [-- Type: text/x-patch, Size: 4066 bytes --] From a51a2ea0f8d72f0c3e691f253bb2b48db69cb035 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Fri, 30 Apr 2021 17:14:45 -0500 Subject: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo keys" e.g.) to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted shares. But with the addition of GCM-256 support, we have to be able to dump 32 byte instead of 16 byte keys which requires adding an additional ioctl for that. Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h index f262c64516bc..9f2ed9cccb08 100644 --- a/fs/cifs/cifs_ioctl.h +++ b/fs/cifs/cifs_ioctl.h @@ -57,6 +57,12 @@ struct smb_query_info { /* char buffer[]; */ } __packed; +/* + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported + * for backlevel compatibility, but is not sufficient for dumping the less + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" + * ioctl for dumping decryption info for GCM256 mounts) + */ struct smb3_key_debug_info { __u64 Suid; __u16 cipher_type; @@ -65,6 +71,18 @@ struct smb3_key_debug_info { __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; } __packed; +/* + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) + * is needed if GCM256 (stronger encryption) negotiated + */ +struct smb3_full_key_debug_info { + __u64 Suid; + __u16 cipher_type; + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; +} __packed; + struct smb3_notify { __u32 completion_filter; bool watch_tree; @@ -78,6 +96,7 @@ struct smb3_notify { #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct smb3_full_key_debug_info) #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) /* diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c index ef41fa878793..e4321e2a27d2 100644 --- a/fs/cifs/ioctl.c +++ b/fs/cifs/ioctl.c @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) { struct inode *inode = file_inode(filep); struct smb3_key_debug_info pkey_inf; + struct smb3_full_key_debug_info pfull_key_inf; int rc = -ENOTTY; /* strange error - but the precedent */ unsigned int xid; struct cifsFileInfo *pSMBFile = filep->private_data; @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) else rc = 0; break; + /* + * Dump full key (32 bytes instead of 16 bytes) is + * needed if GCM256 (stronger encryption) negotiated + */ + case CIFS_DUMP_FULL_KEY: + if (pSMBFile == NULL) + break; + if (!capable(CAP_SYS_ADMIN)) { + rc = -EACCES; + break; + } + + tcon = tlink_tcon(pSMBFile->tlink); + if (!smb3_encryption_required(tcon)) { + rc = -EOPNOTSUPP; + break; + } + pfull_key_inf.cipher_type = + le16_to_cpu(tcon->ses->server->cipher_type); + pfull_key_inf.Suid = tcon->ses->Suid; + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); + memcpy(pfull_key_inf.smb3decryptionkey, + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); + memcpy(pfull_key_inf.smb3encryptionkey, + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); + if (copy_to_user((void __user *)arg, &pfull_key_inf, + sizeof(struct smb3_full_key_debug_info))) + rc = -EFAULT; + else + rc = 0; + break; case CIFS_IOC_NOTIFY: if (!S_ISDIR(inode->i_mode)) { /* Notify can only be done on directories */ -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-04-30 22:19 [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares Steve French @ 2021-05-01 4:00 ` ronnie sahlberg 2021-05-01 4:17 ` Steve French 2021-05-07 0:17 ` Stefan Metzmacher 1 sibling, 1 reply; 16+ messages in thread From: ronnie sahlberg @ 2021-05-01 4:00 UTC (permalink / raw) To: Steve French; +Cc: CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER These elements should probably be [32] and not + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; Because this is now visible to userspace and we can not allow this to ever change. Because when GCM512 is eventually released, if we bump SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > keys" e.g.) > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > shares. But with the addition of GCM-256 support, we have to be able to dump > 32 byte instead of 16 byte keys which requires adding an additional ioctl > for that. > > Signed-off-by: Steve French <stfrench@microsoft.com> > --- > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > index f262c64516bc..9f2ed9cccb08 100644 > --- a/fs/cifs/cifs_ioctl.h > +++ b/fs/cifs/cifs_ioctl.h > @@ -57,6 +57,12 @@ struct smb_query_info { > /* char buffer[]; */ > } __packed; > > +/* > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > + * for backlevel compatibility, but is not sufficient for dumping the less > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > + * ioctl for dumping decryption info for GCM256 mounts) > + */ > struct smb3_key_debug_info { > __u64 Suid; > __u16 cipher_type; > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > } __packed; > > +/* > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > + * is needed if GCM256 (stronger encryption) negotiated > + */ > +struct smb3_full_key_debug_info { > + __u64 Suid; > + __u16 cipher_type; > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > +} __packed; > + > struct smb3_notify { > __u32 completion_filter; > bool watch_tree; > @@ -78,6 +96,7 @@ struct smb3_notify { > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > smb3_full_key_debug_info) > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > /* > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > index ef41fa878793..e4321e2a27d2 100644 > --- a/fs/cifs/ioctl.c > +++ b/fs/cifs/ioctl.c > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > command, unsigned long arg) > { > struct inode *inode = file_inode(filep); > struct smb3_key_debug_info pkey_inf; > + struct smb3_full_key_debug_info pfull_key_inf; > int rc = -ENOTTY; /* strange error - but the precedent */ > unsigned int xid; > struct cifsFileInfo *pSMBFile = filep->private_data; > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > command, unsigned long arg) > else > rc = 0; > break; > + /* > + * Dump full key (32 bytes instead of 16 bytes) is > + * needed if GCM256 (stronger encryption) negotiated > + */ > + case CIFS_DUMP_FULL_KEY: > + if (pSMBFile == NULL) > + break; > + if (!capable(CAP_SYS_ADMIN)) { > + rc = -EACCES; > + break; > + } > + > + tcon = tlink_tcon(pSMBFile->tlink); > + if (!smb3_encryption_required(tcon)) { > + rc = -EOPNOTSUPP; > + break; > + } > + pfull_key_inf.cipher_type = > + le16_to_cpu(tcon->ses->server->cipher_type); > + pfull_key_inf.Suid = tcon->ses->Suid; > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > + memcpy(pfull_key_inf.smb3decryptionkey, > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > + memcpy(pfull_key_inf.smb3encryptionkey, > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > + sizeof(struct smb3_full_key_debug_info))) > + rc = -EFAULT; > + else > + rc = 0; > + break; > case CIFS_IOC_NOTIFY: > if (!S_ISDIR(inode->i_mode)) { > /* Notify can only be done on directories */ > > -- > Thanks, > > Steve ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-01 4:00 ` ronnie sahlberg @ 2021-05-01 4:17 ` Steve French 2021-05-01 10:53 ` Shyam Prasad N 0 siblings, 1 reply; 16+ messages in thread From: Steve French @ 2021-05-01 4:17 UTC (permalink / raw) To: ronnie sahlberg; +Cc: CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER [-- Attachment #1: Type: text/plain, Size: 4736 bytes --] changed as suggested - see attached On Fri, Apr 30, 2021 at 11:00 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > These elements should probably be [32] and not > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > Because this is now visible to userspace and we can not allow this to > ever change. > Because when GCM512 is eventually released, if we bump > SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. > > > On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > > keys" e.g.) > > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > > shares. But with the addition of GCM-256 support, we have to be able to dump > > 32 byte instead of 16 byte keys which requires adding an additional ioctl > > for that. > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > --- > > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > > 2 files changed, 52 insertions(+) > > > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > > index f262c64516bc..9f2ed9cccb08 100644 > > --- a/fs/cifs/cifs_ioctl.h > > +++ b/fs/cifs/cifs_ioctl.h > > @@ -57,6 +57,12 @@ struct smb_query_info { > > /* char buffer[]; */ > > } __packed; > > > > +/* > > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > > + * for backlevel compatibility, but is not sufficient for dumping the less > > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > > + * ioctl for dumping decryption info for GCM256 mounts) > > + */ > > struct smb3_key_debug_info { > > __u64 Suid; > > __u16 cipher_type; > > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > > } __packed; > > > > +/* > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > + * is needed if GCM256 (stronger encryption) negotiated > > + */ > > +struct smb3_full_key_debug_info { > > + __u64 Suid; > > + __u16 cipher_type; > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > +} __packed; > > + > > struct smb3_notify { > > __u32 completion_filter; > > bool watch_tree; > > @@ -78,6 +96,7 @@ struct smb3_notify { > > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > > smb3_full_key_debug_info) > > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > /* > > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > > index ef41fa878793..e4321e2a27d2 100644 > > --- a/fs/cifs/ioctl.c > > +++ b/fs/cifs/ioctl.c > > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > > command, unsigned long arg) > > { > > struct inode *inode = file_inode(filep); > > struct smb3_key_debug_info pkey_inf; > > + struct smb3_full_key_debug_info pfull_key_inf; > > int rc = -ENOTTY; /* strange error - but the precedent */ > > unsigned int xid; > > struct cifsFileInfo *pSMBFile = filep->private_data; > > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > > command, unsigned long arg) > > else > > rc = 0; > > break; > > + /* > > + * Dump full key (32 bytes instead of 16 bytes) is > > + * needed if GCM256 (stronger encryption) negotiated > > + */ > > + case CIFS_DUMP_FULL_KEY: > > + if (pSMBFile == NULL) > > + break; > > + if (!capable(CAP_SYS_ADMIN)) { > > + rc = -EACCES; > > + break; > > + } > > + > > + tcon = tlink_tcon(pSMBFile->tlink); > > + if (!smb3_encryption_required(tcon)) { > > + rc = -EOPNOTSUPP; > > + break; > > + } > > + pfull_key_inf.cipher_type = > > + le16_to_cpu(tcon->ses->server->cipher_type); > > + pfull_key_inf.Suid = tcon->ses->Suid; > > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > > + memcpy(pfull_key_inf.smb3decryptionkey, > > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > + memcpy(pfull_key_inf.smb3encryptionkey, > > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > > + sizeof(struct smb3_full_key_debug_info))) > > + rc = -EFAULT; > > + else > > + rc = 0; > > + break; > > case CIFS_IOC_NOTIFY: > > if (!S_ISDIR(inode->i_mode)) { > > /* Notify can only be done on directories */ > > > > -- > > Thanks, > > > > Steve -- Thanks, Steve [-- Attachment #2: 0001-smb3.1.1-allow-dumping-GCM256-keys-to-improve-debugg.patch --] [-- Type: text/x-patch, Size: 4106 bytes --] From a51a2ea0f8d72f0c3e691f253bb2b48db69cb035 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Fri, 30 Apr 2021 17:14:45 -0500 Subject: [PATCH 1/2] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo keys" e.g.) to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted shares. But with the addition of GCM-256 support, we have to be able to dump 32 byte instead of 16 byte keys which requires adding an additional ioctl for that. Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h index f262c64516bc..9f2ed9cccb08 100644 --- a/fs/cifs/cifs_ioctl.h +++ b/fs/cifs/cifs_ioctl.h @@ -57,6 +57,12 @@ struct smb_query_info { /* char buffer[]; */ } __packed; +/* + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported + * for backlevel compatibility, but is not sufficient for dumping the less + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" + * ioctl for dumping decryption info for GCM256 mounts) + */ struct smb3_key_debug_info { __u64 Suid; __u16 cipher_type; @@ -65,6 +71,18 @@ struct smb3_key_debug_info { __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; } __packed; +/* + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) + * is needed if GCM256 (stronger encryption) negotiated + */ +struct smb3_full_key_debug_info { + __u64 Suid; + __u16 cipher_type; + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ + __u8 smb3encryptionkey[32]; /* SMB3_ENC_DEC_KEY_SIZE */ + __u8 smb3decryptionkey[32]; /* SMB3_ENC_DEC_KEY_SIZE */ +} __packed; + struct smb3_notify { __u32 completion_filter; bool watch_tree; @@ -78,6 +96,7 @@ struct smb3_notify { #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct smb3_full_key_debug_info) #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) /* diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c index ef41fa878793..e4321e2a27d2 100644 --- a/fs/cifs/ioctl.c +++ b/fs/cifs/ioctl.c @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) { struct inode *inode = file_inode(filep); struct smb3_key_debug_info pkey_inf; + struct smb3_full_key_debug_info pfull_key_inf; int rc = -ENOTTY; /* strange error - but the precedent */ unsigned int xid; struct cifsFileInfo *pSMBFile = filep->private_data; @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) else rc = 0; break; + /* + * Dump full key (32 bytes instead of 16 bytes) is + * needed if GCM256 (stronger encryption) negotiated + */ + case CIFS_DUMP_FULL_KEY: + if (pSMBFile == NULL) + break; + if (!capable(CAP_SYS_ADMIN)) { + rc = -EACCES; + break; + } + + tcon = tlink_tcon(pSMBFile->tlink); + if (!smb3_encryption_required(tcon)) { + rc = -EOPNOTSUPP; + break; + } + pfull_key_inf.cipher_type = + le16_to_cpu(tcon->ses->server->cipher_type); + pfull_key_inf.Suid = tcon->ses->Suid; + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); + memcpy(pfull_key_inf.smb3decryptionkey, + tcon->ses->smb3decryptionkey, 32 /* SMB3_ENC_DEC_KEY_SIZE */); + memcpy(pfull_key_inf.smb3encryptionkey, + tcon->ses->smb3encryptionkey, 32 /* SMB3_ENC_DEC_KEY_SIZE */); + if (copy_to_user((void __user *)arg, &pfull_key_inf, + sizeof(struct smb3_full_key_debug_info))) + rc = -EFAULT; + else + rc = 0; + break; case CIFS_IOC_NOTIFY: if (!S_ISDIR(inode->i_mode)) { /* Notify can only be done on directories */ -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-01 4:17 ` Steve French @ 2021-05-01 10:53 ` Shyam Prasad N 2021-05-01 20:49 ` ronnie sahlberg 0 siblings, 1 reply; 16+ messages in thread From: Shyam Prasad N @ 2021-05-01 10:53 UTC (permalink / raw) To: Steve French Cc: ronnie sahlberg, CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER Looks good to me. On a related note, we need a way for the root user to dump keys for another SMB session to the same path. This will be useful for mutli-user scenario. i.e. for dumping keys for SMB session as another user. Since we're adding a new IOCTL, perhaps we should add another arg which identifies the user? Maybe based on the UID:GID of the user session, in addition to the path supplied? Regards, Shyam On Sat, May 1, 2021 at 9:49 AM Steve French <smfrench@gmail.com> wrote: > > changed as suggested - see attached > > On Fri, Apr 30, 2021 at 11:00 PM ronnie sahlberg > <ronniesahlberg@gmail.com> wrote: > > > > These elements should probably be [32] and not > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > Because this is now visible to userspace and we can not allow this to > > ever change. > > Because when GCM512 is eventually released, if we bump > > SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. > > > > > > On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > > > > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > > > keys" e.g.) > > > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > > > shares. But with the addition of GCM-256 support, we have to be able to dump > > > 32 byte instead of 16 byte keys which requires adding an additional ioctl > > > for that. > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > --- > > > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > > > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > > > 2 files changed, 52 insertions(+) > > > > > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > > > index f262c64516bc..9f2ed9cccb08 100644 > > > --- a/fs/cifs/cifs_ioctl.h > > > +++ b/fs/cifs/cifs_ioctl.h > > > @@ -57,6 +57,12 @@ struct smb_query_info { > > > /* char buffer[]; */ > > > } __packed; > > > > > > +/* > > > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > > > + * for backlevel compatibility, but is not sufficient for dumping the less > > > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > > > + * ioctl for dumping decryption info for GCM256 mounts) > > > + */ > > > struct smb3_key_debug_info { > > > __u64 Suid; > > > __u16 cipher_type; > > > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > > > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > > > } __packed; > > > > > > +/* > > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > > + * is needed if GCM256 (stronger encryption) negotiated > > > + */ > > > +struct smb3_full_key_debug_info { > > > + __u64 Suid; > > > + __u16 cipher_type; > > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > +} __packed; > > > + > > > struct smb3_notify { > > > __u32 completion_filter; > > > bool watch_tree; > > > @@ -78,6 +96,7 @@ struct smb3_notify { > > > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > > > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > > > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > > > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > > > smb3_full_key_debug_info) > > > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > > > /* > > > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > > > index ef41fa878793..e4321e2a27d2 100644 > > > --- a/fs/cifs/ioctl.c > > > +++ b/fs/cifs/ioctl.c > > > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > > > command, unsigned long arg) > > > { > > > struct inode *inode = file_inode(filep); > > > struct smb3_key_debug_info pkey_inf; > > > + struct smb3_full_key_debug_info pfull_key_inf; > > > int rc = -ENOTTY; /* strange error - but the precedent */ > > > unsigned int xid; > > > struct cifsFileInfo *pSMBFile = filep->private_data; > > > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > > > command, unsigned long arg) > > > else > > > rc = 0; > > > break; > > > + /* > > > + * Dump full key (32 bytes instead of 16 bytes) is > > > + * needed if GCM256 (stronger encryption) negotiated > > > + */ > > > + case CIFS_DUMP_FULL_KEY: > > > + if (pSMBFile == NULL) > > > + break; > > > + if (!capable(CAP_SYS_ADMIN)) { > > > + rc = -EACCES; > > > + break; > > > + } > > > + > > > + tcon = tlink_tcon(pSMBFile->tlink); > > > + if (!smb3_encryption_required(tcon)) { > > > + rc = -EOPNOTSUPP; > > > + break; > > > + } > > > + pfull_key_inf.cipher_type = > > > + le16_to_cpu(tcon->ses->server->cipher_type); > > > + pfull_key_inf.Suid = tcon->ses->Suid; > > > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > > > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > > > + memcpy(pfull_key_inf.smb3decryptionkey, > > > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > + memcpy(pfull_key_inf.smb3encryptionkey, > > > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > > > + sizeof(struct smb3_full_key_debug_info))) > > > + rc = -EFAULT; > > > + else > > > + rc = 0; > > > + break; > > > case CIFS_IOC_NOTIFY: > > > if (!S_ISDIR(inode->i_mode)) { > > > /* Notify can only be done on directories */ > > > > > > -- > > > Thanks, > > > > > > Steve > > > > -- > Thanks, > > Steve -- Regards, Shyam ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-01 10:53 ` Shyam Prasad N @ 2021-05-01 20:49 ` ronnie sahlberg 2021-05-02 4:05 ` Steve French 0 siblings, 1 reply; 16+ messages in thread From: ronnie sahlberg @ 2021-05-01 20:49 UTC (permalink / raw) To: Shyam Prasad N Cc: Steve French, CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER On Sat, May 1, 2021 at 8:53 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > Looks good to me. > > On a related note, we need a way for the root user to dump keys for > another SMB session to the same path. This will be useful for > mutli-user scenario. > i.e. for dumping keys for SMB session as another user. > Since we're adding a new IOCTL, perhaps we should add another arg > which identifies the user? Maybe based on the UID:GID of the user > session, in addition to the path supplied? Or as an alternative, dump an array of ALL user sessions with information about which user and which part of a multi-channel connection that the keys belong to. And let userspace sort out "which keys do I need for my wireshark session". > > Regards, > Shyam > > On Sat, May 1, 2021 at 9:49 AM Steve French <smfrench@gmail.com> wrote: > > > > changed as suggested - see attached > > > > On Fri, Apr 30, 2021 at 11:00 PM ronnie sahlberg > > <ronniesahlberg@gmail.com> wrote: > > > > > > These elements should probably be [32] and not > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > > Because this is now visible to userspace and we can not allow this to > > > ever change. > > > Because when GCM512 is eventually released, if we bump > > > SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. > > > > > > > > > On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > > > > keys" e.g.) > > > > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > > > > shares. But with the addition of GCM-256 support, we have to be able to dump > > > > 32 byte instead of 16 byte keys which requires adding an additional ioctl > > > > for that. > > > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > > --- > > > > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > > > > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > > > > 2 files changed, 52 insertions(+) > > > > > > > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > > > > index f262c64516bc..9f2ed9cccb08 100644 > > > > --- a/fs/cifs/cifs_ioctl.h > > > > +++ b/fs/cifs/cifs_ioctl.h > > > > @@ -57,6 +57,12 @@ struct smb_query_info { > > > > /* char buffer[]; */ > > > > } __packed; > > > > > > > > +/* > > > > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > > > > + * for backlevel compatibility, but is not sufficient for dumping the less > > > > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > > > > + * ioctl for dumping decryption info for GCM256 mounts) > > > > + */ > > > > struct smb3_key_debug_info { > > > > __u64 Suid; > > > > __u16 cipher_type; > > > > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > > > > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > > > > } __packed; > > > > > > > > +/* > > > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > > > + * is needed if GCM256 (stronger encryption) negotiated > > > > + */ > > > > +struct smb3_full_key_debug_info { > > > > + __u64 Suid; > > > > + __u16 cipher_type; > > > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > +} __packed; > > > > + > > > > struct smb3_notify { > > > > __u32 completion_filter; > > > > bool watch_tree; > > > > @@ -78,6 +96,7 @@ struct smb3_notify { > > > > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > > > > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > > > > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > > > > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > > > > smb3_full_key_debug_info) > > > > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > > > > > /* > > > > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > > > > index ef41fa878793..e4321e2a27d2 100644 > > > > --- a/fs/cifs/ioctl.c > > > > +++ b/fs/cifs/ioctl.c > > > > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > command, unsigned long arg) > > > > { > > > > struct inode *inode = file_inode(filep); > > > > struct smb3_key_debug_info pkey_inf; > > > > + struct smb3_full_key_debug_info pfull_key_inf; > > > > int rc = -ENOTTY; /* strange error - but the precedent */ > > > > unsigned int xid; > > > > struct cifsFileInfo *pSMBFile = filep->private_data; > > > > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > command, unsigned long arg) > > > > else > > > > rc = 0; > > > > break; > > > > + /* > > > > + * Dump full key (32 bytes instead of 16 bytes) is > > > > + * needed if GCM256 (stronger encryption) negotiated > > > > + */ > > > > + case CIFS_DUMP_FULL_KEY: > > > > + if (pSMBFile == NULL) > > > > + break; > > > > + if (!capable(CAP_SYS_ADMIN)) { > > > > + rc = -EACCES; > > > > + break; > > > > + } > > > > + > > > > + tcon = tlink_tcon(pSMBFile->tlink); > > > > + if (!smb3_encryption_required(tcon)) { > > > > + rc = -EOPNOTSUPP; > > > > + break; > > > > + } > > > > + pfull_key_inf.cipher_type = > > > > + le16_to_cpu(tcon->ses->server->cipher_type); > > > > + pfull_key_inf.Suid = tcon->ses->Suid; > > > > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > > > > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > > > > + memcpy(pfull_key_inf.smb3decryptionkey, > > > > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > + memcpy(pfull_key_inf.smb3encryptionkey, > > > > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > > > > + sizeof(struct smb3_full_key_debug_info))) > > > > + rc = -EFAULT; > > > > + else > > > > + rc = 0; > > > > + break; > > > > case CIFS_IOC_NOTIFY: > > > > if (!S_ISDIR(inode->i_mode)) { > > > > /* Notify can only be done on directories */ > > > > > > > > -- > > > > Thanks, > > > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > Regards, > Shyam ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-01 20:49 ` ronnie sahlberg @ 2021-05-02 4:05 ` Steve French 2021-05-02 23:02 ` Steve French 0 siblings, 1 reply; 16+ messages in thread From: Steve French @ 2021-05-02 4:05 UTC (permalink / raw) To: ronnie sahlberg Cc: Shyam Prasad N, CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER I think it is reasonably easy to read in an optional SUID (smb session uid) as a parm on the new "DUMP_FULL_KEY" ioctl - less code to add in the followon patch. Will spin something up later tonight On Sat, May 1, 2021 at 3:49 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > On Sat, May 1, 2021 at 8:53 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > Looks good to me. > > > > On a related note, we need a way for the root user to dump keys for > > another SMB session to the same path. This will be useful for > > mutli-user scenario. > > i.e. for dumping keys for SMB session as another user. > > Since we're adding a new IOCTL, perhaps we should add another arg > > which identifies the user? Maybe based on the UID:GID of the user > > session, in addition to the path supplied? > > Or as an alternative, dump an array of ALL user sessions with > information about which user and which part of a multi-channel > connection that the keys belong to. > And let userspace sort out "which keys do I need for my wireshark session". > > > > > Regards, > > Shyam > > > > On Sat, May 1, 2021 at 9:49 AM Steve French <smfrench@gmail.com> wrote: > > > > > > changed as suggested - see attached > > > > > > On Fri, Apr 30, 2021 at 11:00 PM ronnie sahlberg > > > <ronniesahlberg@gmail.com> wrote: > > > > > > > > These elements should probably be [32] and not > > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > > > > Because this is now visible to userspace and we can not allow this to > > > > ever change. > > > > Because when GCM512 is eventually released, if we bump > > > > SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. > > > > > > > > > > > > On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > > > > > keys" e.g.) > > > > > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > > > > > shares. But with the addition of GCM-256 support, we have to be able to dump > > > > > 32 byte instead of 16 byte keys which requires adding an additional ioctl > > > > > for that. > > > > > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > > > --- > > > > > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > > > > > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > > > > > index f262c64516bc..9f2ed9cccb08 100644 > > > > > --- a/fs/cifs/cifs_ioctl.h > > > > > +++ b/fs/cifs/cifs_ioctl.h > > > > > @@ -57,6 +57,12 @@ struct smb_query_info { > > > > > /* char buffer[]; */ > > > > > } __packed; > > > > > > > > > > +/* > > > > > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > > > > > + * for backlevel compatibility, but is not sufficient for dumping the less > > > > > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > > > > > + * ioctl for dumping decryption info for GCM256 mounts) > > > > > + */ > > > > > struct smb3_key_debug_info { > > > > > __u64 Suid; > > > > > __u16 cipher_type; > > > > > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > > > > > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > > > > > } __packed; > > > > > > > > > > +/* > > > > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > > > > + * is needed if GCM256 (stronger encryption) negotiated > > > > > + */ > > > > > +struct smb3_full_key_debug_info { > > > > > + __u64 Suid; > > > > > + __u16 cipher_type; > > > > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > +} __packed; > > > > > + > > > > > struct smb3_notify { > > > > > __u32 completion_filter; > > > > > bool watch_tree; > > > > > @@ -78,6 +96,7 @@ struct smb3_notify { > > > > > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > > > > > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > > > > > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > > > > > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > > > > > smb3_full_key_debug_info) > > > > > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > > > > > > > /* > > > > > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > > > > > index ef41fa878793..e4321e2a27d2 100644 > > > > > --- a/fs/cifs/ioctl.c > > > > > +++ b/fs/cifs/ioctl.c > > > > > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > > command, unsigned long arg) > > > > > { > > > > > struct inode *inode = file_inode(filep); > > > > > struct smb3_key_debug_info pkey_inf; > > > > > + struct smb3_full_key_debug_info pfull_key_inf; > > > > > int rc = -ENOTTY; /* strange error - but the precedent */ > > > > > unsigned int xid; > > > > > struct cifsFileInfo *pSMBFile = filep->private_data; > > > > > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > > command, unsigned long arg) > > > > > else > > > > > rc = 0; > > > > > break; > > > > > + /* > > > > > + * Dump full key (32 bytes instead of 16 bytes) is > > > > > + * needed if GCM256 (stronger encryption) negotiated > > > > > + */ > > > > > + case CIFS_DUMP_FULL_KEY: > > > > > + if (pSMBFile == NULL) > > > > > + break; > > > > > + if (!capable(CAP_SYS_ADMIN)) { > > > > > + rc = -EACCES; > > > > > + break; > > > > > + } > > > > > + > > > > > + tcon = tlink_tcon(pSMBFile->tlink); > > > > > + if (!smb3_encryption_required(tcon)) { > > > > > + rc = -EOPNOTSUPP; > > > > > + break; > > > > > + } > > > > > + pfull_key_inf.cipher_type = > > > > > + le16_to_cpu(tcon->ses->server->cipher_type); > > > > > + pfull_key_inf.Suid = tcon->ses->Suid; > > > > > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > > > > > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > > > > > + memcpy(pfull_key_inf.smb3decryptionkey, > > > > > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > > + memcpy(pfull_key_inf.smb3encryptionkey, > > > > > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > > > > > + sizeof(struct smb3_full_key_debug_info))) > > > > > + rc = -EFAULT; > > > > > + else > > > > > + rc = 0; > > > > > + break; > > > > > case CIFS_IOC_NOTIFY: > > > > > if (!S_ISDIR(inode->i_mode)) { > > > > > /* Notify can only be done on directories */ > > > > > > > > > > -- > > > > > Thanks, > > > > > > > > > > Steve > > > > > > > > > > > > -- > > > Thanks, > > > > > > Steve > > > > > > > > -- > > Regards, > > Shyam -- Thanks, Steve ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-02 4:05 ` Steve French @ 2021-05-02 23:02 ` Steve French 2021-05-07 13:48 ` Aurélien Aptel 0 siblings, 1 reply; 16+ messages in thread From: Steve French @ 2021-05-02 23:02 UTC (permalink / raw) To: ronnie sahlberg Cc: Shyam Prasad N, CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER [-- Attachment #1: Type: text/plain, Size: 7389 bytes --] How about something like this to allow optional passing in of uid (which you can get from /proc/fs/cifs) to dump keys for the multiuser case? On Sat, May 1, 2021 at 11:05 PM Steve French <smfrench@gmail.com> wrote: > > I think it is reasonably easy to read in an optional SUID (smb session > uid) as a parm on the new "DUMP_FULL_KEY" ioctl - less code to add in > the followon patch. Will spin something up later tonight > > On Sat, May 1, 2021 at 3:49 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > > > On Sat, May 1, 2021 at 8:53 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > Looks good to me. > > > > > > On a related note, we need a way for the root user to dump keys for > > > another SMB session to the same path. This will be useful for > > > mutli-user scenario. > > > i.e. for dumping keys for SMB session as another user. > > > Since we're adding a new IOCTL, perhaps we should add another arg > > > which identifies the user? Maybe based on the UID:GID of the user > > > session, in addition to the path supplied? > > > > Or as an alternative, dump an array of ALL user sessions with > > information about which user and which part of a multi-channel > > connection that the keys belong to. > > And let userspace sort out "which keys do I need for my wireshark session". > > > > > > > > Regards, > > > Shyam > > > > > > On Sat, May 1, 2021 at 9:49 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > changed as suggested - see attached > > > > > > > > On Fri, Apr 30, 2021 at 11:00 PM ronnie sahlberg > > > > <ronniesahlberg@gmail.com> wrote: > > > > > > > > > > These elements should probably be [32] and not > > > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > > > > > > Because this is now visible to userspace and we can not allow this to > > > > > ever change. > > > > > Because when GCM512 is eventually released, if we bump > > > > > SMB3_ENC_DEC_KEY_SIZE to a larger value we suddenly break userspace. > > > > > > > > > > > > > > > On Sat, May 1, 2021 at 8:20 AM Steve French <smfrench@gmail.com> wrote: > > > > > > > > > > > > Previously we were only able to dump CCM or GCM-128 keys (see "smbinfo > > > > > > keys" e.g.) > > > > > > to allow network debugging (e.g. wireshark) of mounts to SMB3.1.1 encrypted > > > > > > shares. But with the addition of GCM-256 support, we have to be able to dump > > > > > > 32 byte instead of 16 byte keys which requires adding an additional ioctl > > > > > > for that. > > > > > > > > > > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > > > > --- > > > > > > fs/cifs/cifs_ioctl.h | 19 +++++++++++++++++++ > > > > > > fs/cifs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ > > > > > > 2 files changed, 52 insertions(+) > > > > > > > > > > > > diff --git a/fs/cifs/cifs_ioctl.h b/fs/cifs/cifs_ioctl.h > > > > > > index f262c64516bc..9f2ed9cccb08 100644 > > > > > > --- a/fs/cifs/cifs_ioctl.h > > > > > > +++ b/fs/cifs/cifs_ioctl.h > > > > > > @@ -57,6 +57,12 @@ struct smb_query_info { > > > > > > /* char buffer[]; */ > > > > > > } __packed; > > > > > > > > > > > > +/* > > > > > > + * Dumping the commonly used 16 byte (e.g. CCM and GCM128) keys still supported > > > > > > + * for backlevel compatibility, but is not sufficient for dumping the less > > > > > > + * frequently used GCM256 (32 byte) keys (see the newer "CIFS_DUMP_FULL_KEY" > > > > > > + * ioctl for dumping decryption info for GCM256 mounts) > > > > > > + */ > > > > > > struct smb3_key_debug_info { > > > > > > __u64 Suid; > > > > > > __u16 cipher_type; > > > > > > @@ -65,6 +71,18 @@ struct smb3_key_debug_info { > > > > > > __u8 smb3decryptionkey[SMB3_SIGN_KEY_SIZE]; > > > > > > } __packed; > > > > > > > > > > > > +/* > > > > > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > > > > > + * is needed if GCM256 (stronger encryption) negotiated > > > > > > + */ > > > > > > +struct smb3_full_key_debug_info { > > > > > > + __u64 Suid; > > > > > > + __u16 cipher_type; > > > > > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > > > > > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > > > > > > +} __packed; > > > > > > + > > > > > > struct smb3_notify { > > > > > > __u32 completion_filter; > > > > > > bool watch_tree; > > > > > > @@ -78,6 +96,7 @@ struct smb3_notify { > > > > > > #define CIFS_QUERY_INFO _IOWR(CIFS_IOCTL_MAGIC, 7, struct smb_query_info) > > > > > > #define CIFS_DUMP_KEY _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info) > > > > > > #define CIFS_IOC_NOTIFY _IOW(CIFS_IOCTL_MAGIC, 9, struct smb3_notify) > > > > > > +#define CIFS_DUMP_FULL_KEY _IOWR(CIFS_IOCTL_MAGIC, 10, struct > > > > > > smb3_full_key_debug_info) > > > > > > #define CIFS_IOC_SHUTDOWN _IOR ('X', 125, __u32) > > > > > > > > > > > > /* > > > > > > diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > > > > > > index ef41fa878793..e4321e2a27d2 100644 > > > > > > --- a/fs/cifs/ioctl.c > > > > > > +++ b/fs/cifs/ioctl.c > > > > > > @@ -218,6 +218,7 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > > > command, unsigned long arg) > > > > > > { > > > > > > struct inode *inode = file_inode(filep); > > > > > > struct smb3_key_debug_info pkey_inf; > > > > > > + struct smb3_full_key_debug_info pfull_key_inf; > > > > > > int rc = -ENOTTY; /* strange error - but the precedent */ > > > > > > unsigned int xid; > > > > > > struct cifsFileInfo *pSMBFile = filep->private_data; > > > > > > @@ -354,6 +355,38 @@ long cifs_ioctl(struct file *filep, unsigned int > > > > > > command, unsigned long arg) > > > > > > else > > > > > > rc = 0; > > > > > > break; > > > > > > + /* > > > > > > + * Dump full key (32 bytes instead of 16 bytes) is > > > > > > + * needed if GCM256 (stronger encryption) negotiated > > > > > > + */ > > > > > > + case CIFS_DUMP_FULL_KEY: > > > > > > + if (pSMBFile == NULL) > > > > > > + break; > > > > > > + if (!capable(CAP_SYS_ADMIN)) { > > > > > > + rc = -EACCES; > > > > > > + break; > > > > > > + } > > > > > > + > > > > > > + tcon = tlink_tcon(pSMBFile->tlink); > > > > > > + if (!smb3_encryption_required(tcon)) { > > > > > > + rc = -EOPNOTSUPP; > > > > > > + break; > > > > > > + } > > > > > > + pfull_key_inf.cipher_type = > > > > > > + le16_to_cpu(tcon->ses->server->cipher_type); > > > > > > + pfull_key_inf.Suid = tcon->ses->Suid; > > > > > > + memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, > > > > > > + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); > > > > > > + memcpy(pfull_key_inf.smb3decryptionkey, > > > > > > + tcon->ses->smb3decryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > > > + memcpy(pfull_key_inf.smb3encryptionkey, > > > > > > + tcon->ses->smb3encryptionkey, SMB3_ENC_DEC_KEY_SIZE); > > > > > > + if (copy_to_user((void __user *)arg, &pfull_key_inf, > > > > > > + sizeof(struct smb3_full_key_debug_info))) > > > > > > + rc = -EFAULT; > > > > > > + else > > > > > > + rc = 0; > > > > > > + break; > > > > > > case CIFS_IOC_NOTIFY: > > > > > > if (!S_ISDIR(inode->i_mode)) { > > > > > > /* Notify can only be done on directories */ > > > > > > > > > > > > -- > > > > > > Thanks, > > > > > > > > > > > > Steve > > > > > > > > > > > > > > > > -- > > > > Thanks, > > > > > > > > Steve > > > > > > > > > > > > -- > > > Regards, > > > Shyam > > > > -- > Thanks, > > Steve -- Thanks, Steve [-- Attachment #2: 0001-smb3.1.1-allow-dumping-keys-for-multiuser-mounts.patch --] [-- Type: text/x-patch, Size: 3577 bytes --] From a4853ef4df258ae9c8aa8b955dde44d8ef1625d5 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Sun, 2 May 2021 17:39:30 -0500 Subject: [PATCH] smb3.1.1: allow dumping keys for multiuser mounts When mounted multiuser it is hard to dump keys for the other sessions which makes it hard to debug using network traces (e.g. using wireshark). Suggested-by: Shyam Prasad N <sprasad@microsoft.com> Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/ioctl.c | 66 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c index 7d9654f56edc..37476ce13de9 100644 --- a/fs/cifs/ioctl.c +++ b/fs/cifs/ioctl.c @@ -214,11 +214,54 @@ static int cifs_shutdown(struct super_block *sb, unsigned long arg) return 0; } +static int cifs_dump_full_key(struct cifs_tcon *tcon, unsigned long arg) +{ + struct smb3_full_key_debug_info pfull_key_inf; + __u64 suid; + struct list_head *tmp; + struct cifs_ses *ses; + bool found = false; + + if (!smb3_encryption_required(tcon)) + return -EOPNOTSUPP; + + ses = tcon->ses; /* default to user id for current user */ + if (get_user(suid, (__u32 __user *)arg)) + suid = 0; + if (suid) { + /* search to see if there is a session with a matching SMB UID */ + spin_lock(&cifs_tcp_ses_lock); + list_for_each(tmp, &tcon->ses->server->smb_ses_list) { + ses = list_entry(tmp, struct cifs_ses, smb_ses_list); + if (ses->Suid == suid) { + found = true; + break; + } + } + spin_unlock(&cifs_tcp_ses_lock); + if (found == false) + return -EINVAL; + } /* else uses default user's SMB UID (ie current user) */ + + pfull_key_inf.cipher_type = le16_to_cpu(ses->server->cipher_type); + pfull_key_inf.Suid = ses->Suid; + memcpy(pfull_key_inf.auth_key, ses->auth_key.response, + 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); + memcpy(pfull_key_inf.smb3decryptionkey, ses->smb3decryptionkey, + 32 /* SMB3_ENC_DEC_KEY_SIZE */); + memcpy(pfull_key_inf.smb3encryptionkey, + ses->smb3encryptionkey, 32 /* SMB3_ENC_DEC_KEY_SIZE */); + if (copy_to_user((void __user *)arg, &pfull_key_inf, + sizeof(struct smb3_full_key_debug_info))) + return -EFAULT; + + return 0; +} + long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) { struct inode *inode = file_inode(filep); struct smb3_key_debug_info pkey_inf; - struct smb3_full_key_debug_info pfull_key_inf; int rc = -ENOTTY; /* strange error - but the precedent */ unsigned int xid; struct cifsFileInfo *pSMBFile = filep->private_data; @@ -366,26 +409,9 @@ long cifs_ioctl(struct file *filep, unsigned int command, unsigned long arg) rc = -EACCES; break; } - tcon = tlink_tcon(pSMBFile->tlink); - if (!smb3_encryption_required(tcon)) { - rc = -EOPNOTSUPP; - break; - } - pfull_key_inf.cipher_type = - le16_to_cpu(tcon->ses->server->cipher_type); - pfull_key_inf.Suid = tcon->ses->Suid; - memcpy(pfull_key_inf.auth_key, tcon->ses->auth_key.response, - 16 /* SMB2_NTLMV2_SESSKEY_SIZE */); - memcpy(pfull_key_inf.smb3decryptionkey, - tcon->ses->smb3decryptionkey, 32 /* SMB3_ENC_DEC_KEY_SIZE */); - memcpy(pfull_key_inf.smb3encryptionkey, - tcon->ses->smb3encryptionkey, 32 /* SMB3_ENC_DEC_KEY_SIZE */); - if (copy_to_user((void __user *)arg, &pfull_key_inf, - sizeof(struct smb3_full_key_debug_info))) - rc = -EFAULT; - else - rc = 0; + cifs_dump_full_key(tcon, arg); + break; case CIFS_IOC_NOTIFY: if (!S_ISDIR(inode->i_mode)) { -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-02 23:02 ` Steve French @ 2021-05-07 13:48 ` Aurélien Aptel 0 siblings, 0 replies; 16+ messages in thread From: Aurélien Aptel @ 2021-05-07 13:48 UTC (permalink / raw) To: Steve French, ronnie sahlberg Cc: Shyam Prasad N, CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER Steve French <smfrench@gmail.com> writes: >> > Or as an alternative, dump an array of ALL user sessions with >> > information about which user and which part of a multi-channel >> > connection that the keys belong to. >> > And let userspace sort out "which keys do I need for my wireshark session". All channels for 1 session share the same keys (except signing). I like Shyam's idea of having extra uid arg but it's more tricky to get (have to walk the tlink RB tree and find matching uid) compared to an extra session id arg. > +static int cifs_dump_full_key(struct cifs_tcon *tcon, unsigned long arg) > +{ > + struct smb3_full_key_debug_info pfull_key_inf; > + __u64 suid; > + struct list_head *tmp; > + struct cifs_ses *ses; > + bool found = false; > + > + if (!smb3_encryption_required(tcon)) > + return -EOPNOTSUPP; > + > + ses = tcon->ses; /* default to user id for current user */ > + if (get_user(suid, (__u32 __user *)arg)) > + suid = 0; > + if (suid) { > + /* search to see if there is a session with a matching SMB UID */ > + spin_lock(&cifs_tcp_ses_lock); > + list_for_each(tmp, &tcon->ses->server->smb_ses_list) { > + ses = list_entry(tmp, struct cifs_ses, smb_ses_list); > + if (ses->Suid == suid) { > + found = true; > + break; > + } > + } > + spin_unlock(&cifs_tcp_ses_lock); Btw, if we go with this we need to bump the session refcount if access it outside of the locked section (and decrement before returning). Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-04-30 22:19 [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares Steve French 2021-05-01 4:00 ` ronnie sahlberg @ 2021-05-07 0:17 ` Stefan Metzmacher 2021-05-07 3:18 ` Steve French 2021-05-07 10:28 ` Aurélien Aptel 1 sibling, 2 replies; 16+ messages in thread From: Stefan Metzmacher @ 2021-05-07 0:17 UTC (permalink / raw) To: Steve French, CIFS; +Cc: samba-technical, linux-cifsd-devel Hi Steve, > +/* > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > + * is needed if GCM256 (stronger encryption) negotiated > + */ > +struct smb3_full_key_debug_info { > + __u64 Suid; > + __u16 cipher_type; > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ Why this? With kerberos the authentication key can be 32 bytes too. Why are you exporting it at all? > + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; > +} __packed; > + As encryption and decryption is relative wouldn't something like smb3_s2c_cipherkey and smb3_c2s_cipherkey be better names? They are derived with SMBS2CCipherKey and SMBC2SCipherKey as labels. metze ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 0:17 ` Stefan Metzmacher @ 2021-05-07 3:18 ` Steve French 2021-05-07 10:28 ` Aurélien Aptel 1 sibling, 0 replies; 16+ messages in thread From: Steve French @ 2021-05-07 3:18 UTC (permalink / raw) To: Stefan Metzmacher Cc: CIFS, samba-technical, COMMON INTERNET FILE SYSTEM SERVER On Thu, May 6, 2021 at 7:17 PM Stefan Metzmacher <metze@samba.org> wrote: > > Hi Steve, > > > +/* > > + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) > > + * is needed if GCM256 (stronger encryption) negotiated > > + */ > > +struct smb3_full_key_debug_info { > > + __u64 Suid; > > + __u16 cipher_type; > > + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > Why this? With kerberos the authentication key can be 32 bytes too. > > Why are you exporting it at all? I don't remember the original reason for why it was thought wireshark could use this. Aurelien, Do you remember the context/reasons for each of the exported fields? -- Thanks, Steve ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 0:17 ` Stefan Metzmacher 2021-05-07 3:18 ` Steve French @ 2021-05-07 10:28 ` Aurélien Aptel 2021-05-07 12:43 ` Stefan Metzmacher 1 sibling, 1 reply; 16+ messages in thread From: Aurélien Aptel @ 2021-05-07 10:28 UTC (permalink / raw) To: Stefan Metzmacher, Steve French, CIFS; +Cc: samba-technical, linux-cifsd-devel Stefan Metzmacher <metze@samba.org> writes: > Hi Steve, > >> +/* >> + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) >> + * is needed if GCM256 (stronger encryption) negotiated >> + */ >> +struct smb3_full_key_debug_info { >> + __u64 Suid; >> + __u16 cipher_type; >> + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ > > Why this? With kerberos the authentication key can be 32 bytes too. That field should be named sesion_key. We only need 16 bytes for the session key. If the source has less we pad, if the source has more we truncate. That's how the specification describes it. > Why are you exporting it at all? Wireshark initially derived the keys by itself from the session key and computing the preauth from the trace: Pros: only need to type one thing in Wireshark and it can figure out the rest from the trace Cons: The trace needs to contain to negprog&session setup After several complaints, I added a way to directly provide S2C and C2S keys. You can either provide any combination of Sesssion Key, S2C or C2S and Wireshark will do best-effort to figure things out. >> + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; >> + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; >> +} __packed; >> + > > As encryption and decryption is relative wouldn't > > something like smb3_s2c_cipherkey and smb3_c2s_cipherkey be better names? > > They are derived with SMBS2CCipherKey and SMBC2SCipherKey as labels. I would call it server_in_key and server_out_key. I think we should have 2 ioctl: - GET_FULL_KEY_SIZE: return the size of the struct - GET_FULL_KEY: return the struct (note: no __packed, this might create all sorts of issues and kernel ABI will be stable across the system anyway. I hope we didn't pack other ioctl in the past... need to check) struct smb3_full_key_debug_info { __u64 session_id; __u16 cipher_type; union { struct smb3_aes128_debug_info { __u8 session_key[16]; __u8 server_in_key[16]; __u8 server_out_key[16]; } aes128; struct smb3_aes256_debug_info { __u8 session_key[16]; __u8 server_in_key[32]; __u8 server_out_key[32]; } aes256; } keys; }; * Users will call GET_FULL_KEY_SIZE to allocate a properly sized buffer. * Users can do a switch on cipher_type and process what they know exist * We can update the struct if we need to in the future without breaking userspace This should cover any possible updates (AES-512 or some new cipher...) unless I'm missing something. Thoughts? Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 10:28 ` Aurélien Aptel @ 2021-05-07 12:43 ` Stefan Metzmacher 2021-05-07 13:23 ` Aurélien Aptel 0 siblings, 1 reply; 16+ messages in thread From: Stefan Metzmacher @ 2021-05-07 12:43 UTC (permalink / raw) To: Aurélien Aptel, Steve French, CIFS Cc: samba-technical, linux-cifsd-devel [-- Attachment #1.1: Type: text/plain, Size: 3503 bytes --] Am 07.05.21 um 12:28 schrieb Aurélien Aptel via samba-technical: > Stefan Metzmacher <metze@samba.org> writes: > >> Hi Steve, >> >>> +/* >>> + * Dump full key (32 byte encrypt/decrypt keys instead of 16 bytes) >>> + * is needed if GCM256 (stronger encryption) negotiated >>> + */ >>> +struct smb3_full_key_debug_info { >>> + __u64 Suid; >>> + __u16 cipher_type; >>> + __u8 auth_key[16]; /* SMB2_NTLMV2_SESSKEY_SIZE */ >> >> Why this? With kerberos the authentication key can be 32 bytes too. > > That field should be named sesion_key. > > We only need 16 bytes for the session key. If the source has less we > pad, if the source has more we truncate. That's how the specification > describes it. No in order to derive the aes256 keys we need the full session key that falls out of the authentication and that's 32 bytes is kerberos with aes256-cts-hmac-sha1-96 (the default) is used. >> Why are you exporting it at all? > > Wireshark initially derived the keys by itself from the session key and > computing the preauth from the trace: > > Pros: only need to type one thing in Wireshark and it can figure out the > rest from the trace > > Cons: The trace needs to contain to negprog&session setup > > After several complaints, I added a way to directly provide S2C and C2S > keys. You can either provide any combination of Sesssion Key, S2C or C2S > and Wireshark will do best-effort to figure things out. Ok, the it's fine to export all 3. >>> + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE]; >>> + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE]; >>> +} __packed; >>> + >> >> As encryption and decryption is relative wouldn't >> >> something like smb3_s2c_cipherkey and smb3_c2s_cipherkey be better names? >> >> They are derived with SMBS2CCipherKey and SMBC2SCipherKey as labels. > > I would call it server_in_key and server_out_key. That's fine too. > I think we should have 2 ioctl: > - GET_FULL_KEY_SIZE: return the size of the struct > - GET_FULL_KEY: return the struct > > (note: no __packed, this might create all sorts of issues and kernel ABI > will be stable across the system anyway. I hope we didn't pack other > ioctl in the past... need to check) > struct smb3_full_key_debug_info { > __u64 session_id; > __u16 cipher_type; > union { > struct smb3_aes128_debug_info { > __u8 session_key[16]; > __u8 server_in_key[16]; > __u8 server_out_key[16]; > } aes128; > struct smb3_aes256_debug_info { > __u8 session_key[16]; > __u8 server_in_key[32]; > __u8 server_out_key[32]; > } aes256; > } keys; > }; > > * Users will call GET_FULL_KEY_SIZE to allocate a properly sized buffer. > * Users can do a switch on cipher_type and process what they know exist > * We can update the struct if we need to in the future without breaking > userspace > > This should cover any possible updates (AES-512 or some new cipher...) > unless I'm missing something. I think we should have explicit length fields for each key, if it's easier we can still use fixed size arrays. struct smb3_cipher_key_debug_info { __u64 session_id; __u16 cipher_algorithm; __u8 session_key_length; __u8 session_key[32]; __u8 server_in_key_length; __u8 server_in_key[32]; __u8 server_out_key_length; __u8 server_out_key[32]; }; SMB3_GET_CIPHER_KEY_DEBUG_INFO would be the ioctl constant. metze [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 12:43 ` Stefan Metzmacher @ 2021-05-07 13:23 ` Aurélien Aptel 2021-05-07 13:33 ` Aurélien Aptel 2021-05-07 14:16 ` Stefan Metzmacher 0 siblings, 2 replies; 16+ messages in thread From: Aurélien Aptel @ 2021-05-07 13:23 UTC (permalink / raw) To: Stefan Metzmacher, Steve French, CIFS; +Cc: samba-technical, linux-cifsd-devel Stefan Metzmacher <metze@samba.org> writes: > No in order to derive the aes256 keys we need the full session key that > falls out of the authentication and that's 32 bytes is kerberos > with aes256-cts-hmac-sha1-96 (the default) is used. Ok >> I would call it server_in_key and server_out_key. > > That's fine too. > >> I think we should have 2 ioctl: >> - GET_FULL_KEY_SIZE: return the size of the struct >> - GET_FULL_KEY: return the struct >> >> (note: no __packed, this might create all sorts of issues and kernel ABI >> will be stable across the system anyway. I hope we didn't pack other >> ioctl in the past... need to check) I've checked and sadly we use __packed for every struct :( That means we are forcing users to have packed struct as well which is not a standard C attribute. Portable C code will have to process each member field manually. I guess clang and gcc both support it so that's not a huge deal... anyway that's a problem for another day. >> struct smb3_full_key_debug_info { >> __u64 session_id; >> __u16 cipher_type; >> union { >> struct smb3_aes128_debug_info { >> __u8 session_key[16]; >> __u8 server_in_key[16]; >> __u8 server_out_key[16]; >> } aes128; >> struct smb3_aes256_debug_info { >> __u8 session_key[16]; >> __u8 server_in_key[32]; >> __u8 server_out_key[32]; >> } aes256; >> } keys; >> }; >> >> * Users will call GET_FULL_KEY_SIZE to allocate a properly sized buffer. >> * Users can do a switch on cipher_type and process what they know exist >> * We can update the struct if we need to in the future without breaking >> userspace >> >> This should cover any possible updates (AES-512 or some new cipher...) >> unless I'm missing something. > > I think we should have explicit length fields for each key, if it's easier > we can still use fixed size arrays. The problem with this is that if we make the keys bigger for a future cipher, old userspace program will send us a buffer of the size of the old smaller struct. eg: * user on kernel version N compiles: struct smb3_cipher_key_debug_info { __u64 session_id; __u16 cipher_algorithm; __u8 session_key_length; __u8 session_key[32]; __u8 server_in_key_length; __u8 server_in_key[32]; __u8 server_out_key_length; __u8 server_out_key[32]; } buf; // sizeof => 16 ioctl(fd, CIFS_DUMP_FULL_KEY, &buf); * user keeps compiled program and upgrades kernel to version N+1 where the kernel added AES-512 or whatever: * in the kernel, sizeof(smb3_cipher_key_debug_info) is now let's say 24 copy_to_user(userbuf, &kernelbuf, sizeof(struct smb3_full_key_debug_info)); Since we don't know the size of the user provided buffer and we assume it's the same as the current size of the struct, So we will actually write past it (24-16 => invalid write of 8 bytes). With the extra ioctl to get the size, userspace will always give us a buffer that matches the struct size of the running system. Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 13:23 ` Aurélien Aptel @ 2021-05-07 13:33 ` Aurélien Aptel 2021-05-07 14:16 ` Stefan Metzmacher 1 sibling, 0 replies; 16+ messages in thread From: Aurélien Aptel @ 2021-05-07 13:33 UTC (permalink / raw) To: Stefan Metzmacher, Steve French, CIFS; +Cc: samba-technical, linux-cifsd-devel Aurélien Aptel <aaptel@suse.com> writes: > } buf; // sizeof => 16 err that 16 is wrong sorry, but the rest of the argument stands, the kernel v.N+1 struct will be bigger and cause bad write. Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 13:23 ` Aurélien Aptel 2021-05-07 13:33 ` Aurélien Aptel @ 2021-05-07 14:16 ` Stefan Metzmacher 2021-05-07 15:37 ` Aurélien Aptel 1 sibling, 1 reply; 16+ messages in thread From: Stefan Metzmacher @ 2021-05-07 14:16 UTC (permalink / raw) To: Aurélien Aptel, Steve French, CIFS Cc: samba-technical, linux-cifsd-devel [-- Attachment #1.1: Type: text/plain, Size: 3516 bytes --] Am 07.05.21 um 15:23 schrieb Aurélien Aptel: > Stefan Metzmacher <metze@samba.org> writes: >> No in order to derive the aes256 keys we need the full session key that >> falls out of the authentication and that's 32 bytes is kerberos >> with aes256-cts-hmac-sha1-96 (the default) is used. > > Ok > >>> I would call it server_in_key and server_out_key. >> >> That's fine too. >> >>> I think we should have 2 ioctl: >>> - GET_FULL_KEY_SIZE: return the size of the struct >>> - GET_FULL_KEY: return the struct >>> >>> (note: no __packed, this might create all sorts of issues and kernel ABI >>> will be stable across the system anyway. I hope we didn't pack other >>> ioctl in the past... need to check) > > I've checked and sadly we use __packed for every struct :( > That means we are forcing users to have packed struct as well which is > not a standard C attribute. Portable C code will have to process each > member field manually. I guess clang and gcc both support it so that's > not a huge deal... anyway that's a problem for another day. > >>> struct smb3_full_key_debug_info { >>> __u64 session_id; >>> __u16 cipher_type; >>> union { >>> struct smb3_aes128_debug_info { >>> __u8 session_key[16]; >>> __u8 server_in_key[16]; >>> __u8 server_out_key[16]; >>> } aes128; >>> struct smb3_aes256_debug_info { >>> __u8 session_key[16]; >>> __u8 server_in_key[32]; >>> __u8 server_out_key[32]; >>> } aes256; >>> } keys; >>> }; >>> >>> * Users will call GET_FULL_KEY_SIZE to allocate a properly sized buffer. >>> * Users can do a switch on cipher_type and process what they know exist >>> * We can update the struct if we need to in the future without breaking >>> userspace >>> >>> This should cover any possible updates (AES-512 or some new cipher...) >>> unless I'm missing something. >> >> I think we should have explicit length fields for each key, if it's easier >> we can still use fixed size arrays. > > The problem with this is that if we make the keys bigger for a future > cipher, old userspace program will send us a buffer of the size of the > old smaller struct. > > eg: > > * user on kernel version N compiles: > > struct smb3_cipher_key_debug_info { > __u64 session_id; > __u16 cipher_algorithm; > __u8 session_key_length; > __u8 session_key[32]; > __u8 server_in_key_length; > __u8 server_in_key[32]; > __u8 server_out_key_length; > __u8 server_out_key[32]; > } buf; // sizeof => 16 > > ioctl(fd, CIFS_DUMP_FULL_KEY, &buf); > > * user keeps compiled program and upgrades kernel to version N+1 where the > kernel added AES-512 or whatever: > > * in the kernel, sizeof(smb3_cipher_key_debug_info) is now let's say 24 > > copy_to_user(userbuf, &kernelbuf, sizeof(struct smb3_full_key_debug_info)); > > Since we don't know the size of the user provided buffer and we assume > it's the same as the current size of the struct, So we will actually > write past it (24-16 => invalid write of 8 bytes). > > With the extra ioctl to get the size, userspace will always give us a > buffer that matches the struct size of the running system. If you ever change it just use another struct and another ioctl opcode. also the ioctl macros encode the struct size into the id, the the ioctl opcode would change anyway. metze [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares 2021-05-07 14:16 ` Stefan Metzmacher @ 2021-05-07 15:37 ` Aurélien Aptel 0 siblings, 0 replies; 16+ messages in thread From: Aurélien Aptel @ 2021-05-07 15:37 UTC (permalink / raw) To: Stefan Metzmacher, Steve French, CIFS; +Cc: samba-technical, linux-cifsd-devel Stefan Metzmacher <metze@samba.org> writes: > If you ever change it just use another struct and another ioctl opcode. > also the ioctl macros encode the struct size into the id, the the ioctl opcode would > change anyway. Ah I didn't realize the sizeof() was used to generate the code, good catch. If we have a different code per struct size then the code name should have the key size in it and there's no need to encode the key length in the struct itself. So something like: // rename current one #define CIFS_DUMP_KEY_128 _IOWR(CIFS_IOCTL_MAGIC, 8, struct smb3_key_debug_info_128) // add 256 one #define CIFS_DUMP_KEY_256 _IOWR(CIFS_IOCTL_MAGIC, 10, struct smb3_key_debug_info_256) Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-05-07 15:37 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-30 22:19 [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares Steve French 2021-05-01 4:00 ` ronnie sahlberg 2021-05-01 4:17 ` Steve French 2021-05-01 10:53 ` Shyam Prasad N 2021-05-01 20:49 ` ronnie sahlberg 2021-05-02 4:05 ` Steve French 2021-05-02 23:02 ` Steve French 2021-05-07 13:48 ` Aurélien Aptel 2021-05-07 0:17 ` Stefan Metzmacher 2021-05-07 3:18 ` Steve French 2021-05-07 10:28 ` Aurélien Aptel 2021-05-07 12:43 ` Stefan Metzmacher 2021-05-07 13:23 ` Aurélien Aptel 2021-05-07 13:33 ` Aurélien Aptel 2021-05-07 14:16 ` Stefan Metzmacher 2021-05-07 15:37 ` Aurélien Aptel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.