[PATCH] Reuse of nonces patch breaks SMB3.0 connections without encryption.

[Simo Sorce]

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.