Reset dhm_P and dhm_G if config call repeated; avoid memory leak

[gstrauss]

Description

Reset dhm_P and dhm_G if call to mbedtls_ssl_config_defaults() repeated
to avoid leaking memory.

Signed-off-by: Glenn Strauss gstrauss@gluelogic.com

Status

READY

Requires Backporting

Not required, IMHO, but could be backported. In mbedtls 3.0.0, mbedtls_ssl_config members dhm_P and dhm_G are private, so routines underlying mbedtls_ssl_config_defaults() should avoid leaking memory if mbedtls_ssl_config_defaults() is called more than once. If mbedtls_ssl_config_defaults() is called more than once by an application, then with earlier versions of mbedtls, the application could call mbedtls_mpi_free() on dhm_P and dhm_G.

Todos

• Changelog updated (Changelog.d/xxxxx.txt file provided)
• Backport to 2.28 (Backport 2.28: Reset dhm_P and dhm_G if config call repeated #5416)

Other

Test for memory leak not provided.

Observe that mbedtls_ssl_config_free() calls mbedtls_mpi_free() on dhm_P and dhm_G conf members.
Observe that mbedtls_ssl_config_defaults() calls mbedtls_ssl_conf_dh_param_bin(), and if mbedtls_ssl_config_defaults() is called more than once, then mbedtls_ssl_conf_dh_param_bin() is called more than once, and inside mbedtls_ssl_conf_dh_param_bin(), conf members dhm_P and dhm_G are overwritten. They should be freed before being overwritten. Observer in mbedtls_ssl_conf_dh_param_bin() that if there is an error, then mbedtls_mpi_free() is called on dhm_P and dhm_G conf members. This patch calls mbedtls_mpi_free() on those members before overwriting them, so that memory is not leaked if the members had previously been allocated.

[mpg]

LGTM, thanks!

[mpg]

There's an inconsistency between the labels and the ChangeLog entry as to whether this is a bugfix or an enhancement. I tend to agree that this is a bug: the function's documentation doesn't say that you shouldn't call this more than once, so we should react appropriately if this happens, and at any rate, not leak memory.

Bugs need backporting to all maintained branches; currently there's only one: mbedtls-2.28. Backporting is done by creating a new PR targeting mbedtls-2.28 and linking to and from the original PR (example). @gstrauss could you create a PR for this? (If so, you can either wait for this PR to be fully approved, or optimistically create the backport with just one approval on this PR - for such a small PR I'd go with the optimistic approach, I'm mostly mention this in case you contribute larger PRs in the future.)

[gstrauss]

Submitted backport to 2.28 in #5416

[mpg]

Perfect, thanks! The backport is straightforward (I mean, there were no relevant difference between the two branches that would affect the patch) so only needs one reviewer, but the main PR will need to be reviewed by a second reviewer before both PRs can be merged.

[AndrzejKurek]

LGTM.