[PATCH] Reuse of nonces patch breaks SMB3.0 connections without encryption.
Simo Sorce
idra at samba.org
Tue Jun 16 17:30:09 MDT 2015
On Tue, 2015-06-16 at 15:58 -0700, Jeremy Allison wrote:
> The commit :
>
> -------------------------------------------------------
> commit 461c69bd7c52c8b980cf56be2abf9ce7accb6048
> Author: Simo Sorce <idra at samba.org>
> Date: Wed May 20 14:01:44 2015 +0200
>
> s3:smb2_server: In CCM and GCM mode we can't reuse nonces
>
> Reuse of nonces with AES-CCM and AES-GCM leads to catastrophic failure,
> so make sure the server drops the connection if that ever happens.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=11300
>
> Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
>
> Signed-off-by: Simo Sorce <simo at redhat.com>
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> -------------------------------------------------------
>
> that went into master breaks non-encrypted SMB3.0
> connections. The problem is the hunk added here:
>
> @@ -349,8 +353,31 @@ static NTSTATUS smbd_smb2_auth_generic_return(struct smbXsrv_session *session,
> d->context.data, d->context.length,
> x->global->encryption_key.data);
>
> - generate_random_buffer((uint8_t *)&x->nonce_high, sizeof(x->nonce_high));
> - x->nonce_low = 1;
> + /*
> + * CCM and GCM algorithms must never have their
> + * nonce wrap, or the security of the whole
> + * communication and the keys is destroyed.
> + * We must drop the connection once we have
> + * transfered too much data.
> + *
> + * NOTE: We assume nonces greater than 8 bytes.
> + */
> + generate_random_buffer((uint8_t *)&x->nonce_high_random,
> + sizeof(x->nonce_high_random));
> + switch (xconn->smb2.server.cipher) {
> + case SMB2_ENCRYPTION_AES128_CCM:
> + nonce_size = AES_CCM_128_NONCE_SIZE;
> + break;
> + case SMB2_ENCRYPTION_AES128_GCM:
> + nonce_size = AES_GCM_128_IV_SIZE;
> + break;
> + default:
> + ZERO_STRUCT(session_key);
> + return NT_STATUS_INVALID_PARAMETER;
> + }
> + x->nonce_high_max = SMB2_NONCE_HIGH_MAX(nonce_size);
> + x->nonce_high = 0;
> + x->nonce_low = 0;
>
> The previous code generated the nonce and did not
> look at xconn->smb2.server.cipher, meaning in the
> non-encrypted case where xconn->smb2.server.cipher == 0
> the nonce was ignored.
>
> Now if a client negotiates >= PROTOCOL_SMB2_24
> but doesn't negotiate encryption (i.e. xconn->smb2.server.cipher == 0)
> we run smack into the default case and return
> NT_STATUS_INVALID_PARAMETER and the client
> sessionsetup fails.
>
> The only place the nonces are actually used is
> in smb2_get_new_nonce(), which is only used in
> two places:
>
> 1). smbd_smb2_request_reply() when req->do_encryption == true.
> 2). smbd_smb2_send_break() when session->global->encryption_required == true
> or tcon->global->encryption_required == true.
>
> Now req->do_encryption is set to true if
> a header of size SMB2_TF_HDR_SIZE is sent, so
> this might get set at any time by a client
> request, even if the client didn't negotiate
> encryption.
>
> So I think the correct fix to protect all
> code paths is in the case where xconn->smb2.server.cipher
> is unknown (the default: case) is to set x->nonce_high_max = 0,
> and then add a check inside smb2_get_new_nonce() to
> return NT_STATUS_ENCRYPTION_FAILED if
> x->nonce_high_max == 0.
>
> Patch to do this attached. Please check.
>
> FYI. This bug was found by Codenomicon at
> the plugfest.
>
> Cheers,
>
> Jeremy.
The patch looks ok, but let me think about it some.
Simo.
More information about the samba-technical
mailing list