Potentially unsafe uses of unsafe

[alex]

(I'm super excited to see QUIC for Firefox built in rust!)

I did a quick review of the uses of unsafe and a few potential issues jumped out at me.

neqo/neqo-crypto/src/aead.rs

Lines 79 to 86 in 5e5249a

	let rv = SSL_MakeAead(
	version,
	cipher,
	secret,
	p.as_ptr() as *const i8,
	p.len() as u32,
	&mut ctx,
	);

If p.len() larger than can fit in u32, you'll wrap around and produce an incorrect result. I don't think this can happen in reality, but using something like try_into() to propagate an error in that case would be safer.

neqo/neqo-crypto/src/agent.rs

Lines 680 to 684 in 5e5249a

	let url = CString::new(server_name);
	if url.is_err() {
	return Err(Error::InternalError);
	}
	result::result(unsafe { ssl::SSL_SetURL(agent.fd, url.unwrap().as_ptr()) })?;

I believe this produces a use-after-free issue. CString::new returns a Result<CString, SomeError>. Calling unwrap() gives you a CString, and then as_ptr() gets you the raw * const c_char. However, because as_ptr() returns a raw pointer, it doesn't participate in the lifetime, and thus nothing is keeping the CString alive. This issue is described here: https://doc.rust-lang.org/stable/std/ffi/struct.CString.html#method.as_ptr

[alex]

FWIW the second issue is caught by clippy, https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr

You may consider integrating clippy into your CI.

[martinthomson]

Thanks for catching those @alex. I'm surprised that we didn't get reports from clippy, because it is enabled in CI.

[alex]

Ooof, looks like clippy doesn't catch it in the exact structure that neqo has it (https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=409adc9305ceb2fcc63a9eca0f716933). I'll go file a bug on clippy for that.

[KevinHock]

Alex Gaynor saving the world, one PR or issue at a time :D

[Qwaz]

Is the second example an actual UAF? In my test, the temporary CString was dropped after the surrounding function returns.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=17a60c36d9ecb0ce31f6bcf4fd9a6071

[Phosphorus15]

Is the second example an actual UAF? In my test, the temporary CString was dropped after the surrounding function returns.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=17a60c36d9ecb0ce31f6bcf4fd9a6071

It depends on wether the original url string will be held by the ssl client (or copied instead), the former would exactly be an UAF

[Qwaz]

Yes, but that is another UAF from what was discussed in this thread. The discussed bug is UAF only if the temporary CString is dropped before calling SSL_SetURL, which I think it is not the case based on my experiment and the reference. The case you mentioned (pointer is copied inside FFI function) is dangerous even if drop happens after SSL_SetURL.