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: 3516 bytes --]

Am 07.05.21 um 15:23 schrieb Aurélien Aptel:
> Stefan Metzmacher <metze@samba.org> writes:
>> 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.
> 
> Ok
> 
>>> 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)
> 
> I've checked and sadly we use __packed for every struct :(
> That means we are forcing users to have packed struct as well which is
> not a standard C attribute. Portable C code will have to process each
> member field manually. I guess clang and gcc both support it so that's
> not a huge deal... anyway that's a problem for another day.
> 
>>> 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.
> 
> The problem with this is that if we make the keys bigger for a future
> cipher, old userspace program will send us a buffer of the size of the
> old smaller struct.
> 
> eg:
> 
> * user on kernel version N compiles:
> 
>     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];
>     } buf; // sizeof => 16
>     
>     ioctl(fd, CIFS_DUMP_FULL_KEY, &buf);
> 
> * user keeps compiled program and upgrades kernel to version N+1 where the
>   kernel added AES-512 or whatever:
> 
> * in the kernel, sizeof(smb3_cipher_key_debug_info) is now let's say 24
> 
>     copy_to_user(userbuf, &kernelbuf, sizeof(struct smb3_full_key_debug_info));
> 
> Since we don't know the size of the user provided buffer and we assume
> it's the same as the current size of the struct, So we will actually
> write past it (24-16 => invalid write of 8 bytes).
> 
> With the extra ioctl to get the size, userspace will always give us a
> buffer that matches the struct size of the running system.

If you ever change it just use another struct and another ioctl opcode.
also the ioctl macros encode the struct size into the id, the the ioctl opcode would
change anyway.

metze



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