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(+)

  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 */

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

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

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

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

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

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

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

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?

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,

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

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 <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,

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,

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

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,