Skip to content

Remove deprecated X509_NAME_get_text_by_NID() #2389

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.

Sign up for GitHub

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

Open
yadij wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −7
Open

Remove deprecated X509_NAME_get_text_by_NID() #2389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/ssl/gadgets.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

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.

const int nameLen = X509_NAME_get_text_by_NID(
X509_get_subject_name(x509),
nid, name, sizeof(name));

if (nameLen > 0)
return name;
const auto nm = X509_get_subject_name(x509);
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

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);
Comment on lines +1065 to +1066
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

If possible, please make pos constant and apply AAA:

Suggested change
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);

if (pos < 0) {
debugs(83, 3, (pos == -2 ? "Invalid" : "Missing") << " SSL certificate subject name");
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

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:

Suggested change
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.

return nullptr;
}

return nullptr;
const auto str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(nm, pos));
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

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.

xstrncpy(name, reinterpret_cast<const char *>(ASN1_STRING_get0_data(str)), sizeof(name));
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

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 (*name ? name : nullptr);
}

const char *Ssl::CommonHostName(X509 *x509)
Expand Down
9 changes: 8 additions & 1 deletion src/ssl/support.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,14 @@ ssl_get_attribute(X509_NAME * name, const char *attribute_name)
debugs(83, DBG_IMPORTANT, "WARNING: Unknown SSL attribute name '" << attribute_name << "'");
return nullptr;
}
X509_NAME_get_text_by_NID(name, nid, buffer, sizeof(buffer));
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));
Comment on lines +877 to +884
Copy link
Copy Markdown
Contributor

@rousskov rousskov Mar 18, 2026

Choose a reason for hiding this comment

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

Please do not duplicate this non-trivial and error-prone code! Add and reuse an ssl_get_attribute_by_NID() or similar function instead.

}

return *buffer ? buffer : nullptr;
Expand Down
Loading