[s11n 3/7] Make session format ABI-independent #2785
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 13, 2019
jarlamsa
added
needs-work
component-tls
enhancement
mbed TLS team
needs-preceding-pr
jarlamsa
force-pushed
the
abi-indep-session-serial-dev
branch
2 times, most recently
from
37e7142 to
705e515
Compare
|
Amended 28b7590 |
jarlamsa
added
needs-ci
This was referenced Aug 14, 2019
jarlamsa
force-pushed
the
abi-indep-session-serial-dev
branch
from
705e515 to
f952581
Compare
|
Rebased on top of #2766 |
hanno-becker
previously approved these changes
Aug 15, 2019
jarlamsa
force-pushed
the
abi-indep-session-serial-dev
branch
from
f952581 to
16e79fc
Compare
|
Rebased |
|
@hanno-arm please re-review |
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.
Not checking the return value allowed a bug to go undetected, fix the bug and check the return value.
This test works regardless of the serialisation format and embedded pointers in it, contrary to the load-save test, though it requires more maintenance of the test code (sync the member list with the struct definition).
Found by 'all.sh test_no_platform' and by 'tests/scripts/test-ref-configs.pl'.
We have explicit recommendations to use US spelling for technical writing, so let's apply this to code as well for uniformity. (My fingers tend to prefer UK spelling, so this needs to be fixed in many places.) sed -i 's/\([Ss]eriali\)s/\1z/g' **/*.[ch] **/*.function **/*.data ChangeLog
This bug was present since cert digest had been introduced, which highlights the need for testing. While at it, fix a bug in the comment explaining the format - this was introduced by me copy-pasting to hastily from current baremetal, that has a different format (see next PR in the series for the same in development).
The chosen fix matches what's currently done in the baremetal branch - except the `#ifdef` have been adapted because now in baremetal the digest is not kept if renegotiation is disabled.
The size of the ticket used in this test dropped from 192 to 143 bytes, so move all sizes used in this test down 50 bytes. Also, we now need to adapt the server response size as the default size would otherwise collide with the new mtu value.
Previously the test didn't work because of embedded pointer values that are not predictable. Now it works as we no longer serialize such values.
jarlamsa
force-pushed
the
abi-indep-session-serial-dev
branch
from
9fcb57a to
aa75583
Compare
|
Rebased without conflicts |
|
@hanno-arm @jarvte please re-review |
jarvte
approved these changes
Aug 23, 2019
hanno-becker
approved these changes
Aug 23, 2019
|
All of the serialisation PRs now have 2 approving reviews and passing CI. Thus considering all of them ready for merge. |
jarlamsa
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a side-port to development of PR https://github.com/ARMmbed/mbedtls-restricted/pull/570 that has already been merged to baremetal.
Dependencies: based on #2764 and #2766 - see "Background" below.
The goal of this PR is to make the session serialisation ABI-independent.
Background
This is the third in a series of seven PRs implementing serialization of a full SSL context (not just the session), under some restrictions.
The PRs in the series are:
The full series is already merged in baremetal. It needs upstreaming for the benefit of the Could team, who will be the main user, and relies on upstream releases.
Side-porting details
Side-porting was done with
git cherry-pick; most commit were picked with no conflicts. With the remaining one, the conflicts were trivial (only the context had changed due to independent addition at the same place).The table shows how each side-ported commit was handled:
Review notes
Reviewers are strongly encouraged to review commit by commit, as the final diff is unlikely to make sense. Please note that the first commits (up to "Fix serialization tests for !SSL_KEEP_PEER_CERT") are part of #2764 and #2766 and don't need to be re-reviewed here (except checking that they're indeed part of #2764 #2766).