tls_wrap: parse tls session ticket extension

[indutny]

And, if present and non-empty, don't invoke resumeSession callback.

fix #5872

[indutny]

/cc @bnoordhuis @isaacs @bajtos

[bajtos]

You are reading 4 bytes below, I would expect the condition to be avail - ext_off < 4.

[indutny]

Thanks, again! All fixed. /cc @isaacs @bnoordhuis

[bajtos]

For what it's worth, the change LGTM. Can it be back-ported to v0.10 too?

[indutny]

@bajtos no way for backporting it, I promised to not touch clienthello parser in node_crypto.cc in any version.

[bajtos]

@bajtos no way for backporting it, I promised to not touch clienthello parser in node_crypto.cc in any version.

I saw this coming after Ben's comment in the other issue :)

If we can disable TLS session tickets in cluster workers, then these extra resumeSession calls at least won't cause slowdowns, since the session lookup is fast in single-process case.

[bnoordhuis]

Style?

uint16_t cipher_len =
    (data[cipher_offset] << 8) + data[cipher_offset + 1];

Is a little more idiomatic. For that matter, you could write it like this:

uint16_t cipher_len =
    data[cipher_offset] * 256 + data[cipher_offset + 1];

Then you don't have to use parentheses everywhere. Just food for thought. :-)

[indutny]

Well, its mostly style issues :) I like having powers of two in code, though I know value of 2^8, 2^16 and 2^24.

[bnoordhuis]

Also, data[cipher_offset + 1] looks like it could be an out-of-bounds read.

[bnoordhuis]

Some comments, mostly LGTM.

[bnoordhuis]

One more comment (I also mentioned this on IRC): the parser is calling ParseFinish() in a lot of places when the input is malformed. Shouldn't that be (the currently non-existent) ParseError()?

[indutny]

Thanks, landed with fixes in dda22a5.

[indutny]

About ParseError(): I'm calling ParseFinish() to let OpenSSL deal with any trouble or security problem that might be related to incorrect data in ClientHello. Therefore, there is no need in function that'll report errors and figure out what exactly wrong, as its already implemented in openssl.