Skip to content

Fix negative-size-param crash when OBJ_obj2txt() fails #21008

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
ndossche wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −6
Open

Fix negative-size-param crash when OBJ_obj2txt() fails #21008

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
9 changes: 7 additions & 2 deletions ext/openssl/openssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2317,8 +2317,13 @@ PHP_FUNCTION(openssl_pkey_get_details)
add_assoc_stringl(return_value, "key", pbio, pbio_len);

zend_long ktype = php_openssl_pkey_get_details(return_value, pkey);

add_assoc_long(return_value, "type", ktype);
if (ktype != -2) {
add_assoc_long(return_value, "type", ktype);
} else {
php_openssl_store_errors();
zval_ptr_dtor(return_value);
RETVAL_FALSE;
}

BIO_free(out);
}
Expand Down
10 changes: 8 additions & 2 deletions ext/openssl/openssl_backend_v1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,14 @@ zend_long php_openssl_pkey_get_details(zval *return_value, EVP_PKEY *pkey)
obj = OBJ_nid2obj(nid);
if (obj != NULL) {
int oir_len = OBJ_obj2txt(oir_buf, sizeof(oir_buf), obj, 1);
add_assoc_stringl(&ec, "curve_oid", (char*) oir_buf, oir_len);
ASN1_OBJECT_free(obj);
if (oir_len < 0) {
ktype = -2;
ASN1_OBJECT_free(obj);
break;
} else {
add_assoc_stringl(&ec, "curve_oid", (char*) oir_buf, oir_len);
ASN1_OBJECT_free(obj);
}
}
}

Expand Down
10 changes: 8 additions & 2 deletions ext/openssl/openssl_backend_v3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,8 +668,14 @@ zend_long php_openssl_pkey_get_details(zval *return_value, EVP_PKEY *pkey)
// OpenSSL recommends a buffer length of 80.
char oir_buf[80];
int oir_len = OBJ_obj2txt(oir_buf, sizeof(oir_buf), obj, 1);
add_assoc_stringl(&ary, "curve_oid", oir_buf, oir_len);
ASN1_OBJECT_free(obj);
if (oir_len < 0) {
ktype = -2;
Copy link
Copy Markdown
Member

@bukka bukka Apr 3, 2026

Choose a reason for hiding this comment

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

As this is unlikely to happen, I'm not sure we should add new ktype...

Copy link
Copy Markdown
Member Author

@ndossche ndossche Apr 3, 2026

Choose a reason for hiding this comment

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

In an ideal world we'd have an ADT type like in Rust to signal an error and then we don't need such magic numbers.

Copy link
Copy Markdown
Member

@bukka bukka Apr 4, 2026

Choose a reason for hiding this comment

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

what I'm saying is that this fails only on low memory because that obj is from known nid that is for valid curve name so the only case where this fail would be malloc and freinds failure as far as I see. In such case the process will most likely get killed soon so don't think we need to document new error type for this...

ASN1_OBJECT_free(obj);
break;
} else {
add_assoc_stringl(&ary, "curve_oid", oir_buf, oir_len);
ASN1_OBJECT_free(obj);
}
}
}
}
Expand Down