Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Improve openssl ext to generate EC keys with custom EC parameters 
This change extends supported parameter when generating EC keys.

Specifically following parameters are now supported: p, a, b, order,
generator, seed, cofactory, g_x, g_y, x, y and d.

Those parameters can be passed to ec field in openssl_pkey_new options.

It also fixes some issues openssl_pkey_get_details related to SM2
support.

Closes GH-9991
  • Loading branch information
@Eno-CN @bukka
Eno-CN authored and bukka committed Jul 6, 2023
1 parent b0a2727 commit 0dadd66
Show file tree
Hide file tree
Showing 5 changed files with 495 additions and 121 deletions.
3 changes: 3 additions & 0 deletions NEWS
View file
Expand Up @@ -10,6 +10,9 @@ PHP NEWS
. Fixed line number of JMP instruction over else block. (ilutov)
. Fixed use-of-uninitialized-value with ??= on assert. (ilutov)

- OpenSSL
. Added support for additional EC parameters in openssl_pkey_new. (Eno-CN)

06 Jul 2023, PHP 8.3.0alpha3

- Core:
Expand Down

9 comments on commit 0dadd66

@remicollet
Copy link
Contributor

@remicollet remicollet commented on 0dadd66 Jul 19, 2023 •
edited

Choose a reason for hiding this comment

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

@Eno-CN @bukka this test is failing in Fedora <= 37 / RHEL <= 8 (ok on Fedora 38 and RHEL 9)

On this system

$ sapi/cli/php -r 'print_r(openssl_get_curve_names());'
Array
(
    [0] => secp224r1
    [1] => secp256k1
    [2] => secp384r1
    [3] => secp521r1
    [4] => prime256v1
)

Test output:

TEST 1/1 [ext/openssl/tests/ecc_custom_params.phpt]
========DIFF========
--
     }
     bool(true)
     Testing openssl_pkey_new with ec explicit parameters
006- bool(true)
007- bool(true)
008- bool(true)
009- Testing openssl_pkey_new with ec missing params 
     
011- Warning: openssl_pkey_new(): Unknown elliptic curve (short) name invalid_curve_name in %s on line %d
012- bool(false)
013- 
014- Warning: openssl_pkey_new(): Missing params: curve_name in %s on line %d
015- bool(false)
016- 
017- Warning: openssl_pkey_new(): Missing params: curve_name or p, a, b, order in %s on line %d
018- bool(false)
019- 
020- Warning: openssl_pkey_new(): Missing params: generator or g_x and g_y in %s on line %d
021- bool(false)
007+ Fatal error: Uncaught TypeError: openssl_pkey_get_details(): Argument #1 ($key) must be of type OpenSSLAsymmetricKey, false given in /work/build/phpmaster/ext/openssl/tests/ecc_custom_params.php:41
008+ Stack trace:
009+ #0 /work/build/phpmaster/ext/openssl/tests/ecc_custom_params.php(41): openssl_pkey_get_details(false)
010+ #1 {main}
011+   thrown in /work/build/phpmaster/ext/openssl/tests/ecc_custom_params.php on line 41
========DONE========
FAIL openssl_*() with OPENSSL_KEYTYPE_EC for ec custom params [ext/openssl/tests/ecc_custom_params.phpt]

Can you please:
add a skip for missing EC ?
or use another more common EC ?
or split the test for this specific EC ?

P.S. after more tests, seems the used EC is "secp224r1" which is available... need to understand why openssl_pkey_new fails...

@Eno-CN
Copy link
Contributor Author

@Eno-CN Eno-CN commented on 0dadd66 Jul 19, 2023

Choose a reason for hiding this comment

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

@remicollet What version of the OpenSSL library are you using?

@bukka
Copy link
Member

@bukka bukka commented on 0dadd66 Jul 19, 2023

Choose a reason for hiding this comment

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

Please also provide OpenSSL config options so we can verify the fix...

@remicollet
Copy link
Contributor

@remicollet remicollet commented on 0dadd66 Jul 19, 2023 •
edited

Choose a reason for hiding this comment

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

OpenSSL Version https://rpms.remirepo.net/rpmphp/zoom.php?rpm=openssl

Strangely, it fails with 3.0.9 on Fedora 37 and passes with the same version on Fedora 38

Please also provide OpenSSL config options so we can verify the fix...

I was thinking all our tests use the ext/openssl/tests/openssl.cnf ?

See openssl.txt

P.S. despite same version, applied patches are very different

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking Fedora OpenSSL maintainer about this in https://bugzilla.redhat.com/2223953

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

From OpenSSL maintainer:

Both Fedora and RHEL do not support arbitrary EC parameters
I would expect the test to fail on RHEL 9 and Fedora 38, too, unless the curve tested happens to match a well-known curve that RHEL 9 and F38 accept.

So I will ignore this test during RPM build.

@remicollet
Copy link
Contributor

Choose a reason for hiding this comment

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

@Eno-CN and @bukka
FYI, RPM build will include a patch to always warn when curve_name is missing (to avoid user frustration on silently failing function)
See https://git.remirepo.net/cgit/rpms/scl-php83/php.git/plain/php-8.3.0-openssl-ec-param.patch?id=16bfe8481842870229de0c47d55091cef3d2e6ad

@bukka
Copy link
Member

@bukka bukka commented on 0dadd66 Jul 24, 2023

Choose a reason for hiding this comment

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

I think we should instead check in build if it is supported and if not and used, it should have warning describing that the feature is not enabled. Might be also good to introduce some user space constant so users can check the support.

@Eno-CN
Copy link
Contributor Author

@Eno-CN Eno-CN commented on 0dadd66 Jul 25, 2023

Choose a reason for hiding this comment

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

@bukka
There may be a risk of security vulnerabilities here, but I'm unsure if any special handling is needed for unrepaired versions of OpenSSL.
If necessary, it may be required to introduce macro definitions to indicate "support for explicit elliptic curve parameters" and then perform conditional compilation in the necessary places (such as creating private keys, parsing public and private keys, etc.) to fix security vulnerabilities.
CVE-2022-0778

Please sign in to comment.