This uncovered a bug that led to a double-free (in practice, in general could
be free() on any invalid value): initially the session structure is loaded
with `memcpy()` which copies the previous values of pointers peer_cert and
ticket to heap-allocated buffers (or any other value if the input is
attacker-controlled). Now if we exit before we got a chance to replace those
invalid values with valid ones (for example because the input buffer is too
small, or because the second malloc() failed), then the next call to
session_free() is going to call free() on invalid pointers.
This bug is fixed in this commit by always setting the pointers to NULL right
after they've been read from the serialised state, so that the invalid values
can never be used.
(An alternative would be to NULL-ify them when writing, which was rejected
mostly because we need to do it when reading anyway (as the consequences of
free(invalid) are too severe to take any risk), so doing it when writing as
well is redundant and a waste of code size.)
Also, while thinking about what happens in case of errors, it became apparent
to me that it was bad practice to leave the session structure in an
half-initialised state and rely on the caller to call session_free(), so this
commit also ensures we always clear the structure when loading failed.
This allows callers to discover what an appropriate size is. Otherwise they'd
have to either try repeatedly, or allocate an overly large buffer (or some
combination of those).
Adapt documentation an example usage in ssl_client2.
Avoid useless copy with mbedtls_ssl_get_session() before serialising.
Used in ssl_client2 for testing and demonstrating usage, but unfortunately
that means mbedtls_ssl_get_session() is no longer tested, which will be fixed
in the next commit.
On client side, this is required for the main use case where of serialising a
session for later resumption, in case tickets are used.
On server side, this doesn't change much as ticket_len will always be 0.
This unblocks testing the functions by using them in ssl_client2, which will
be done in the next commit.
This finishes making these functions public. Next step is to get them tested,
but there's currently a blocker for that, see next commit (and the commit
after it for tests).
While 'session hash' is currently unique, so suitable to prove that the
intended code path has been taken, it's a generic enough phrase that in the
future we might add other debug messages containing it in completely unrelated
code paths. In order to future-proof the accuracy of the test, let's use a
more specific string.
The previous comment used "TLS" as a shortcut for "TLS 1.0/1.1" which was
confusing. This partially reflected the names of the calc_verify/finished that
go ssl, tls (for 1.0/1.1) tls_shaxxx (for 1.2), but still it's clearer to be
explicit in the comment - and perhaps in the long term the function names
could be clarified instead.
When using this function to deserialize, it's not a problem to have a session
structure as input as we'll have one around anyway (most probably freshly
deserialised).
However for tests it's convenient to be able to build a transform without
having a session structure around.
Also, removing this structure from parameters makes the function signature
more uniform, the only exception left being the ssl param at the end that's
hard to avoid for now.
Configs with no DEBUG_C are used for example in test-ref-configs.pl, which also
runs parts of compat.sh or ssl-opt.sh on them, so the added 'ssl = NULL'
statements will be exercised in those tests at least.
Make it more explicit what's used. Unfortunately, we still need ssl as a
parameter for debugging, and because calc_verify wants it as a parameter (for
all TLS versions except SSL3 it would actually only need handshake, but SSL3
also accesses session_negotiate).
It's also because of calc_verify that we can't make it const yet, but see next
commit.
* origin/pr/2790: (40 commits)
Fix possibly-lossy conversion warning from MSVC
Reintroduce length 0 check for records
Don't use memcpy() for 2-byte copy operation
Remove integer parsing macro
Fix alignment in record header parsing routine
Don't disallow 'record from another epoch' log msg in proxy ref test
Make sure 'record from another epoch' is displayed for next epoch
Implement record checking API
Mark ssl_parse_record_header() as `const` in SSL context
Make mbedtls_ssl_in_hdr_len() CID-unaware
Remove duplicate setting of ssl->in_msgtype and ssl->in_msglen
Move update of in_xxx fields in ssl_get_next_record()
Move update of in_xxx fields outside of ssl_prepare_record_content()
Reduce dependency of ssl_prepare_record_content() on in_xxx fields
Move ssl_update_in_pointers() to after record hdr parsing
Mark DTLS replay check as `const` on the SSL context
Move updating the internal rec ptrs to outside of rec hdr parsing
Mark ssl_decrypt_buf() as `const in the input SSL context
Adapt ssl_prepare_record_content() to use SSL record structure
Use record length from record structure when fetching content in TLS
...
* origin/pr/2781:
Documentation fixes according to review
Remove unused label in ssl_client2/ssl_server2
Add missing word in documentation of mbedtls_ssl_check_record()
cli/srv ex: Add dbg msg if record checking gives inconsistent result
Fix minor issues in documentation of mbedtls_ssl_check_record()
State that record checking is DTLS only and doesn't check content type
Update version_features.c
Pass dgrams to mbedtls_ssl_check_record in ssl_client2/server2
Add IO wrappers to ssl_server2 as interm's between NET and SSL layer
Add IO wrappers to ssl_client2 as interm's between NET and SSL layer
Introduce configuration option and API for SSL record checking
This commit implements the record checking API
mbedtls_ssl_check_record()
on top of the restructured incoming record stack.
Specifically, it makes use of the fact that the core processing routines
ssl_parse_record_header()
mbedtls_ssl_decrypt_buf()
now operate on instances of the SSL record structure mbedtls_record
instead of the previous mbedtls_ssl_context::in_xxx fields.
ssl_get_next_record() updates the legacy in_xxx fields in two places,
once before record decryption and once after. Now that record decryption
doesn't use or affect the in_xxx fields anymore, setting up the these
legacy fields can entirely be moved to the end of ssl_get_next_record(),
which is what this comit does.
This commit solely moves existing code, but doesn't yet simplify the
now partially redundant settings of the in_xxx fields. This will be
done in a separate commit.
Multiple record attributes such as content type and payload length
may change during record decryption, and the legacy in_xxx fields
in the SSL context therefore need to be updated after the record
decryption routine ssl_decrypt_buf() has been called.
After the previous commit has made ssl_prepare_record_content()
independent of the in_xxx fields, setting them can be moved
outside of ssl_prepare_record_content(), which is what this
commit does.
Previously, ssl_update_in_pointers() ensured that the in_xxx pointers
in the SSL context are set to their default state so that the record
header parsing function ssl_parse_record_header() could make use of them.
By now, the latter is independent of these pointers, so they don't need
to be setup before calling ssl_parse_record_header() anymore.
However, other parts of the messaging stack might still depend on it
(to be studied), and hence this commit does not yet reomve
ssl_update_in_pointers() entirely.
