[Patches] SESSION_EXPIRED on encrypted sessions (bug #13624)
Jeremy Allison
jra at samba.org
Mon Oct 1 23:15:18 UTC 2018
On Mon, Oct 01, 2018 at 10:43:21AM +0200, Stefan Metzmacher via samba-technical wrote:
> Hi,
>
> here're patches which demonstrate bug #13624 ( STATUS_SESSION_EXPIRED
> error is returned unencrypted, if the request was encrypted)
> and fix it.
>
> Here's a merge request
> https://gitlab.com/samba-team/samba/merge_requests/81
> and the related pipeline:
> https://gitlab.com/samba-team/devel/samba/pipelines/31492348
>
> Please review and push:-)
RB+ good catch. Did you see this at the plugfest or at
the Redmond event ?
Took me a while to go through the codepaths to make
sure the:
@@ -2544,10 +2553,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
session_info->info->domain_name);
}
- if (req->was_encrypted || encryption_desired) {
- req->do_encryption = true;
- }
-
if (req->session) {
Hunk was OK, but I'm pretty sure it is :-).
Eventually we might want to decorate the
bools related to encryption and signing
in this function to make everything very
clear for reviewers, but I guess this isn't
that patch :-).
Cheers,
Jeremy.
> From 055b738fb4309d322ea8bb4c225c967b6f1b0a9d Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 28 Sep 2018 12:23:37 +0200
> Subject: [PATCH 1/2] s4:torture: split smb2.session.expire{1,2} to run with
> signing and encryptpion
>
> This reproduces the problem we have with expired encrypted sessions.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13624
>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> selftest/knownfail.d/session-expire | 2 ++
> source4/torture/smb2/session.c | 50 ++++++++++++++++++++++++++---
> 2 files changed, 48 insertions(+), 4 deletions(-)
> create mode 100644 selftest/knownfail.d/session-expire
>
> diff --git a/selftest/knownfail.d/session-expire b/selftest/knownfail.d/session-expire
> new file mode 100644
> index 000000000000..033564afb58d
> --- /dev/null
> +++ b/selftest/knownfail.d/session-expire
> @@ -0,0 +1,2 @@
> +^samba3.smb2.session krb5.expire1e
> +^samba3.smb2.session krb5.expire2e
> diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c
> index f3fa596e4648..7dc9ba19ee6e 100644
> --- a/source4/torture/smb2/session.c
> +++ b/source4/torture/smb2/session.c
> @@ -1046,7 +1046,8 @@ done:
> }
>
>
> -static bool test_session_expire1(struct torture_context *tctx)
> +static bool test_session_expire1i(struct torture_context *tctx,
> + bool force_encryption)
> {
> NTSTATUS status;
> bool ret = false;
> @@ -1075,6 +1076,7 @@ static bool test_session_expire1(struct torture_context *tctx)
> lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4");
>
> lpcfg_smbcli_options(tctx->lp_ctx, &options);
> + options.signing = SMB_SIGNING_REQUIRED;
>
> status = smb2_connect(tctx,
> host,
> @@ -1091,6 +1093,12 @@ static bool test_session_expire1(struct torture_context *tctx)
> torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> "smb2_connect failed");
>
> + if (force_encryption) {
> + status = smb2cli_session_encryption_on(tree->session->smbXcli);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2cli_session_encryption_on failed");
> + }
> +
> /* Add some random component to the file name. */
> snprintf(fname, sizeof(fname), "session_expire1_%s.dat",
> generate_random_str(tctx, 8));
> @@ -1168,7 +1176,20 @@ done:
> return ret;
> }
>
> -static bool test_session_expire2(struct torture_context *tctx)
> +static bool test_session_expire1s(struct torture_context *tctx)
> +{
> + return test_session_expire1i(tctx,
> + false); /* force_encryption */
> +}
> +
> +static bool test_session_expire1e(struct torture_context *tctx)
> +{
> + return test_session_expire1i(tctx,
> + true); /* force_encryption */
> +}
> +
> +static bool test_session_expire2i(struct torture_context *tctx,
> + bool force_encryption)
> {
> NTSTATUS status;
> bool ret = false;
> @@ -1218,6 +1239,7 @@ static bool test_session_expire2(struct torture_context *tctx)
> lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4");
>
> lpcfg_smbcli_options(tctx->lp_ctx, &options);
> + options.signing = SMB_SIGNING_REQUIRED;
>
> unc = talloc_asprintf(tctx, "\\\\%s\\%s", host, share);
> torture_assert(tctx, unc != NULL, "talloc_asprintf");
> @@ -1237,6 +1259,12 @@ static bool test_session_expire2(struct torture_context *tctx)
> torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> "smb2_connect failed");
>
> + if (force_encryption) {
> + status = smb2cli_session_encryption_on(tree->session->smbXcli);
> + torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> + "smb2cli_session_encryption_on failed");
> + }
> +
> caps = smb2cli_conn_server_capabilities(tree->session->transport->conn);
>
> /* Add some random component to the file name. */
> @@ -1528,6 +1556,18 @@ done:
> return ret;
> }
>
> +static bool test_session_expire2s(struct torture_context *tctx)
> +{
> + return test_session_expire2i(tctx,
> + false); /* force_encryption */
> +}
> +
> +static bool test_session_expire2e(struct torture_context *tctx)
> +{
> + return test_session_expire2i(tctx,
> + true); /* force_encryption */
> +}
> +
> bool test_session_bind1(struct torture_context *tctx, struct smb2_tree *tree1)
> {
> const char *host = torture_setting_string(tctx, "host", NULL);
> @@ -1681,8 +1721,10 @@ struct torture_suite *torture_smb2_session_init(TALLOC_CTX *ctx)
> torture_suite_add_1smb2_test(suite, "reauth4", test_session_reauth4);
> torture_suite_add_1smb2_test(suite, "reauth5", test_session_reauth5);
> torture_suite_add_1smb2_test(suite, "reauth6", test_session_reauth6);
> - torture_suite_add_simple_test(suite, "expire1", test_session_expire1);
> - torture_suite_add_simple_test(suite, "expire2", test_session_expire2);
> + torture_suite_add_simple_test(suite, "expire1s", test_session_expire1s);
> + torture_suite_add_simple_test(suite, "expire1e", test_session_expire1e);
> + torture_suite_add_simple_test(suite, "expire2s", test_session_expire2s);
> + torture_suite_add_simple_test(suite, "expire2e", test_session_expire2e);
> torture_suite_add_1smb2_test(suite, "bind1", test_session_bind1);
>
> suite->description = talloc_strdup(suite, "SMB2-SESSION tests");
> --
> 2.17.1
>
>
> From c185c1fd8ca336581dd1594b59341e4ee621dd93 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 17 Aug 2018 11:35:41 +0200
> Subject: [PATCH 2/2] smb2_server: set req->do_encryption = true earlier
>
> The STATUS_SESSION_EXPIRED error was returned unencrypted,
> if the request was encrypted.
>
> If clients use SMB3 encryption and the kerberos authenticated session
> expires, clients disconnect the connection instead of doing a reauthentication.
>
> From https://blogs.msdn.microsoft.com/openspecification/2012/10/05/encryption-in-smb-3-0-a-protocol-perspective/
>
> The sender encrypts the message if any of the following conditions is
> satisfied:
>
> - If the sender is sending a response to an encrypted request.
> - If Session.EncryptData is TRUE and the request or response being
> sent is not NEGOTIATE.
> - If Session.EncryptData is FALSE, the request or response being sent
> is not NEGOTIATE or SESSION_SETUP or TREE_CONNECT, and
> <TreeConnect|Share>.EncryptData is TRUE.
>
> [MS-SMB2] 3.3.4.1.4 Encrypting the Message
>
> If Connection.Dialect belongs to the SMB 3.x dialect family and
> Connection.ClientCapabilities includes the SMB2_GLOBAL_CAP_ENCRYPTION
> bit, the server MUST encrypt the message before sending, if any of the
> following conditions are satisfied:
>
> - If the message being sent is any response to a client request for which
> Request.IsEncrypted is TRUE.
>
> - If Session.EncryptData is TRUE and the response being sent is not
> SMB2_NEGOTIATE or SMB2 SESSION_SETUP.
>
> - If Session.EncryptData is FALSE, the response being sent is not
> SMB2_NEGOTIATE or SMB2 SESSION_SETUP or SMB2 TREE_CONNECT, and
> Share.EncryptData for the share associated with the TreeId in the SMB2
> header of the response is TRUE.
>
> The server MUST encrypt the message as specified in section 3.1.4.3,
> before sending it to the client.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13624
>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
> selftest/knownfail.d/session-expire | 2 --
> source3/smbd/smb2_server.c | 15 ++++++++++-----
> 2 files changed, 10 insertions(+), 7 deletions(-)
> delete mode 100644 selftest/knownfail.d/session-expire
>
> diff --git a/selftest/knownfail.d/session-expire b/selftest/knownfail.d/session-expire
> deleted file mode 100644
> index 033564afb58d..000000000000
> --- a/selftest/knownfail.d/session-expire
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -^samba3.smb2.session krb5.expire1e
> -^samba3.smb2.session krb5.expire2e
> diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
> index 229f8ab0e856..e36db1e55f5e 100644
> --- a/source3/smbd/smb2_server.c
> +++ b/source3/smbd/smb2_server.c
> @@ -2359,7 +2359,11 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
>
> req->async_internal = false;
> req->do_signing = false;
> - req->do_encryption = false;
> + if (opcode != SMB2_OP_SESSSETUP) {
> + req->do_encryption = encryption_desired;
> + } else {
> + req->do_encryption = false;
> + }
> req->was_encrypted = false;
> if (intf_v->iov_len == SMB2_TF_HDR_SIZE) {
> const uint8_t *intf = SMBD_SMB2_IN_TF_PTR(req);
> @@ -2383,9 +2387,11 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
> }
>
> req->was_encrypted = true;
> + req->do_encryption = true;
> }
>
> if (encryption_required && !req->was_encrypted) {
> + req->do_encryption = true;
> return smbd_smb2_request_error(req,
> NT_STATUS_ACCESS_DENIED);
> }
> @@ -2523,8 +2529,11 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
> encryption_required = true;
> }
> if (encryption_required && !req->was_encrypted) {
> + req->do_encryption = true;
> return smbd_smb2_request_error(req,
> NT_STATUS_ACCESS_DENIED);
> + } else if (encryption_desired) {
> + req->do_encryption = true;
> }
> } else if (call->need_session) {
> struct auth_session_info *session_info = NULL;
> @@ -2544,10 +2553,6 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
> session_info->info->domain_name);
> }
>
> - if (req->was_encrypted || encryption_desired) {
> - req->do_encryption = true;
> - }
> -
> if (req->session) {
> bool update_session_global = false;
> bool update_tcon_global = false;
> --
> 2.17.1
>
More information about the samba-technical
mailing list