[s11n 4/7] Add version + config check to SSL tickets #2788

jarlamsa · 2019-08-14T10:31:30Z

Description

This is a side-port to development of PR https://github.com/ARMmbed/mbedtls-restricted/pull/567 that has already been merged to baremetal.

Dependencies: based on #2764, #2766 and #2785 - see "Background" below.

This PR contains a proposal for how to add version + config identifiers to serialized SSL sessions (and, later, also to serialized SSL contexts) in order to detect the use serialized sessions in versions and/or configurations different from those in which they were issued.

Background

This is the fourth 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:

r563 -> [s11n 1/7] Refactor derive keys (side-port) #2764 Refactor derive keys
r568 -> [s11n 2/7] Make session serialisation functions public #2766 Make session serialisation functions public
r570 -> [s11n 3/7] Make session format ABI-independent #2785 Make session format ABI-independent
r567 -> [s11n 4/7] Add version + config check to SSL tickets #2788 Add version + config check to SSL tickets (this PR)
r572 -> [s11n 5/7] Context serialisation: API and documentation #2789 Context serialisation: API and documentation
r575 -> [s11n 6/7] Serialisation test #2791 Serialization test
r616 -> [s11n 7/7] Implement context serialisation #2792 Implement context serialization

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; all commits were picked with no conflicts.

The table shows how each side-ported commit was handled:

(nothing) cherry-picking worked without conflict
"t" only trivial context-related conflicts (eg. independent additions to the same place)
"r" manual rework after cherry-picking
"R" the commit was reconstructed manually (moving code around)
"+" added in this side-port PR

resolution	original commit	commit message
	`b5352f0`	Add Mbed TLS version to SSL sessions
	`557fe9f`	Add configuration identifier to serialized SSL sessions
	`4152762`	Encode relevant parts of the config in serialized session header
	`f99ec26`	Add negative tests for unexpected ver/cfg in session deserialization
	`cb9ba0f`	Use consistent spelling of 'serialise/serialize' in SSL test suite
	`1d8b6d7`	Session serialization: Fail with BAD_INPUT_DATA if buffer too small
	`26829e9`	Improve doc'n of config-identifying bitfield in serialized session
	`b36db4f`	Note that ver+fmt bytes in serialized data must not be removed
	`baf968c`	Use def'n consts for bits in config-identifier of serialized data
	`08ec129`	Use US spelling 'serialize' instead of UK spelling 'serialise'
	`f78af37`	Improve test for detection of ver/cfg corruption in serialized data
	`5dbcc9f`	Introduce specific error for ver/cfg mismatch on deserialization
	`7bf7710`	Remove reference to outdated compile-time option

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 "Re-enable test that now works with new format") are part of #2764, #2766 and #2785 and don't need to be re-reviewed here (except checking that they're indeed part of #2764 #2766 #2785).

hanno-becker

I did the rebase myself and it went through without conflicts, leading the same result as this PR.

jarlamsa · 2019-08-15T07:16:54Z

Rebased on top of #2785

hanno-becker

I did the rebase myself and obtained the same result.

jarlamsa · 2019-08-20T10:25:23Z

rebased

jarlamsa · 2019-08-20T10:32:01Z

@hanno-arm please re-review

hanno-becker

I did the rebase myself and obtained an equivalent result. Therefore re-approving.

This is currently a dummy, just introducing the new name.

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.

The format of serialized SSL sessions depends on the version and the configuration of Mbed TLS; attempts to restore sessions established in different versions and/or configurations lead to undefined behaviour. This commit adds an 3-byte version header to the serialized session generated and cleanly fails ticket parsing in case a session from a non-matching version of Mbed TLS is presented.

This commit adds space for two bytes in the header of serizlied SSL sessions which can be used to determine the structure of the remaining serialized session in the respective version of Mbed TLS. Specifically, if parts of the session depend on whether specific compile-time options are set or not, the setting of these options can be encoded in the added space. This commit doesn't yet make use of the fields.

This commit makes use of the added space in the session header to encode the state of those parts of the compile-time configuration which influence the structure of the serialized session in the present version of Mbed TLS. Specifically, these are - the options which influence the presence/omission of fields from mbedtls_ssl_session (which is currently shallow-copied into the serialized session) - the setting of MBEDTLS_X509_CRT_PARSE_C, which determines whether the serialized session contains a CRT-length + CRT-value pair after the shallow-copied mbedtls_ssl_session instance. - the setting of MBEDTLS_SSL_SESSION_TICKETS, which determines whether the serialized session contains a session ticket.

This commit improves the test exercising the behaviour of session deserialization when facing an unexpected version or config, by testing ver/cfg corruption at any bit in the ver/cfg header of the serialized data; previously, it had only tested the first bit of each byte.

This commit introduces a new SSL error code `MBEDTLS_ERR_SSL_VERSION_MISMATCH` which can be used to indicate operation failure due to a mismatch of version or configuration. It is put to use in the implementation of `mbedtls_ssl_session_load()` to signal the attempt to de-serialize a session which has been serialized in a build of Mbed TLS using a different version or configuration.

jarlamsa · 2019-08-23T09:51:53Z

Rebased without conflicts

jarlamsa · 2019-08-23T10:14:48Z

@hanno-arm @jarvte please re-review

hanno-becker

I did the rebase myself and obtained the same result. Therefore reapproving.

jarlamsa · 2019-08-26T06:30:17Z

All of the serialisation PRs now have 2 approving reviews and passing CI. Thus considering all of them ready for merge.

jarlamsa added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-tls needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Aug 14, 2019

jarlamsa requested review from simonbutcher and hanno-becker August 14, 2019 10:31

jarlamsa mentioned this pull request Aug 14, 2019

[s11n 3/7] Make session format ABI-independent #2785

Merged

This was referenced Aug 14, 2019

[s11n 2/7] Make session serialisation functions public #2766

Merged

[s11n 1/7] Refactor derive keys (side-port) #2764

Merged

jarlamsa mentioned this pull request Aug 14, 2019

[s11n 5/7] Context serialisation: API and documentation #2789

Merged

hanno-becker previously approved these changes Aug 14, 2019

View reviewed changes

jarlamsa mentioned this pull request Aug 15, 2019

[s11n 6/7] Serialisation test #2791

Merged

jarlamsa dismissed hanno-becker’s stale review via 74075ba August 15, 2019 07:16

jarlamsa force-pushed the session-serial-version-check-dev branch from 7ba1f4f to 74075ba Compare August 15, 2019 07:16

jarlamsa mentioned this pull request Aug 15, 2019

[s11n 7/7] Implement context serialisation #2792

Merged

hanno-becker previously approved these changes Aug 15, 2019

View reviewed changes

jarlamsa dismissed hanno-becker’s stale review via c85c336 August 20, 2019 10:25

jarlamsa force-pushed the session-serial-version-check-dev branch from 74075ba to c85c336 Compare August 20, 2019 10:25

jarlamsa requested a review from hanno-becker August 20, 2019 10:32

jarlamsa force-pushed the session-serial-version-check-dev branch from c85c336 to f40a5e0 Compare August 21, 2019 06:13

hanno-becker previously approved these changes Aug 21, 2019

View reviewed changes

jarvte self-requested a review August 23, 2019 07:17

jarvte previously approved these changes Aug 23, 2019

View reviewed changes

Start splitting populate_transform() out of derive_keys()

e59ae23

This is currently a dummy, just introducing the new name.

mpg and others added 16 commits August 23, 2019 12:50

Add new ABI-independent format for serialization

f743c03

Adapt buffering test to new ticket size

eef4c75

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.

Re-enable test that now works with new format

aa75583

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.

Add negative tests for unexpected ver/cfg in session deserialization

861d0bb

Use consistent spelling of 'serialise/serialize' in SSL test suite

bb54d5a

Session serialization: Fail with BAD_INPUT_DATA if buffer too small

f37d918

Improve doc'n of config-identifying bitfield in serialized session

50b5966

Note that ver+fmt bytes in serialized data must not be removed

dc28b6c

Use def'n consts for bits in config-identifier of serialized data

3e08866

Use US spelling 'serialize' instead of UK spelling 'serialise'

363b646

Remove reference to outdated compile-time option

be34e8e

jarlamsa dismissed stale reviews from jarvte and hanno-becker via be34e8e August 23, 2019 09:51

jarlamsa force-pushed the session-serial-version-check-dev branch from f40a5e0 to be34e8e Compare August 23, 2019 09:51

jarvte self-requested a review August 23, 2019 10:30

jarvte approved these changes Aug 23, 2019

View reviewed changes

hanno-becker approved these changes Aug 23, 2019

View reviewed changes

jarlamsa added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Aug 26, 2019

Patater merged commit be34e8e into Mbed-TLS:development Aug 27, 2019

Patater removed the needs-preceding-pr Requires another PR to be merged first label Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[s11n 4/7] Add version + config check to SSL tickets #2788

[s11n 4/7] Add version + config check to SSL tickets #2788

Uh oh!

jarlamsa commented Aug 14, 2019 •

edited

Loading

Uh oh!

hanno-becker left a comment

Uh oh!

jarlamsa commented Aug 15, 2019

Uh oh!

hanno-becker left a comment

Uh oh!

jarlamsa commented Aug 20, 2019

Uh oh!

jarlamsa commented Aug 20, 2019

Uh oh!

hanno-becker left a comment

Uh oh!

jarlamsa commented Aug 23, 2019

Uh oh!

jarlamsa commented Aug 23, 2019

Uh oh!

hanno-becker left a comment

Uh oh!

jarlamsa commented Aug 26, 2019

Uh oh!

Uh oh!

[s11n 4/7] Add version + config check to SSL tickets #2788

[s11n 4/7] Add version + config check to SSL tickets #2788

Uh oh!

Conversation

jarlamsa commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Background

Side-porting details

Review notes

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

jarlamsa commented Aug 15, 2019

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

jarlamsa commented Aug 20, 2019

Uh oh!

jarlamsa commented Aug 20, 2019

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

jarlamsa commented Aug 23, 2019

Uh oh!

jarlamsa commented Aug 23, 2019

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

jarlamsa commented Aug 26, 2019

Uh oh!

Uh oh!

jarlamsa commented Aug 14, 2019 •

edited

Loading