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

["Aurélien Aptel" <aaptel@suse.com>]

Stefan Metzmacher <metze@samba.org> writes:

> Hi Steve,
>
>> +/*
>> + * 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 */
>
> Why this? With kerberos the authentication key can be 32 bytes too.

That field should be named sesion_key.

We only need 16 bytes for the session key. If the source has less we
pad, if the source has more we truncate. That's how the specification
describes it.

> Why are you exporting it at all?

Wireshark initially derived the keys by itself from the session key and
computing the preauth from the trace:

Pros: only need to type one thing in Wireshark and it can figure out the
rest from the trace

Cons: The trace needs to contain to negprog&session setup

After several complaints, I added a way to directly provide S2C and C2S
keys. You can either provide any combination of Sesssion Key, S2C or C2S
and Wireshark will do best-effort to figure things out.

>> + __u8 smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE];
>> + __u8 smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE];
>> +} __packed;
>> +
>
> As encryption and decryption is relative wouldn't
>
> something like smb3_s2c_cipherkey and smb3_c2s_cipherkey be better names?
>
> They are derived with SMBS2CCipherKey and SMBC2SCipherKey as labels.

I would call it server_in_key and server_out_key.

I think we should have 2 ioctl:
- GET_FULL_KEY_SIZE: return the size of the struct
- GET_FULL_KEY: return the struct

(note: no __packed, this might create all sorts of issues and kernel ABI
will be stable across the system anyway. I hope we didn't pack other
ioctl in the past... need to check)

struct smb3_full_key_debug_info {
	__u64 session_id;
	__u16 cipher_type;
        union {
            struct smb3_aes128_debug_info {
        	__u8 session_key[16];
		__u8 server_in_key[16];
		__u8 server_out_key[16];
            } aes128;
            struct smb3_aes256_debug_info {
        	__u8 session_key[16];
		__u8 server_in_key[32];
		__u8 server_out_key[32];
            } aes256;
        } keys;
};

* Users will call GET_FULL_KEY_SIZE to allocate a properly sized buffer.
* Users can do a switch on cipher_type and process what they know exist
* We can update the struct if we need to in the future without breaking
  userspace

This should cover any possible updates (AES-512 or some new cipher...)
unless I'm missing something.

Thoughts?

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)