linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ronnie sahlberg <ronniesahlberg@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: CIFS <linux-cifs@vger.kernel.org>
Subject: Re: [PATCH][SMB3.1.1] add ability to send signing negotiate context
Date: Thu, 8 Jul 2021 14:51:40 +1000	[thread overview]
Message-ID: <CAN05THSBo74HOMYV+mHejb1s4tAb_uv=8cmcO1UDRScFw-WUQg@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5mstq3L33-RBDrJ+0Z1K2PAP0z=5bm=wQ8XHdx4th_caFw@mail.gmail.com>

lgtm

On Thu, Jul 8, 2021 at 2:49 PM Steve French <smfrench@gmail.com> wrote:
>
> v4 of patch (includes minor change to set local variable "num_algs"
> once to number of algorithms and use that throughout
> build_signing_context to make code a little clearer)
>
>
> On Wed, Jul 7, 2021 at 11:27 PM Steve French <smfrench@gmail.com> wrote:
> >
> > v3 of patch.  Updated with additional feedback from Ronnie (to make it
> > more context len and datalength clearer)
> >
> >
> > On Wed, Jul 7, 2021 at 9:44 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Support for faster packet signing (using GMAC instead of CMAC) can
> > > now be negotiated to some newer servers, including Windows.
> > > See MS-SMB2 section 2.2.3.17.
> > >
> > > This patch adds support for sending the new negotiate context
> > > with the first of three supported signing algorithms (AES-CMAC)
> > > and decoding the response.  A followon patch will add support
> > > for sending the other two (including AES-GMAC, which is fastest)
> > > and changing the signing algorithm used based on what was
> > > negotiated.
> > >
> > > To allow the client to request GMAC signing set module parameter
> > > "enable_negotiate_signing" to 1.
> > >
> > > Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/cifsfs.c   |  4 +++
> > >  fs/cifs/cifsglob.h |  3 ++
> > >  fs/cifs/smb2pdu.c  | 83 ++++++++++++++++++++++++++++++++++++++++------
> > >  fs/cifs/smb2pdu.h  |  5 ++-
> > >  4 files changed, 84 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > > index 9fb874dd8d24..476b07213fcd 100644
> > > --- a/fs/cifs/cifsfs.c
> > > +++ b/fs/cifs/cifsfs.c
> > > @@ -65,6 +65,7 @@ bool lookupCacheEnabled = true;
> > >  bool disable_legacy_dialects; /* false by default */
> > >  bool enable_gcm_256 = true;
> > >  bool require_gcm_256; /* false by default */
> > > +bool enable_negotiate_signing; /* false by default */
> > >  unsigned int global_secflags = CIFSSEC_DEF;
> > >  /* unsigned int ntlmv2_support = 0; */
> > >  unsigned int sign_CIFS_PDUs = 1;
> > > @@ -104,6 +105,9 @@ MODULE_PARM_DESC(enable_gcm_256, "Enable
> > > requesting strongest (256 bit) GCM encr
> > >  module_param(require_gcm_256, bool, 0644);
> > >  MODULE_PARM_DESC(require_gcm_256, "Require strongest (256 bit) GCM
> > > encryption. Default: n/N/0");
> > >
> > > +module_param(enable_negotiate_signing, bool, 0644);
> > > +MODULE_PARM_DESC(enable_GMAC_signing, "Enable requesting faster
> > > (GMAC) packet signing. Default: n/N/0");
> > > +
> > >  module_param(disable_legacy_dialects, bool, 0644);
> > >  MODULE_PARM_DESC(disable_legacy_dialects, "To improve security it may be "
> > >     "helpful to restrict the ability to "
> > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > > index 921680fb7931..3c2e117bb926 100644
> > > --- a/fs/cifs/cifsglob.h
> > > +++ b/fs/cifs/cifsglob.h
> > > @@ -667,9 +667,11 @@ struct TCP_Server_Info {
> > >   unsigned int max_write;
> > >   unsigned int min_offload;
> > >   __le16 compress_algorithm;
> > > + __u16 signing_algorithm;
> > >   __le16 cipher_type;
> > >   /* save initital negprot hash */
> > >   __u8 preauth_sha_hash[SMB2_PREAUTH_HASH_SIZE];
> > > + bool signing_negotiated; /* true if valid signing context rcvd from server */
> > >   bool posix_ext_supported;
> > >   struct delayed_work reconnect; /* reconnect workqueue job */
> > >   struct mutex reconnect_mutex; /* prevent simultaneous reconnects */
> > > @@ -1869,6 +1871,7 @@ extern unsigned int global_secflags; /* if on,
> > > session setup sent
> > >  extern unsigned int sign_CIFS_PDUs;  /* enable smb packet signing */
> > >  extern bool enable_gcm_256; /* allow optional negotiate of strongest
> > > signing (aes-gcm-256) */
> > >  extern bool require_gcm_256; /* require use of strongest signing
> > > (aes-gcm-256) */
> > > +extern bool enable_negotiate_signing; /* request use of faster (GMAC)
> > > signing if available */
> > >  extern bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> > >  extern unsigned int CIFSMaxBufSize;  /* max size not including hdr */
> > >  extern unsigned int cifs_min_rcv;    /* min size of big ntwrk buf pool */
> > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> > > index 962826dc3316..757f145e70e5 100644
> > > --- a/fs/cifs/smb2pdu.c
> > > +++ b/fs/cifs/smb2pdu.c
> > > @@ -433,6 +433,23 @@ build_compression_ctxt(struct
> > > smb2_compression_capabilities_context *pneg_ctxt)
> > >   pneg_ctxt->CompressionAlgorithms[2] = SMB3_COMPRESS_LZNT1;
> > >  }
> > >
> > > +static void
> > > +build_signing_ctxt(struct smb2_signing_capabilities *pneg_ctxt)
> > > +{
> > > + pneg_ctxt->ContextType = SMB2_SIGNING_CAPABILITIES;
> > > + /*
> > > + * Data length must be increased here if more than 3 signing algorithms
> > > + * sent but currently only 3 are defined. And context Data length must
> > > + * be rounded to multiple of 8 for some servers.
> > > + */
> > > + pneg_ctxt->DataLength =
> > > + cpu_to_le16(DIV_ROUND_UP(sizeof(struct smb2_signing_capabilities) -
> > > + sizeof(struct smb2_neg_context), 8) * 8);
> > > + pneg_ctxt->SigningAlgorithmCount = cpu_to_le16(1);
> > > + pneg_ctxt->SigningAlgorithms[0] = cpu_to_le16(SIGNING_ALG_AES_CMAC);
> > > + /* TBD add SIGNING_ALG_AES_GMAC and/or SIGNING_ALG_HMAC_SHA256 */
> > > +}
> > > +
> > >  static void
> > >  build_encrypt_ctxt(struct smb2_encryption_neg_context *pneg_ctxt)
> > >  {
> > > @@ -498,7 +515,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >         struct TCP_Server_Info *server, unsigned int *total_len)
> > >  {
> > >   char *pneg_ctxt;
> > > - unsigned int ctxt_len;
> > > + unsigned int ctxt_len, neg_context_count;
> > >
> > >   if (*total_len > 200) {
> > >   /* In case length corrupted don't want to overrun smb buffer */
> > > @@ -525,6 +542,17 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >   *total_len += ctxt_len;
> > >   pneg_ctxt += ctxt_len;
> > >
> > > + ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > + server->hostname);
> > > + *total_len += ctxt_len;
> > > + pneg_ctxt += ctxt_len;
> > > +
> > > + build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > + *total_len += sizeof(struct smb2_posix_neg_context);
> > > + pneg_ctxt += sizeof(struct smb2_posix_neg_context);
> > > +
> > > + neg_context_count = 4;
> > > +
> > >   if (server->compress_algorithm) {
> > >   build_compression_ctxt((struct smb2_compression_capabilities_context *)
> > >   pneg_ctxt);
> > > @@ -533,17 +561,24 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> > >   8) * 8;
> > >   *total_len += ctxt_len;
> > >   pneg_ctxt += ctxt_len;
> > > - req->NegotiateContextCount = cpu_to_le16(5);
> > > - } else
> > > - req->NegotiateContextCount = cpu_to_le16(4);
> > > + neg_context_count++;
> > > + }
> > >
> > > - ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> > > - server->hostname);
> > > - *total_len += ctxt_len;
> > > - pneg_ctxt += ctxt_len;
> > > + if (enable_negotiate_signing) {
> > > + pr_warn_once("requesting GMAC signing is experimental\n");
> > > + build_signing_ctxt((struct smb2_signing_capabilities *)
> > > + pneg_ctxt);
> > > + ctxt_len = DIV_ROUND_UP(
> > > + sizeof(struct smb2_signing_capabilities),
> > > + 8) * 8;
> > > + *total_len += ctxt_len;
> > > + pneg_ctxt += ctxt_len;
> > > + neg_context_count++;
> > > + }
> > > +
> > > + /* check for and add transport_capabilities and signing capabilities */
> > > + req->NegotiateContextCount = cpu_to_le16(neg_context_count);
> > >
> > > - build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> > > - *total_len += sizeof(struct smb2_posix_neg_context);
> > >  }
> > >
> > >  static void decode_preauth_context(struct smb2_preauth_neg_context *ctxt)
> > > @@ -632,6 +667,31 @@ static int decode_encrypt_ctx(struct
> > > TCP_Server_Info *server,
> > >   return 0;
> > >  }
> > >
> > > +static void decode_signing_ctx(struct TCP_Server_Info *server,
> > > +        struct smb2_signing_capabilities *pctxt)
> > > +{
> > > + unsigned int len = le16_to_cpu(pctxt->DataLength);
> > > +
> > > + if ((len < 4) || (len > 16)) {
> > > + pr_warn_once("server sent bad signing negcontext\n");
> > > + return;
> > > + }
> > > + if (le16_to_cpu(pctxt->SigningAlgorithmCount) != 1) {
> > > + pr_warn_once("Invalid signing algorithm count\n");
> > > + return;
> > > + }
> > > + if (le16_to_cpu(pctxt->SigningAlgorithms[0]) > 2) {
> > > + pr_warn_once("unknown signing algorithm\n");
> > > + return;
> > > + }
> > > +
> > > + server->signing_negotiated = true;
> > > + server->signing_algorithm = le16_to_cpu(pctxt->SigningAlgorithms[0]);
> > > + cifs_dbg(FYI, "signing algorithm %d chosen\n",
> > > +      server->signing_algorithm);
> > > +}
> > > +
> > > +
> > >  static int smb311_decode_neg_context(struct smb2_negotiate_rsp *rsp,
> > >        struct TCP_Server_Info *server,
> > >        unsigned int len_of_smb)
> > > @@ -675,6 +735,9 @@ static int smb311_decode_neg_context(struct
> > > smb2_negotiate_rsp *rsp,
> > >   (struct smb2_compression_capabilities_context *)pctx);
> > >   else if (pctx->ContextType == SMB2_POSIX_EXTENSIONS_AVAILABLE)
> > >   server->posix_ext_supported = true;
> > > + else if (pctx->ContextType == SMB2_SIGNING_CAPABILITIES)
> > > + decode_signing_ctx(server,
> > > + (struct smb2_signing_capabilities *)pctx);
> > >   else
> > >   cifs_server_dbg(VFS, "unknown negcontext of type %d ignored\n",
> > >   le16_to_cpu(pctx->ContextType));
> > > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> > > index ba75e65924ac..4b27cb9105fd 100644
> > > --- a/fs/cifs/smb2pdu.h
> > > +++ b/fs/cifs/smb2pdu.h
> > > @@ -329,7 +329,7 @@ struct smb2_neg_context {
> > >   __le16 ContextType;
> > >   __le16 DataLength;
> > >   __le32 Reserved;
> > > - /* Followed by array of data */
> > > + /* Followed by array of data. NOTE: some servers require padding to
> > > 8 byte boundary */
> > >  } __packed;
> > >
> > >  #define SMB311_LINUX_CLIENT_SALT_SIZE 32
> > > @@ -394,6 +394,7 @@ struct smb2_compression_capabilities_context {
> > >   __u16 Padding;
> > >   __u32 Flags;
> > >   __le16 CompressionAlgorithms[3];
> > > + /* Check if pad needed */
> > >  } __packed;
> > >
> > >  /*
> > > @@ -420,6 +421,7 @@ struct smb2_transport_capabilities_context {
> > >   __le16  DataLength;
> > >   __u32 Reserved;
> > >   __le32 Flags;
> > > + __u32 Pad;
> > >  } __packed;
> > >
> > >  /*
> > > @@ -458,6 +460,7 @@ struct smb2_signing_capabilities {
> > >   __u32 Reserved;
> > >   __le16 SigningAlgorithmCount;
> > >   __le16 SigningAlgorithms[];
> > > + /*  Followed by padding to 8 byte boundary (required by some servers) */
> > >  } __packed;
> > >
> > >  #define POSIX_CTXT_DATA_LEN 16
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

  reply	other threads:[~2021-07-08  4:51 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-08  2:44 [PATCH][SMB3.1.1] add ability to send signing negotiate context Steve French
2021-07-08  4:27 ` Steve French
2021-07-08  4:49   ` Steve French
2021-07-08  4:51     ` ronnie sahlberg [this message]
2021-07-08 22:05       ` Pavel Shilovsky
2021-07-08 23:19         ` Steve French
2021-07-08 23:29           ` Steve French
2021-07-08 23:34             ` Steve French
2021-07-09 17:36               ` Pavel Shilovsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAN05THSBo74HOMYV+mHejb1s4tAb_uv=8cmcO1UDRScFw-WUQg@mail.gmail.com' \
    --to=ronniesahlberg@gmail.com \
    --cc=linux-cifs@vger.kernel.org \
    --cc=smfrench@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).