By looking just at that test, it looks like 2 + dn_size could overflow. In
fact that can't happen as that would mean we've read a CA cert of size is too
big to be represented by a size_t.
However, it's best for code to be more obviously free of overflow without
having to reason about the bigger picture.
There is only one length byte but for some reason we skipped two, resulting in
reading one byte past the end of the extension. Fortunately, even if that
extension is at the very end of the ClientHello, it can't be at the end of the
buffer since the ClientHello length is at most SSL_MAX_CONTENT_LEN and the
buffer has some more room after that for MAC and so on. So there is no
buffer overread.
Possible consequences are:
- nothing, if the next byte is 0x00, which is a comment first byte for other
extensions, which is why the bug remained unnoticed
- using a point format that was not offered by the peer if next byte is 0x01.
In that case the peer will reject our ServerKeyExchange message and the
handshake will fail.
- thinking that we don't have a common point format even if we do, which will
cause us to immediately abort the handshake.
None of these are a security issue.
The same bug was fixed client-side in fd35af15
Backport of f7022d1
Some people recommend using bit operations to avoid the compiler producing a
branch on `ret != 0`, but:
- this makes the code less readable,
- here I got a warning from some compilers about unsigned unary minus
- and anyway modern compilers don't produce a branch here, checked on x64 and
arm with various -O values.
Rationale: if people want to disable RC4 but otherwise keep the default suite
list, it was cumbersome. Also, since it uses a global array,
ssl_list_ciphersuite() is not a convenient place. So the SSL modules look like
the best place, even if it means temporarily adding one SSL setting.
Reading the documentation of ssl_set_truncated_hmac() may give the impression
I changed the default for clients but I didn't, the old documentation was
wrong.
