Large values for openssl_csr_sign() $days overflow #16433

Activity

1. YuanchengJiang

   added

   Bug

   Status: Needs Triage

   on Oct 14, 2024
2. cmb69 commented on Oct 14, 2024

   

   cmb69

   on Oct 14, 2024

   Last edited by cmb69

   Member

   More actions

   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.
3. cmb69

   self-assigned this

   on Oct 14, 2024
4. cmb69

   added

   Extension: openssl

   Status: Verified

   and removed

   Status: Needs Triage

   on Oct 14, 2024
5. cmb69

   changed the title [-]openssl_csr_sign in ext/openssl/openssl.c can cause integer overflow[/-] [+]Large values for openssl_csr_sign() $days overflow[/+] on Oct 14, 2024
6. cmb69

   added a commit that references this issue on Oct 14, 2024

   Fix phpGH-16433: Large values for openssl_csr_sign() $days overflow

   ...

   This issue will close once commit 3d70396 is merged into the 'master' branch.

   3d70396
7. cmb69

   linked a pull request that will close this issueFix GH-16433: Large values for openssl_csr_sign() $days overflow #16437on Oct 14, 2024
8. cmb69

   added 3 commits that reference this issue on Oct 16, 2024

   Fix GH-16433: Large values for openssl_csr_sign() $days overflow

   ...

   This issue will close once commit 2bdf2f9 is merged into the 'master' branch.

   2bdf2f9

   Merge branch 'PHP-8.2' into PHP-8.3

   ...

   This issue will close once commit 931762c is merged into the 'master' branch.

   931762c

   Merge branch 'PHP-8.3' into PHP-8.4

   ...

   This issue will close once commit ef1c3b8 is merged into the 'master' branch.

   ef1c3b8
9. cmb69

   closed this as completedin 2bdf2f9on Oct 16, 2024
10. cmb69

    added a commit that references this issue on Oct 16, 2024

    Merge branch 'PHP-8.4'

    ...

    This issue will close once commit 99aa43a is merged into the 'master' branch.

    99aa43a