Re: [PATCH] smb3.1.1: allow dumping GCM256 keys to improve debugging of encrypted shares

[Steve French <smfrench@gmail.com>]

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