Parsing server SNI response fails

[Rantanen]

Looking at the code (and based on the behavior I'm witnessing), the parse_tls_extension_sni_content requires the 16-bit length to be present unconditionally for the SNI extension parsing to succeed:

tls-parser/src/tls_extensions.rs

Lines 239 to 246 in 867e9d0

	pub fn parse_tls_extension_sni_content(i: &[u8]) -> IResult<&[u8], TlsExtension> {
	let (i, list_len) = be_u16(i)?;
	let (i, v) = map_parser(
	take(list_len),
	many0(complete(parse_tls_extension_sni_hostname)),
	)(i)?;
	Ok((i, TlsExtension::SNI(v)))
	}

However the server is supposed to leave the extension data empty when it responds to the client:

A server that receives a client hello containing the "server_name" extension MAY use the information contained in the extension to guide its selection of an appropriate certificate to return to the client, and/or other aspects of security policy. In this event, the server SHALL include an extension of type "server_name" in the (extended) server hello. The "extension_data" field of this extension SHALL be empty.

Given the extension data may differ between client/server hello, I'd guess there would be some need for context specific parsing implementation.

Currently I'm working around this by grabbing the header length by hand and parsing each extension individually, which allows me to skip the failing SNI here.

[chifflier]

Indeed, the specification allows empty data for server. Thanks for the report!

I can think of two solutions (not mutually exclusive):

• Allow empty input in parse_tls_extension_sni_content, and return an empty Vec in the extension. Pros: minimal code modification, no API change / Cons: leaves check to the caller
• To support contextual parsing, adding pub functions like parse_tls_client_hello_extensions and parse_tls_client_hello_extensions could allow parsing only the expected extensions

[Rantanen]

I think having two functions would be clearer - especially if there are extensions that differ even more (say Client sends a list of strings, server responds with a numeric index). Two functions would allow exposing TlsServerExtensions and TlsClientExtensions.

I'd imagine this would also be somewhat simpler change for parsing even if it would require duplicating most of the current id-to-extension mapping. The problem otherwise would be that id 0x0000 maps to different contextual types of the same extension.

To avoid source code breakage with existing consumers, I guess type TlsExtensions = TlsClientExtensions could be used - although I'm not sure if type aliases can be deprecated without deprecating the underlying type.

But in the end if all of this sounds/ends up being too complicated, I do think allowing empty Vec in the current SNI extension struct wouldn't be too bad of a solution for the current SNI-specific issue. Empty Vec is quite cheap. I definitely wouldn't complain about such a solution as a consumer of tls-parser. I'd imagine most projects have the parser in just one corner of the codebase so it's not like a slightly sub-optimal API is something those projects would need to deal with everywhere.

[chifflier]

A quickfix was released in 0.9.4 (and commit 48db528) (returning an empty Vec).
I'm closing this issue, and have opened another (#13) for the ability to parse Client/Server extensions specifically.