Fix crashing induced by Zonemaster::LDNS::RR::NSEC3::salt() method#177
Merged
marc-vanderwal merged 2 commits intoNov 21, 2023
Merged
Conversation
The salt() method in the Zonemaster::LDNS::RR::NSEC3 module never worked and often caused the Perl interpreter to crash. This commit fixes many long-standing issues with the affected code. Firstly, the root cause of the crash is a double free resulting from the inappropriate use of ldns_rdf_deep_free() in the code. The ldns_nsec3_salt() function returns a pointer to a ldns_rdf structure which is just a window into the salt field, not a copy of the data. So calling ldns_rdf_deep_free() on that ldns_rdf object causes a part of the original resource record structure to be freed instead. This then results in a double free when the memory for the resource record object is deallocated. Calling ldns_rdf_free() instead fixes the crashing. Secondly, the method doesn’t quite return the salt: it actually returns a string containing the salt preceded by its length byte. This is surprising, not as documented and unlikely to be useful. This problem is fixed by rewriting the entire function so as to return the salt, all the salt and nothing but the salt. Thirdly, the method was also insufficiently covered by unit tests. Tests were added, first to help reproduce the crashes, but also to cover the case of an NSEC3 with non-empty salt. Finally, the method returns undef if the salt is empty. Not only is that documented nowhere, but the choice of doing so is questionable. This commit changes the behavior somewhat in this case: if the salt is empty, an empty string is returned instead; the method only returns undef if there was a problem accessing the salt field.
matsduf
previously approved these changes
Nov 20, 2023
matsduf
left a comment
Contributor
There was a problem hiding this comment.
This looks fine. Are there calls in Engine that have to be updated to match this change?
Contributor
Author
I did a |
matsduf
approved these changes
Nov 20, 2023
tgreenx
approved these changes
Nov 21, 2023
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Purpose
The
salt()method in theZonemaster::LDNS::RR::NSEC3module never worked and often caused the Perl interpreter to crash. This PR fixes many long-standing issues with the affected code.Firstly, the root cause of the crash is a double free resulting from the inappropriate use of
ldns_rdf_deep_free()in the code. Theldns_nsec3_salt()function returns a pointer to aldns_rdfstructure which is just a window into the salt field, not a copy of the data. So callingldns_rdf_deep_free()on thatldns_rdfobject causes a part of the original resource record structure to be freed instead. This then results in a double free when the memory for the resource record object is deallocated. Removing the call to that function fixes the crashing.Secondly, the method doesn’t quite return the salt: it actually returns a string containing the salt preceded by its length byte. This is surprising, not as documented and unlikely to be useful. This problem is fixed by rewriting the entire function so as to return the salt, all the salt and nothing but the salt.
Thirdly, the method was also insufficiently covered by unit tests. Tests were added, first to help reproduce the crashes, but also to cover the case of an NSEC3 with non-empty salt.
Finally, the method returns
undefif the salt is empty. Not only is that documented nowhere, but the choice of doing so is questionable. This PR changes the behavior somewhat in this case: if the salt is empty, an empty string is returned instead; the method only returnsundefif there was a problem accessing the salt field.Context
Found by @tgreenx while updating the implementation of DNSSEC03 (see PR zonemaster/zonemaster-engine#1304).
Changes
salt()method in Zonemaster::LDNS::RR::NSEC3 completelyNote: because the function’s behavior is changed in the case of a resource record with an empty salt, this change should be considered breaking.
How to test this PR
Unit tests should pass.