Ntlm fixes for disabled sealing and signing

[pauldumais]

1. NTLM flags now allow to disable signing and sealing
2. MIC is now properly generated if key exchange is disabled (such as the case of disabled sealing)
3. NTLM DecryptMessage now supports STREAM decoding

[CBenoit]

I don’t understand most of it, but I left a few comments FYI. Feel free to ignore the nits.

[CBenoit]

Nit: I understand that we re-use key_exchange_key unless the NTLM_SSP_NEGOTIATE_KEY_EXCH flag is set, in which case we generate a different session key. Instead of generating a session key that we never use and use a mutable binding, you could do that:

    let session_key = if context.flags.contains(NegotiateFlags::NTLM_SSP_NEGOTIATE_KEY_EXCH) {
        OsRng.gen::<[u8; SESSION_KEY_SIZE]>()
    } else {
        key_exchange_key
    };

(if blocks are expressions, and this is more idiomatic)

[pauldumais]

Ok

[CBenoit]

Question: do you know if the "invariant" buffer.len() > 16 is checked somewhere? Inside SecurityBuffer perhaps? Just being cautious, because in Rust, when the index is out of bounds, such indexing, instead of corrupting the memory, will stop the program by panicking (similar to C++ stack unwinding when an exception is raised). However, panicking (or unwinding) across an FFI boundary is undefined behavior. (There is a working group on that matter, but as of today it’s undefined.)

One avenue is to use the get method for a checked access which returns Option<&[u8]>:

let signature = encrypted.get(0..16).ok_or_else(|| /* construct error value */)?;
let enc = &encrypted[16..]; // the second one is fine, because of the above

Another avenue is to perform an upfront check like so:

// Upfront check
if encrypted.len() < 16 {
    return Err(/* error value */);
}

// Split the slice in two using `split_at`
let (signature, enc) = encrypted.split_at(16);

Mostly a matter of preference.

[pauldumais]

Perfect!

[CBenoit]

Nit: as far as I can see in the rest of the code, there is no need to use .to_vec():

Suggested change

	let signature = encrypted[0..16].to_vec();
	let enc = encrypted[16..].to_vec();
	let signature = &encrypted[0..16];
	let enc = &encrypted[16..];

(and remove the .as_slice() below)

[pauldumais]

Done

[CBenoit]

Nit: as far as I know, it’s more idiomatic to write this:

Suggested change

	let mut encrypted = if let Ok(buffer) = SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Token) {
	buffer
	} else {
	SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Stream)?
	}
	.buffer
	.clone();
	let security_buffer = let Ok(buffer) = SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Token) {
	buffer
	} else {
	SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Stream)?
	};
	let encrypted = &security_buffer.buffer;

Or even better:

let security_buffer = SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Token)
    .or_else(|| SecurityBuffer::find_buffer_mut(message, SecurityBufferType::Stream))?;
let encrypted = &security_buffer.buffer;

If I understand correctly, it’s also not necessary to .clone() the buffer at all.

[pauldumais]

This code was copied from the kerberos code, and the changes proposed do not compile, I will leave it as is since it's only a nit.

[RRRadicalEdward]

Benoît has written all the comments. I have nothing to add

[awakecoding]

the changes don't look like they'd break other ways to call the same APIs, so I'm good with them. I'll leave the rest for @CBenoit to review in terms of Rust code style :)