Remove deprecated X509_NAME_get_text_by_NID()#2389
Remove deprecated X509_NAME_get_text_by_NID()#2389yadij wants to merge 1 commit intosquid-cache:masterfrom
Conversation
|
Build tested, but not yet run-time tested. There are many other OpenSSL 4 issues to fix. |
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Build tested, but not yet run-time tested.
Please do not post untested code for review unless you want to discuss some code design issues or some such. In the latter case, please post a draft PR and ask specific questions to guide that discussion.
There are many other OpenSSL 4 issues to fix.
Please do not post more related PRs until this PR is merged and the current backlog is dealt with.
| int pos = -1; | ||
| pos = X509_NAME_get_index_by_NID(nm, nid, pos); | ||
| if (pos < 0) { | ||
| debugs(83, 3, (pos == -2 ? "Invalid" : "Missing") << " SSL certificate subject name"); |
There was a problem hiding this comment.
In current PR code, negative pos does not always imply that the certificate has an invalid or missing "subject name". The name may be there, but it may be missing the requested name entry or field (e.g., CN). When other change requests are addressed, it will never imply that. The suggestion below reflects other change requests:
| debugs(83, 3, (pos == -2 ? "Invalid" : "Missing") << " SSL certificate subject name"); | |
| debugs(83, 3, (pos == -2 ? "invalid" : "missing") << " certificate subject name entry"); |
Converting nid into human-friendly text inside this error message would be nice, but I do not insist on that.
|
|
||
| if (nameLen > 0) | ||
| return name; | ||
| const auto nm = X509_get_subject_name(x509); |
There was a problem hiding this comment.
Please test nm and exit on lookup errors (with a debugs() message) instead of using nil nm in lower code, even if the current code can cope with nil nm. Doing so helps in triage and makes this code safer long-term.
| int pos = -1; | ||
| pos = X509_NAME_get_index_by_NID(nm, nid, pos); |
There was a problem hiding this comment.
If possible, please make pos constant and apply AAA:
| int pos = -1; | |
| pos = X509_NAME_get_index_by_NID(nm, nid, pos); | |
| const auto pos = X509_NAME_get_index_by_NID(nm, nid, -1); |
|
|
||
| return nullptr; | ||
| const auto str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(nm, pos)); | ||
| xstrncpy(name, reinterpret_cast<const char *>(ASN1_STRING_get0_data(str)), sizeof(name)); |
There was a problem hiding this comment.
ASN1_STRING_get0_data() manual page says that "In general it cannot be assumed that the data returned by ASN1_STRING_get0_data() is null terminated or does not contain embedded nulls.`
To avoid out of bound reads, we should at least be using ASN1_STRING_length() to limit what we read/copy (in addition to sizeof(name), of course). To avoid misleading code readers, we should not use c-string functions for copying this raw data.
We should return nil instead of truncated names when the name buffer is too small.
N.B. AFAICT, "get0" in OpenSSL function names means something like "raw access to an internal buffer" rather than a "c-string".
| } | ||
|
|
||
| return nullptr; | ||
| const auto str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(nm, pos)); |
There was a problem hiding this comment.
Similar lack of error checking concerns apply here, especially since ASN1_STRING_get0_data() is going to crash if given a nil str. Please check every OpenSSL call that this PR adds or modifies.
| int pos = -1; | ||
| pos = X509_NAME_get_index_by_NID(name, nid, pos); | ||
| if (pos < 0) { | ||
| debugs(83, 3, (pos == -2 ? "Invalid" : "Missing") << " SSL attribute name '" << attribute_name << "'"); | ||
| return nullptr; | ||
| } | ||
| auto str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, pos)); | ||
| xstrncpy(buffer, reinterpret_cast<const char *>(ASN1_STRING_get0_data(str)), sizeof(buffer)); |
There was a problem hiding this comment.
Please do not duplicate this non-trivial and error-prone code! Add and reuse an ssl_get_attribute_by_NID() or similar function instead.
| @@ -1060,14 +1060,18 @@ static const char *getSubjectEntry(X509 *x509, int nid) | |||
| return nullptr; | |||
|
|
|||
| // TODO: What if the entry is a UTF8String? See X509_NAME_get_index_by_NID(3ssl). | |||
There was a problem hiding this comment.
AFAICT, this comment (and the matching manual page text) were written for X509_NAME_get_text_by_NID() call that this PR upgrades. The comment becomes misplaced in PR code -- it looks like it applies to X509_get_subject_name(), but it does not.
The comment mentions but does not apply to X509_NAME_get_index_by_NID() that official code did not have, but PR code has, causing additional confusion.
The comment now applies to ASN1_STRING_get0_data() call further below, I guess.
Please move and adjust to address all these problems.
2 participants
OpenSSL v4 deprecates this function and produces build errors
when combined with the Squid default -Wall -Werror.
Replace with the recommended X509_NAME_get_index_by_NID()
function which is also supported by older OpenSSL.