Nothing Special   »   [go: up one dir, main page]
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

Jump to bottom

Large values for openssl_csr_sign() $days overflow #16433

Closed
YuanchengJiang opened this issue Oct 14, 2024 · 1 comment
Closed

Large values for openssl_csr_sign() $days overflow #16433

YuanchengJiang opened this issue Oct 14, 2024 · 1 comment
Assignees
@cmb69
Labels
Bug Extension: openssl Status: Verified

Comments

@YuanchengJiang
Copy link

Description

The following code:

<?php
$dn = array();
$csr = openssl_csr_new($dn, $privkey, $args);
var_dump(openssl_csr_sign($csr, null, $privkey, PHP_INT_MAX, $args));

Resulted in this output:

/php-src/ext/openssl/openssl.c:3352:56: runtime error: signed integer overflow: 86400 * 9223372036854775807 cannot be represented in type 'long'

PHP Version

nightly

Operating System

ubuntu 22.04
@cmb69
Copy link
Member
cmb69 commented Oct 14, 2024 •
edited
Loading

php-src/ext/openssl/openssl.c

Line 3352 in 39533af

X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*(long)num_days);

Casting an arbitrary zend_long to long is already wrong for LLP64, and useless for other data models we're supporting. Multiplying by 86400 can't properly work. Assuming that negative values for $days should not be supported, I guess something like the following would do:

 ext/openssl/openssl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
index a4b841dca8..f6994a06c5 100644
--- a/ext/openssl/openssl.c
+++ b/ext/openssl/openssl.c
@@ -3281,6 +3281,11 @@ PHP_FUNCTION(openssl_csr_sign)
 		goto cleanup;
 	}
 
+	if (num_days < 0 || num_days > LONG_MAX / 86400) {
+		php_error_docref(NULL, E_WARNING, "Days must be between 0 and %ld", LONG_MAX / 86400);
+		goto cleanup;
+	}
+
 	if (PHP_SSL_REQ_PARSE(&req, args) == FAILURE) {
 		goto cleanup;
 	}
@@ -3349,7 +3354,7 @@ PHP_FUNCTION(openssl_csr_sign)
 		goto cleanup;
 	}
 	X509_gmtime_adj(X509_getm_notBefore(new_cert), 0);
-	X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*(long)num_days);
+	X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*num_days);
 	i = X509_set_pubkey(new_cert, key);
 	if (!i) {
 		php_openssl_store_errors();

Note that LONG_MAX / 86400 is only 3,90524,855 on LLP64; should still be good enough for practical purposes that's totally sufficient.

@cmb69 cmb69 self-assigned this Oct 14, 2024
@cmb69 cmb69 changed the title openssl_csr_sign in ext/openssl/openssl.c can cause integer overflow Large values for openssl_csr_sign() $days overflow Oct 14, 2024
cmb69 added a commit to cmb69/php-src that referenced this issue Oct 14, 2024
@cmb69
Fix phpGH-16433: Large values for openssl_csr_sign() $days overflow
3d70396 
The `offset_sec` parameter of `X509_gmtime_adj()` expects a `long`, but
the `$days` parameter of `openssl_csr_sign()` a `zend_long`.  We must
avoid signed integer overflow (UB), but also must not silently truncate.
Thus we check the given `$days` for the permissible range, and bail out
otherwise.
@cmb69 cmb69 linked a pull request Oct 14, 2024 that will close this issue
Fix GH-16433: Large values for openssl_csr_sign() $days overflow #16437
Closed
cmb69 added a commit that referenced this issue Oct 16, 2024
@cmb69
Merge branch 'PHP-8.2' into PHP-8.3
931762c 
* PHP-8.2:
  Fix GH-16433: Large values for openssl_csr_sign() $days overflow
cmb69 added a commit that referenced this issue Oct 16, 2024
@cmb69
Merge branch 'PHP-8.3' into PHP-8.4
ef1c3b8 
* PHP-8.3:
  Fix GH-16433: Large values for openssl_csr_sign() $days overflow
@cmb69 cmb69 closed this as completed in 2bdf2f9 Oct 16, 2024
cmb69 added a commit that referenced this issue Oct 16, 2024
@cmb69
Merge branch 'PHP-8.4'
99aa43a 
* PHP-8.4:
  Fix GH-16433: Large values for openssl_csr_sign() $days overflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cmb69 cmb69

Labels
Bug Extension: openssl Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-16433: Large values for openssl_csr_sign() $days overflow cmb69/php-src
2 participants
@cmb69 @YuanchengJiang