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

[Stefan Metzmacher <metze@samba.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3503 bytes --]

Am 07.05.21 um 12:28 schrieb Aurélien Aptel via samba-technical:
> 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.

No in order to derive the aes256 keys we need the full session key that
falls out of the authentication and that's 32 bytes is kerberos
with aes256-cts-hmac-sha1-96 (the default) is used.

>> 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.

Ok, the it's fine to export all 3.

>>> + __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.

That's fine too.

> 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.

I think we should have explicit length fields for each key, if it's easier
we can still use fixed size arrays.

struct smb3_cipher_key_debug_info {
	__u64 session_id;
 	__u16 cipher_algorithm;
	__u8 session_key_length;
  	__u8 session_key[32];
	__u8 server_in_key_length;
	__u8 server_in_key[32];
	__u8 server_out_key_length;
	__u8 server_out_key[32];
};

SMB3_GET_CIPHER_KEY_DEBUG_INFO would be the ioctl constant.

metze




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]