Skip to content
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

Fix potential NULL pointer dereference #1067

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Fix potential NULL pointer dereference #1067

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
4 changes: 4 additions & 0 deletions crypto/x509/name_print.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@ static int do_name_ex(BIO *out, const X509_NAME *n, int indent,
} else {
Copy link
Contributor

@nebeid nebeid Jun 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From the documentation of OBJ_obj2nid() called on l. 174,

aws-lc/include/openssl/obj.h

Lines 110 to 113 in cfce39d

// OBJ_obj2nid returns the nid corresponding to |obj|, or |NID_undef| if no
// such object is known.
OPENSSL_EXPORT int OBJ_obj2nid(const ASN1_OBJECT *obj);

the else branch should be for a known nid. If we prefer to be safe still, maybe the check for NULL would be inside the else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's safer to have the NULL check right before calling strlen unconditionally?

objbuf = OBJ_nid2sn(fn_nid);
}
if (objbuf == NULL) {
return -1;
}

int objlen = strlen(objbuf);
if (!maybe_write(out, objbuf, objlen) ||
!maybe_write(out, sep_eq, sep_eq_len)) {
Expand Down
3 changes: 3 additions & 0 deletions crypto/x509/x509_req.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ int X509_REQ_get_attr_by_OBJ(const X509_REQ *req, const ASN1_OBJECT *obj,
for (; lastpos < n; lastpos++) {
const X509_ATTRIBUTE *attr =
sk_X509_ATTRIBUTE_value(req->req_info->attributes, lastpos);
if (attr == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this can happen, since n was set the number of attributes, if I understood correctly. There is a similar code here

aws-lc/crypto/x509/t_req.c

Lines 153 to 155 in 8402a6e

for (i = 0; i < sk_X509_ATTRIBUTE_num(sk); i++) {
X509_ATTRIBUTE *a = sk_X509_ATTRIBUTE_value(sk, i);
ASN1_OBJECT *aobj = X509_ATTRIBUTE_get0_object(a);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I had the same thought but I'm not 100% certain it can't happen. The additional check is very cheap anyway, so no harm in having it.

return -1;
}
if (OBJ_cmp(attr->object, obj) == 0) {
return lastpos;
}
Expand Down