mbedtls
mbedtls copied to clipboard
Mbed-TLS
Shared code to free x509 structs like mbedtls_x509_named_data
Description
Shared code to free x509 structs like mbedtls_x509_named_data
First commit is shared code.
Second commit is for discussion: is it excessive to zeroize read-only pointers to data? (Of course, the actual sensitive data should still be zeroized when freed)
Status
READY
Requires Backporting
NO
Todos
- [x] Changelog updated
IMO I agree - I don't see a need to zeroise pointers to sensitive data (although we might do it for simplicity when zeroising an entire struct which includes sensitive data we want to zeroise as well as pointers for which we don't care).
I added a Removals line to ChangeLog.d/mbedtls_asn1_type_free.txt
If you prefer to avoid the ABI change, I could leave mbedtls_asn1_free_named_data( mbedtls_asn1_named_data *entry ); even though it is no longer used. It could be removed at some time in the future along with other changes which change the ABI.
@davidhorstmann-arm mentioned this PR over two weeks ago, but since this PR still does not have a second reviewer, code has subsequently been merged which would have been simpler if it were able to use this PR. I'd like to add the following to this PR, but I do not want to lose the 1 approval already present (approved 2 months ago by @AndrzejKurek)
As @daverodgman commented above, the zeroization of read-only pointers appears unnecessary.
--- a/library/x509.c
+++ b/library/x509.c
@@ -472,7 +472,6 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end,
size_t set_len;
const unsigned char *end_set;
mbedtls_x509_name *head = cur;
- mbedtls_x509_name *prev, *allocated;
/* don't use recursion, we'd risk stack overflow if not optimized */
while( 1 )
@@ -530,18 +529,8 @@ int mbedtls_x509_get_name( unsigned char **p, const unsigned char *end,
error:
/* Skip the first element as we did not allocate it */
- allocated = head->next;
-
- while( allocated != NULL )
- {
- prev = allocated;
- allocated = allocated->next;
-
- mbedtls_platform_zeroize( prev, sizeof( *prev ) );
- mbedtls_free( prev );
- }
-
- mbedtls_platform_zeroize( head, sizeof( *head ) );
+ mbedtls_asn1_free_named_data_list_shallow( head->next );
+ head->next = NULL;
return( ret );
}
@gstrauss apologies for the delay. If you add this change, I'll do the second review and @AndrzejKurek can re-review.
Thank you, @davidhorstmann-arm
I added the patch above and also merged the loops in mbedtls_x509_crt_free() and mbedtls_x509_crl_free()
(Edit: I had to rebase to the HEAD of development in order to get @davidhorstmann-arm change in #6391)
(I do not understand why a review can only be requested from one person at a time)
Ah apologies I should have explained. Earlier I self-requested a review and then removed Gilles as a reviewer to mark the fact that myself and Andrzej were planning to review (only 2 approvals are required on a PR).
It's perfectly fine to request any number of reviewers, but only 2 are needed.
@AndrzejKurek @davidhorstmann-arm
I am hesitant to make further changes to a simple PR for which months have passed between reviews, especially since the window for mbedtls 3.3 appears to be closing.
Can we "go a step further" in a follow-up PR? If not, would you please both agree to the requested changes before I make them?
@gstrauss I'm happy for you to make this change and we'll be able to re-review promptly since it's a small change.
A quick note on adding changes: we usually prefer it if you add changes as new commits rather than amending commits and force-pushing. It makes the history a bit more messy, but it makes re-review much easier because we can review just the newly added parts.
I added deprecation.
@davidhorstmann-arm wrote
A quick note on adding changes: we usually prefer it if you add changes as new commits rather than amending commits and force-pushing. It makes the history a bit more messy, but it makes re-review much easier because we can review just the newly added parts.
You can generally click the 'Compare' button on the right-hand side of each push line to see what changed in each push. This is often useful to review changes as long as the push does not also include a rebase onto tip of development.
Personally, I prefer to review code via git fetch and git diff sequence, which reveals differences via canonical use of source control and allows me to reference other parts of the code.
Additionally, the default in github -- and an option I often opt into -- is that reviewers who are committers to the repository have permission to add or modify commits being reviewed. If you need to make a small change such as fixing a typo or whitespace, feel free to do so in the PR without having to leave a note, wait for the original committer to make the change, then have you context-switch to review it again.
There was an infrastructure glitch on the latest run of the internal CI, but the jobs passed on OpenCI, so CI is ok for merging.
@gilles-peskine-arm Deprecation is not well documented and the reviewers apparently do not all know it well, either.
MBEDTLS_DEPRECATED is not documented. MBEDTLS_DEPRECATED is in include/mbedtls/platform_util.h under the section /* Internal helper macros for deprecating API constants. */
ChangeLog.d/00README.md includes "New deprecations" in the list of permitted changelog entry categories.
include/mbedtls/mbedtls_config.h defines MBEDTLS_DEPRECATED_REMOVED and MBEDTLS_DEPRECATED_WARNING and has documentation, but makes no mention of either MBEDTLS_DEPRECATED or "New deprecations"
@AndrzejKurek @daverodgman due to the missing documentation, I think it would have been more appropriate for my PR to have been accepted as-is, and a follow-up issue filed by you to mark the function with appropriate deprecated tagging. As an organization, the mbedtls team is often as slow as molasses reviewing PRs. I hope you try to be more aware of scope creep in the future, as it slows you down further and frustrates contributors.
Deprecation is not well documented
We've realized that and wrote documentation the same day.
and the reviewers apparently do not all know it well, either.
That's why we have gatekeepers.
an organization, the mbedtls team is often as slow as molasses reviewing PRs
A fact that is frustrating for us as well.
And once again, we ask that you please cooperate by making pull requests easy to review. For example, by avoiding rebasing and instead making changes as new commits.
Additionally, the default in github -- and an option I often opt into -- is that reviewers who are committers to the repository have permission to add or modify commits being reviewed. If you need to make a small change such as fixing a typo or whitespace, feel free to do so in the PR without having to leave a note, wait for the original committer to make the change, then have you context-switch to review it again.
I would need write access to your fork to do that, unless you're talking about suggestions or something else.
Yes. When submitting a PR, I usually check the box Allow edits by maintainers The help text for that is: If checked, users with write access to Mbed-TLS/mbedtls can add new commits to your ... branch. ....
Thank you for submitting code suggestions. Since it does not appear that the mbedtls team values a less noisy commit history, I have directly accepted your code change suggestions using the github gui, adding two new commits.
Please also forward your code suggestion here to @davidhorstmann-arm so that
/* BEGIN_CASE depends_on:!MBEDTLS_DEPRECATED_REMOVED:!MBEDTLS_DEPRECATED_WARNING */
is also reflected in https://github.com/Mbed-TLS/mbedtls-docs/pull/62 "Document deprecation in Mbed TLS"
Please recognize that all of the discussion surround code deprecation is scope creep and ancillary to this PR, which is "Shared code to free x509 structs like mbedtls_x509_named_data". While the code deprecation is useful, it could have been -- and I think should have been -- a separate issue and separate PR.
The failures on OpenCI are timeouts. Internal CI passed, so CI is ok for merging.