struct Ntlm field session_key never set to non-None value

[malcolmstill]

struct Ntlm has a session_key field which is Option<[u8; SESSION_KEY_SIZE]>:

sspi-rs/src/ntlm/mod.rs

Lines 85 to 94 in a8272f7

	signing: bool, // integrity
	sealing: bool, // confidentiality
	send_signing_key: [u8; HASH_SIZE],
	recv_signing_key: [u8; HASH_SIZE],
	send_sealing_key: Option<Rc4>,
	recv_sealing_key: Option<Rc4>,
	session_key: Option<[u8; SESSION_KEY_SIZE]>,
	}

In all cases this field seems to be initialised with the value None. However, I don't see anywhere in the code where this is set to a non-None value. As such the getter for the session_key field always returns None:

sspi-rs/src/ntlm/mod.rs

Lines 214 to 216 in a8272f7

	pub fn session_key(&self) -> Option<[u8; SESSION_KEY_SIZE]> {
	self.session_key
	}

It looks like the place where this should be set is:

sspi-rs/src/ntlm/messages/server/complete_authenticate.rs

Lines 36 to 58 in a8272f7

	let session_key = authenticate_message
	.encrypted_random_session_key
	.map_or(Ok(key_exchange_key), |encrypted_random_session_key| {
	get_session_key(key_exchange_key, &encrypted_random_session_key, context.flags)
	})?;
	context.send_signing_key = generate_signing_key(session_key.as_ref(), SERVER_SIGN_MAGIC);
	context.recv_signing_key = generate_signing_key(session_key.as_ref(), CLIENT_SIGN_MAGIC);
	context.send_sealing_key = Some(Rc4::new(&generate_signing_key(session_key.as_ref(), SERVER_SEAL_MAGIC)));
	context.recv_sealing_key = Some(Rc4::new(&generate_signing_key(session_key.as_ref(), CLIENT_SEAL_MAGIC)));
	check_mic_correctness(
	negotiate_message.message.as_ref(),
	challenge_message.message.as_ref(),
	authenticate_message.message.as_ref(),
	&authenticate_message.mic,
	session_key.as_ref(),
	)?;
	context.state = NtlmState::Final;
	Ok(SecurityStatus::Ok)
	}

E.g. a context.session_key = Some(session_key); line before, say, line 55 context.state = NtlmState::Final;

[CBenoit]

Do you think you could take at look at this? @TheBestTvarynka

[TheBestTvarynka]

Yep, let me handle it 😉

[TheBestTvarynka]

@malcolmstill here is my conclusion:

The session_key field and its getter were introduced in #175.

In all cases this field seems to be initialised with the value None. However, I don't see anywhere in the code where this is set to a non-None value. As such the getter for the session_key field always returns None

It's not actually true. We store the session key when using Ntlm as a client, but do not store it when using Ntlm as a server. See this line:

sspi-rs/src/ntlm/messages/client/authenticate.rs

Line 166 in a6b0f8c

context.session_key = Some(session_key);

So, I'd consider it a small bug. We don't often use the Ntlm server part, but I think it'd be great to keep the code consistent and fix it.

[malcolmstill]

It's not actually true. We store the session key when using Ntlm as a client, but do not store it when using Ntlm as a server.

Ah yes, I see now I spoke too generally, thanks for the correction and clarification!

Do you have a preference for making the change yourselves vs me putting a PR in (I'm happy with whichever of those makes your lives easier)?

[TheBestTvarynka]

Do you have a preference for making the change yourselves vs me putting a PR in (I'm happy with whichever of those makes your lives easier)?

We encourage external contributions. I'd be happy to review your PR 😊

[CBenoit]

So, I'd consider it a small bug. We don't often use the Ntlm server part, but I think it'd be great to keep the code consistent and fix it.

Quick note from my side: I’m glad we are getting this kind of small bugs fixed now, because server-side IronRDP is using sspi-rs! Currently the main consumer is a QEMU project for an RDP Display server, but this is definitely aligned with our future priorities!