Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: correct calling to time function in tls13 client&server #7639

Merged
merged 1 commit into from
Jun 5, 2023
Merged

Fix: correct calling to time function in tls13 client&server #7639

merged 1 commit into from
Jun 5, 2023

Conversation

Taowyoo
Copy link
Contributor

@Taowyoo Taowyoo commented May 22, 2023

Description

In library/ssl_tls13_client.c and library/ssl_tls13_server.c, calling to time function need to be changed to mbedtls_time to handle the case when MBEDTLS_PLATFORM_TIME_MACRO is defined.

This PR will close #7655

PR checklist

  • changelog not required
  • backport no (no such code in 2.28)
  • tests not required

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Call `mbedtls_time` to handle the case when MBEDTLS_PLATFORM_TIME_MACRO is defined

Signed-off-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@minosgalanakis minosgalanakis added component-tls size-s Estimated task size: small (~2d) needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels May 30, 2023
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@davidhorstmann-arm davidhorstmann-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 5, 2023
@gilles-peskine-arm
Copy link
Contributor

The 2.28 LTS branch has some TLS 1.3 code, but does not have a call to time(), so no backport is necessary.

@gilles-peskine-arm gilles-peskine-arm merged commit 763c19a into Mbed-TLS:development Jun 5, 2023
@Taowyoo Taowyoo deleted the yx/fix-time-tls13-client-server branch June 5, 2023 16:49
bors bot added a commit to fortanix/rust-mbedtls that referenced this pull request Jun 26, 2023
278: Upgrade `mbedtls` to 3.4.0 r=Taowyoo a=Taowyoo

## Overview

[PR #213](#213) introduces some breaking changes:
- Update vendor `mbedtls` code from version `2.28.3` to a `3.4.0`
	- Please checkout C `mbedtls` migration guide for 3.X here [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md) to
- Update the build code for `mbedtls-sys-auto` crate to sync up with vendor code change:
	- Changes in C DEFINE's for configuration
	- Changes in header files
	- Add binding code generation for `static inline` code in C side
	- Removing deprecated cargo features or dependencies
	- Add a cargo feature for TLS 1.3
- Update rust wrapper code in `./mbedtls` to sync up the changes in  `mbedtls-sys-auto`
	- Removing deprecated cargo features or dependencies
	- Update rust code to sync up API changes
	- Add types and functions for TLS 1.3
	- Add a cargo feature for TLS 1.3
	- Update integration tests for new API and TLS 1.3
	- Update dependencies

## Breaking Changes:

**Note**: entries with 💥 are ensured that they will break the downstream.


### Changes in `mbedtls-sys-auto`

#### Vendor code changes

**Upstream changes:**

- 💥 Upgrade vendor `mbeldtls` code to `3.4.0`

**Changes on our side:**

- Cherry picked previous changes in old versions:
	- commit: [vendor change: Adding mpi_force_c_code feature](c8cd406)
- New changes
    - [vendor change: fix time call in tls13 client&server](bafc52d) : This has been merged into upstream, see: Mbed-TLS/mbedtls#7639 .

#### rust code changes

**Features:**

- 💥 `zlib` is removed: support for TLS record-level compression is removed in `mbedtls` 3.X
  - Related C DEFINE `MBEDTLS_ZLIB_SUPPORT` is also removed
- 💥 `legacy_protocols` is removed: all protocols early than TLS 1.2 is removed in `mbedtls` 3.X
  - Related C DEFINE's are also removed: `MBEDTLS_SSL_PROTO_SSL3`, `MBEDTLS_SSL_PROTO_TLS1`, `MBEDTLS_SSL_PROTO_TLS1_1`, `MBEDTLS_SSL_CBC_RECORD_SPLITTING`
- 💥 `pkcs11` is removed: wrapper for `libpkcs11-helper` is removed in `mbedtls` 3.X, see [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md#remove-wrapper-for-libpkcs11-helper)
- 💥 ~~`pkcs12` is removed: because #269 `pkcs12` still defined since it is needed for reading some pkcs8 format keys
- Put TLS 1.3 behind a feature `tls13` : because the dependency of TLS 1.3 in mbedtls 3.X are using a global state RNG which breaks the requirements for FIPS, so this feature enables use to avoid these code from compilation.
- 💥 Deprecated features  `custom_threading` , `custom_time` , `custom_gmtime_r` , `pthread`  are removed

**Dependencies:**

- 💥 Bump `mbedtls-sys` version to `3.4.0`
- 💥 Deprecated dependencies are removed
  - `libz-sys` : support for TLS record-level compression is removed in `mbedtls` 3.X
  - `libc`: `libc` is not needed in `sgx`

**Build code changes:**

Following changes are made according to [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md).

- Remove `MBEDTLS_CONFIG_H` in `mbedtls_config.h`
- Remove `#include <mbedtls/check_config.h>`
- `mbedtls-sys/build/bindgen.rs`
	-  Allow `bindgen` to generate bindings for functions, types and variables start with `psa_`, and put them in a sub `mod psa` because they are needed by TLS 1.3
	- Use  `bindgen` experiment feature to generate C function wrapper for C `static inline` functions
- `mbedtls-sys/build/headers.rs` : Update header files
- `mbedtls-sys/build/config.rs` : Remove/add C defines
	- 💥 Added `MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG` : this is added because TLS 1.3 need to use PSA library which need this when feature `std` is off, user need to provide their implementation. In `rus-mbedtls`, one implementation is proveded, see: `mbedtls/src/rng/mod.rs`.

### Changes in `mbedtls-platform-support`

- Added `once_cell` for initializing PSA only one times: see `fn psa_crypto_init()` in `mbedtls-platform-support/src/lib.rs`, this is needed because:
	- > MBEDTLS_USE_PSA_CRYPTO means that X.509 and TLS will use PSA Crypto as much as possible (that is, everywhere except for features that are not supported by PSA Crypto, see "Internal Changes" below for a complete list of exceptions). When it is enabled, you need to call psa_crypto_init() before calling any function from PK, X.509 or TLS;
	- Ref: https://github.com/Mbed-TLS/mbedtls/blob/0b3de6fcec4aa4b23a9ee1e076714cbc796f3ac4/docs/use-psa-crypto.md#general-considerations
- Add function pointer `mbedtls_psa_external_get_random`  which is needed when C DEFINE option `MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG` is turned on. This will be used in case when system default RNG or entropy is no available (for example in SGX)
    - Using function pointer here ensure there is no link time conflict in future when mutiple `rust-mbedtls` is using this crate.
    - User need to call function `set_psa_external_rng_callback` before using any PSA functions or TLS 1.3
- Add rust implementation of `explicit_bzero`, which is needed in SGX. Because in SGX, our [rs-libc](https://github.com/fortanix/rust-sgx/tree/master/rs-libc) does not support this function.
- Update self tests:
  - following are removed:
    - arc4_self_test
    - md2_self_test
    - md4_self_test
    - x509_self_test
    - xtea_self_test
  - following are added:
    - sha384_self_test
    - chacha20_self_test
    - chachapoly_self_test
    - poly1305_self_test
    - sha224_self_test

### Changes in `mbedtls`

**Features:**

- 💥 `zlib`, `legacy_protocols` removed to sync with changes in `mbedtls-sys`
- 💥 `pkcs12` and `pkcs12_rc2` are removed: see #269
- Put TLS 1.3 logic behind a feature `tls13`: check reason above

**Dependencies:**

- 💥 Bump `rust-mbedtls` version to `0.11.0`
- 💥 Bump dependency `mbedtls-sys-auto` version to `3.4.0`
- Added `rstest` `rstest_reuse` `lazy_static` `env_logger`: used for improving code of tests


### Code changes

- Function `EcPoint::mul` now need to pass in a RNG to ensure blinding.
- Add prefix `private_` to some fields of `mbedtls` types under `mbedtls/src/cipher/raw`
- 💥 Removed and added some options in `Error`, `CipherType`, `CipherMode` and `CipherSuite`  to sync with changes in `mbedtls` 3.X
- 💥 `mbedtls/src/pk/ec.rs` : User need to provide a RNG callback for function `EcPoint::mul`, this originally is not a hard requirement, but in C `mbedtls` 3.X this become a hard requirement for blinding to defend side channel attack.
- 💥 `mbedtls/hash` :
	- `Md2` and `Md4` are removed since they are no longer supported in `mbedtls` 3.X
	- fn `pbkdf_pkcs12` is removed since `pkcs12` is removed
- 💥 `mbedtls/pk/mod.rs` :
	- Remove `CustomPk`
	- User need to provide a RNG callback for `Pk::from_private_key`, this originally is not a hard requirement, but in C `mbedtls` 3.X this become a hard requirement for blinding to defend side channel attack.
- 💥 `mbedtls/src/ssl/ciphersuites.rs`: Rename `TlsCipherSuite` to `Tls12CipherSuite`, and add enum: `Tls13CipherSuite`, `IanaTlsNamedGroup`, `TLS13SignatureAlgorithms`: these are introduced by TLS 1.3
- `mbedtls/src/ssl/ssl_states.rs`: Add `SslStates` to represent the state of SSL handshake
- Update tests accordingly

**Special code need to notice**:

In `impl` of `std::io::Read` under `mbedtls/src/ssl/io.rs` and `tokio::io::AsyncRead` under `mbedtls/src/ssl/async_io.rs`, there are some code to handle the special case when using `mbedtls` as `client` to connect to a server whose `session ticket` extension is enabled.

This case is found when connecting to `goolge.com`, where Google's server send the `session ticket` after the completion of handshake, which cause `C-mbedtls` throw errors when client is try to read msg data.

## CI changes

- Use [cargo-nextest](https://nexte.st/#cargo-nextest) to run tests
	- Reduce time to run tests
	- Have ability to run some tests in serial
		- tests under `hyper.rs` need to access to `google.com` which has QPS limit
		- some tests function use some system resource, see https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls/tests/support/net.rs

Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
Co-authored-by: Vardhan Thigle <vardhan.thigle@fortanix.com>
Co-authored-by: Raoul Strackx <raoul.strackx@fortanix.com>
bors bot added a commit to fortanix/rust-mbedtls that referenced this pull request Jun 26, 2023
278: Upgrade `mbedtls` to 3.4.0 r=Taowyoo a=Taowyoo

## Overview

[PR #213](#213) introduces some breaking changes:
- Update vendor `mbedtls` code from version `2.28.3` to a `3.4.0`
	- Please checkout C `mbedtls` migration guide for 3.X here [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md) to
- Update the build code for `mbedtls-sys-auto` crate to sync up with vendor code change:
	- Changes in C DEFINE's for configuration
	- Changes in header files
	- Add binding code generation for `static inline` code in C side
	- Removing deprecated cargo features or dependencies
	- Add a cargo feature for TLS 1.3
- Update rust wrapper code in `./mbedtls` to sync up the changes in  `mbedtls-sys-auto`
	- Removing deprecated cargo features or dependencies
	- Update rust code to sync up API changes
	- Add types and functions for TLS 1.3
	- Add a cargo feature for TLS 1.3
	- Update integration tests for new API and TLS 1.3
	- Update dependencies

## Breaking Changes:

**Note**: entries with 💥 are ensured that they will break the downstream.


### Changes in `mbedtls-sys-auto`

#### Vendor code changes

**Upstream changes:**

- 💥 Upgrade vendor `mbeldtls` code to `3.4.0`

**Changes on our side:**

- Cherry picked previous changes in old versions:
	- commit: [vendor change: Adding mpi_force_c_code feature](c8cd406)
- New changes
    - [vendor change: fix time call in tls13 client&server](bafc52d) : This has been merged into upstream, see: Mbed-TLS/mbedtls#7639 .

#### rust code changes

**Features:**

- 💥 `zlib` is removed: support for TLS record-level compression is removed in `mbedtls` 3.X
  - Related C DEFINE `MBEDTLS_ZLIB_SUPPORT` is also removed
- 💥 `legacy_protocols` is removed: all protocols early than TLS 1.2 is removed in `mbedtls` 3.X
  - Related C DEFINE's are also removed: `MBEDTLS_SSL_PROTO_SSL3`, `MBEDTLS_SSL_PROTO_TLS1`, `MBEDTLS_SSL_PROTO_TLS1_1`, `MBEDTLS_SSL_CBC_RECORD_SPLITTING`
- 💥 `pkcs11` is removed: wrapper for `libpkcs11-helper` is removed in `mbedtls` 3.X, see [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md#remove-wrapper-for-libpkcs11-helper)
- 💥 ~~`pkcs12` is removed: because #269 `pkcs12` still defined since it is needed for reading some pkcs8 format keys
- Put TLS 1.3 behind a feature `tls13` : because the dependency of TLS 1.3 in mbedtls 3.X are using a global state RNG which breaks the requirements for FIPS, so this feature enables use to avoid these code from compilation.
- 💥 Deprecated features  `custom_threading` , `custom_time` , `custom_gmtime_r` , `pthread`  are removed

**Dependencies:**

- 💥 Bump `mbedtls-sys` version to `3.4.0`
- 💥 Deprecated dependencies are removed
  - `libz-sys` : support for TLS record-level compression is removed in `mbedtls` 3.X
  - `libc`: `libc` is not needed in `sgx`

**Build code changes:**

Following changes are made according to [3.0-migration-guide.md](https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls-sys/vendor/docs/3.0-migration-guide.md).

- Remove `MBEDTLS_CONFIG_H` in `mbedtls_config.h`
- Remove `#include <mbedtls/check_config.h>`
- `mbedtls-sys/build/bindgen.rs`
	-  Allow `bindgen` to generate bindings for functions, types and variables start with `psa_`, and put them in a sub `mod psa` because they are needed by TLS 1.3
	- Use  `bindgen` experiment feature to generate C function wrapper for C `static inline` functions
- `mbedtls-sys/build/headers.rs` : Update header files
- `mbedtls-sys/build/config.rs` : Remove/add C defines
	- 💥 Added `MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG` : this is added because TLS 1.3 need to use PSA library which need this when feature `std` is off, user need to provide their implementation. In `rus-mbedtls`, one implementation is proveded, see: `mbedtls/src/rng/mod.rs`.

### Changes in `mbedtls-platform-support`

- Added `once_cell` for initializing PSA only one times: see `fn psa_crypto_init()` in `mbedtls-platform-support/src/lib.rs`, this is needed because:
	- > MBEDTLS_USE_PSA_CRYPTO means that X.509 and TLS will use PSA Crypto as much as possible (that is, everywhere except for features that are not supported by PSA Crypto, see "Internal Changes" below for a complete list of exceptions). When it is enabled, you need to call psa_crypto_init() before calling any function from PK, X.509 or TLS;
	- Ref: https://github.com/Mbed-TLS/mbedtls/blob/0b3de6fcec4aa4b23a9ee1e076714cbc796f3ac4/docs/use-psa-crypto.md#general-considerations
- Add function pointer `mbedtls_psa_external_get_random`  which is needed when C DEFINE option `MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG` is turned on. This will be used in case when system default RNG or entropy is no available (for example in SGX)
    - Using function pointer here ensure there is no link time conflict in future when mutiple `rust-mbedtls` is using this crate.
    - User need to call function `set_psa_external_rng_callback` before using any PSA functions or TLS 1.3
- Add rust implementation of `explicit_bzero`, which is needed in SGX. Because in SGX, our [rs-libc](https://github.com/fortanix/rust-sgx/tree/master/rs-libc) does not support this function.
- Update self tests:
  - following are removed:
    - arc4_self_test
    - md2_self_test
    - md4_self_test
    - x509_self_test
    - xtea_self_test
  - following are added:
    - sha384_self_test
    - chacha20_self_test
    - chachapoly_self_test
    - poly1305_self_test
    - sha224_self_test

### Changes in `mbedtls`

**Features:**

- 💥 `zlib`, `legacy_protocols` removed to sync with changes in `mbedtls-sys`
- 💥 `pkcs12` and `pkcs12_rc2` are removed: see #269
- Put TLS 1.3 logic behind a feature `tls13`: check reason above

**Dependencies:**

- 💥 Bump `rust-mbedtls` version to `0.11.0`
- 💥 Bump dependency `mbedtls-sys-auto` version to `3.4.0`
- Added `rstest` `rstest_reuse` `lazy_static` `env_logger`: used for improving code of tests


### Code changes

- Function `EcPoint::mul` now need to pass in a RNG to ensure blinding.
- Add prefix `private_` to some fields of `mbedtls` types under `mbedtls/src/cipher/raw`
- 💥 Removed and added some options in `Error`, `CipherType`, `CipherMode` and `CipherSuite`  to sync with changes in `mbedtls` 3.X
- 💥 `mbedtls/src/pk/ec.rs` : User need to provide a RNG callback for function `EcPoint::mul`, this originally is not a hard requirement, but in C `mbedtls` 3.X this become a hard requirement for blinding to defend side channel attack.
- 💥 `mbedtls/hash` :
	- `Md2` and `Md4` are removed since they are no longer supported in `mbedtls` 3.X
	- fn `pbkdf_pkcs12` is removed since `pkcs12` is removed
- 💥 `mbedtls/pk/mod.rs` :
	- Remove `CustomPk`
	- User need to provide a RNG callback for `Pk::from_private_key`, this originally is not a hard requirement, but in C `mbedtls` 3.X this become a hard requirement for blinding to defend side channel attack.
- 💥 `mbedtls/src/ssl/ciphersuites.rs`: Rename `TlsCipherSuite` to `Tls12CipherSuite`, and add enum: `Tls13CipherSuite`, `IanaTlsNamedGroup`, `TLS13SignatureAlgorithms`: these are introduced by TLS 1.3
- `mbedtls/src/ssl/ssl_states.rs`: Add `SslStates` to represent the state of SSL handshake
- Update tests accordingly

**Special code need to notice**:

In `impl` of `std::io::Read` under `mbedtls/src/ssl/io.rs` and `tokio::io::AsyncRead` under `mbedtls/src/ssl/async_io.rs`, there are some code to handle the special case when using `mbedtls` as `client` to connect to a server whose `session ticket` extension is enabled.

This case is found when connecting to `goolge.com`, where Google's server send the `session ticket` after the completion of handshake, which cause `C-mbedtls` throw errors when client is try to read msg data.

## CI changes

- Use [cargo-nextest](https://nexte.st/#cargo-nextest) to run tests
	- Reduce time to run tests
	- Have ability to run some tests in serial
		- tests under `hyper.rs` need to access to `google.com` which has QPS limit
		- some tests function use some system resource, see https://github.com/fortanix/rust-mbedtls/blob/yx/upgrade-mbedtls/mbedtls/tests/support/net.rs

Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
Co-authored-by: Vardhan Thigle <vardhan.thigle@fortanix.com>
Co-authored-by: Raoul Strackx <raoul.strackx@fortanix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-tls size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS13 code does not correctly call time function
5 participants