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.

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

OpenSSL 3.0.8: ed25519 a decode from and then encode to a pem file corrupts the key if fips+base provider is used #20758

Closed
junaruga opened this issue Apr 17, 2023 · 34 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug

Comments

@junaruga
Copy link

At an unit test in the OpenSSL Ruby binding (https://github.com/ruby/openssl), I see an FIPS mode specific issue related to ed25519 encryption. In my understanding, the program reads an ed25519 public key PEM file and convert it to something, then convert it to the PEM file again. The result of the final PEM file should be the same with the original PEM file. But the content is different with the OpenSSL 3.0.8 FIPS mode.

Fortunately I was able to prepare a reproducing program with only C language without Ruby. You can see it below.

The Ruby program

First, just let me share how the issue look like in the OpenSSL Ruby binding. I am testing it on the branch on my forked repository of the ruby/openssl, https://github.com/junaruga/openssl/tree/wip/fips-ed25519-report here.

In the following case with the ed25519 public key,

$ cat ed25519_pub.pem
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----
$ cat /etc/fedora-release 
Fedora release 37 (Thirty Seven)

$ ruby -v
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-linux]

Non-FIPS mode

We expect that the following command returns the original ed25519_pub.pem's content. And it works with the OpenSSL 3.0.8 non-FIPS mode.

$ LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.0.8-debug/lib/ \
  ruby -I lib -e "require 'openssl'; puts OpenSSL::PKey.read(File.read('ed25519_pub.pem')).public_to_pem"
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----

FIPS mode

$ LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.0.8-fips-debug/lib/ \
  OPENSSL_CONF=/home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips.cnf \
  ruby -I lib -e "require 'openssl'; puts OpenSSL::PKey.read(File.read('ed25519_pub.pem')).public_to_pem"
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-----END PUBLIC KEY-----

A reproducing program with C

Here is the program, https://github.com/junaruga/report-openssl-fips-ed25519.

$ cat ed25519_pub.pem
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----
$ gcc --version
gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ gcc -o ed25519 ed25519.c -lcrypto

Non-FIPS mode

The printed text on the last lines after [DEBUG] ossl_membio2str buf->data: are the same with the ed25519_pub.pem.

$ OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.0.8-debug/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.0.8-debug/lib/ossl-modules \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  default
[DEBUG] Got a pkey! 0x24294e0
[DEBUG] It's held by the provider default
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----

FIPS mode

The printed text is different from with the ed25519_pub.pem. Is this an expected behavior?

$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.0.8-fips-debug/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.0.8-fips-debug/lib/ossl-modules \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] Got a pkey! 0x21476a0
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-----END PUBLIC KEY-----
@junaruga junaruga added the issue: question The issue was opened to ask a question label Apr 17, 2023
@t8m t8m added triaged: question The issue contains a question and removed issue: question The issue was opened to ask a question labels Apr 17, 2023
@t8m
Copy link
Member

t8m commented Apr 17, 2023

Hmm... this certainly does not look right.

Can you please paste the contents of the openssl_fips.cnf file?

@junaruga
Copy link
Author

Can you please paste the contents of the openssl_fips.cnf file?

Sure! the openssl_fips.cnf file is below.

$ cat /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips.cnf
config_diagnostics = 1
openssl_conf = openssl_init

.include /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/fipsmodule.cnf
#.include ./fipsmodule.cnf

[openssl_init]
providers = provider_sect
alg_section = algorithm_sect

[provider_sect]
fips = fips_sect
base = base_sect

[base_sect]
activate = 1

[algorithm_sect]
default_properties = fips=yes

And the fipsmodule.cnf is below.

$ cat /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/fipsmodule.cnf
[fips_sect]
activate = 1
conditional-errors = 1
security-checks = 1
module-mac = XX:XX:XX... (I am masking the info here just in case)

@t8m
Copy link
Member

t8m commented Apr 17, 2023

What happens if you remove the default_properties setting?

Actually the ED25519 algorithm is not FIPS approved so it is not supported with fips=yes property. That still does not mean the failure mode should be to output a bogus public key. Instead the functions should fail to import/export ED25519 keys completely.

@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch branch: 3.1 Merge to openssl-3.1 and removed triaged: question The issue contains a question labels Apr 17, 2023
@t8m t8m changed the title OpenSSL 3.0 FIPS: ed25519 a conversion to the pem file wrong? OpenSSL 3.0.8 FIPS: ed25519 a decode from and then encode to a pem file wrong? Apr 17, 2023
@junaruga
Copy link
Author

What happens if you remove the default_properties setting?

Sure. I tested it with the config file removing the default_properties setting. And the result is same with the case of the openssl_fips.cnf with default_properties setting.

$ diff -u /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips.cnf /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips_without_default_properties.cnf
--- /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips.cnf	2023-03-29 11:16:26.411563394 +0200
+++ /home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips_without_default_properties.cnf	2023-04-17 19:56:06.862567059 +0200
@@ -16,4 +16,3 @@
 activate = 1
 
 [algorithm_sect]
-default_properties = fips=yes
$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.0.8-fips-debug/ssl/openssl_fips_without_default_properties.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.0.8-fips-debug/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.0.8-fips-debug/lib/ossl-modules \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] Got a pkey! 0x213c6c0
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-----END PUBLIC KEY-----

@junaruga
Copy link
Author

I reported this issue with the ed25519 on the FIPS mode. And a similar issue outputting the bogus public key also happens with the x25519 pem file on the FIPS mode too.

@t8m t8m added the severity: regression The issue/pr is a regression from previous released version label Apr 25, 2023
rkarmaka98 added a commit to rkarmaka98/openssl that referenced this issue Apr 30, 2023
rkarmaka98 added a commit to rkarmaka98/openssl that referenced this issue Apr 30, 2023
@levitte
Copy link
Member

levitte commented May 1, 2023

What happens if you remove the default_properties setting?

Actually the ED25519 algorithm is not FIPS approved so it is not supported with fips=yes property.

Not according to our 3.0 code:

{ PROV_NAMES_ED25519, FIPS_DEFAULT_PROPERTIES, ossl_ed25519_signature_functions },

{ PROV_NAMES_ED25519, FIPS_DEFAULT_PROPERTIES, ossl_ed25519_keymgmt_functions,
PROV_DESCS_ED25519 },

(the same goes in 3.1, btw)

That still does not mean the failure mode should be to output a bogus public key. Instead the functions should fail to import/export ED25519 keys completely.

It shouldn't fail completely in 3.0. But, I do wonder why a bogus key is being output...

@levitte
Copy link
Member

levitte commented May 1, 2023

UPDATE: NEVER MIND THIS, that's of course the first DER decode attempt.

I did a little bit of debugging of the example program, and when studying the decoder ('cause it seems that's where whings aren't going quite right), a break on ossl_d2i_ED25519_PUBKEY and then going on frame up lead me to this disturbing discovery:

(gdb) f 1
#1  0x00007ffff7d69d82 in der2key_decode (vctx=0x5555556a9460, 
    cin=0x5555556a97a0, selection=2, data_cb=0x7ffff7be4c76 <decoder_process>, 
    data_cbarg=0x7fffffffd2f0, 
    pw_cb=0x7ffff7c61105 <ossl_pw_passphrase_callback_dec>, 
    pw_cbarg=0x555555685108)
    at providers/implementations/encode_decode/decode_der2key.c:229
229	            key = ctx->desc->d2i_PUBKEY(NULL, &derp, der_len);
(gdb) p derp
$1 = (const unsigned char *) 0x5555556a8930 "-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEAPUAX"

That's not DER that derp is pointing at...

@paulidale
Copy link
Contributor

The 3.0 security policy forbids EdDSA operations. Refer to table 8 on page 24.

@levitte
Copy link
Member

levitte commented May 3, 2023

The 3.0 security policy forbids EdDSA operations. Refer to table 8 on page 24.

Yeah, but as was previously said, our code says something different, and returns a bogus key. The issue is very likely not so much about the abuse of the FIPS module as it's a problem with what data gets passed over an export/import dance when the decoder and the keymgmt are in different providers.

I'm starting to have it my mind to put together an experiment where the decoders and encoders are removed from the default provider, i.e. only exist in the base provider, just to see what falls apart.

@junaruga
Copy link
Author

How is the progress of this issue?

@junaruga
Copy link
Author

junaruga commented Jul 14, 2023

I tested this issue with the OpenSSL latest master branch 1e398bec538978b9957e69bf9e12b3c626290bea by using my reproducing program on my local Fedora Linux 38.

$ gcc --version
gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4)
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Install OpenSSL master

I installed the OpenSSL like this from the GitHub source.

$ ./Configure \
  --prefix=${HOME}/.local/openssl-3.2.0.dev-fips-debug-1e398bec53 \
  --libdir=lib \
  shared \ 
  enable-fips \
  enable-trace \
  -O0 -g3 -ggdb3 -gdwarf-5
$ make -j$(nproc)
$ make install

I added the FIPS config file as usual.

$ vi /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl/openssl_fips.cnf
config_diagnostics = 1
openssl_conf = openssl_init

# Need to set the absolute path of the fipsmodule.cnf.
# https://github.com/openssl/openssl/issues/17704
.include /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl/fipsmodule.cnf
#.include fipsmodule.cnf

[openssl_init]
providers = provider_sect
alg_section = algorithm_sect

[provider_sect]
fips = fips_sect
base = base_sect

[base_sect]
activate = 1

[algorithm_sect]
default_properties = fips=yes

Run the reproducing program

I compiled the reproducing program like this. Interestingly the behavior is different from the OpenSSL 3.0.8.

$ gcc -I /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/include \
  -L /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib \
  -O0 -g3 -ggdb3 -gdwarf-5 \
  -o ed25519 ed25519.c -lcrypto

I ran the program on the FIPS case like this. It failed with segmentation fault.

$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
Could not parse PKey
PEM_write_bio_PUBKEY
Segmentation fault (core dumped)

I also ran the program in the non-FIPS case. It succeeded as expected

$ OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  default
[DEBUG] Got a pkey! 0x1964dc0
[DEBUG] It's held by the provider default
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----

Debug

I debugged the FIPS case with the GDB like this. It seems that the value of the b->method in crypto/bio/bio_lib.c#BIO_ctrl is something wrong.

$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib/ossl-modules \
  gdb --args ./ed25519 ed25519_pub.pem
Reading symbols from ./ed25519...
(gdb) set environment LD_LIBRARY_PATH /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-1e398bec53/lib
(gdb) r
Starting program: /home/jaruga/var/git/report-openssl-fips-ed25519/ed25519 ed25519_pub.pem
[Thread debugging using libthread_db enabled]                                                         
Using host libthread_db library "/lib64/libthread_db.so.1".
[DEBUG] Loaded providers:
  fips
  base
Could not parse PKey
PEM_write_bio_PUBKEY

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff78fab4c in BIO_ctrl (b=0x53f3b0, cmd=115, larg=0, parg=0x7fffffffd798)
    at crypto/bio/bio_lib.c:666
666	    if (b->method == NULL || b->method->ctrl == NULL) {
(gdb) p *b
$1 = {libctx = 0x551c7f, method = 0xdba217e81ce8af5f, callback = 0x0, callback_ex = 0x0, 
  cb_arg = 0x0, init = 1, shutdown = 1, flags = 0, retry_reason = 0, num = -1, ptr = 0x551cc0, 
  next_bio = 0x0, prev_bio = 0x0, references = {val = 0}, num_read = 0, num_write = 0, ex_data = {
    ctx = 0x0, sk = 0x0}}
(gdb) p b->method->ctrl
Cannot access memory at address 0xdba217e81ce8af9f
(gdb) p b->method
$2 = (const BIO_METHOD *) 0xdba217e81ce8af5f
(gdb) p *(b->method)
Cannot access memory at address 0xdba217e81ce8af5f
(gdb) bt
#0  0x00007ffff78fab4c in BIO_ctrl (b=0x53f3b0, cmd=115, larg=0, parg=0x7fffffffd798)
    at crypto/bio/bio_lib.c:666
#1  0x000000000040174d in ossl_membio2str (bio=0x53f3b0) at ed25519.c:149
#2  0x0000000000401721 in ossl_pkey_export_spki (pkey=0x0, to_der=0) at ed25519.c:141
#3  0x0000000000401896 in main (argc=2, argv=0x7fffffffd938) at ed25519.c:187

@junaruga
Copy link
Author

junaruga commented Jul 14, 2023

I executed git-bisect to find which commit is the first commit causing this segmentation fault between the latest master branch (1e398bec538978b9957e69bf9e12b3c626290bea) and the tag openssl-3.0.8 by using the script git-bisect.sh.

$ pwd
/home/jaruga/git/openssl2
$ git bisect start 1e398bec538978b9957e69bf9e12b3c626290bea openssl-3.0.8

$ git bisect run ~/git/report-openssl-fips-ed25519/git-bisect.sh
...
9fa553247874728cee8ca0ece9aaed476eb0f303 is the first bad commit
commit 9fa553247874728cee8ca0ece9aaed476eb0f303
Author: Pauli <pauli@openssl.org>
Date:   Mon Jan 9 11:25:55 2023 +1100

    fips: make EdDSA unapproved for FIPS
    
    Likewise for the related ECX key exchanges.
    
    NIST is mandating this until FIPS 186-5 is finalised.
    
    Reviewed-by: Hugo Landau <hlandau@openssl.org>
    Reviewed-by: Tomas Mraz <tomas@openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/20020)

 providers/fips/fipsprov.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
bisect found first bad commit

And the result shows 9fa5532 is the commit.

And here is the result of the git bisect log just in case.

$ git bisect log
# bad: [1e398bec538978b9957e69bf9e12b3c626290bea] Add CHANGES.md and NEWS.md entries for CVE-2023-2975
# good: [31157bc0b46e04227b8468d3e6915e4d0332777c] Prepare for release of 3.0.8
git bisect start '1e398bec538978b9957e69bf9e12b3c626290bea' 'openssl-3.0.8'
# good: [59f4a51a7f2c53b9fd161b032d0fcb8a85f4f19d] Add a test for verifying an email with a bad othername type
git bisect good 59f4a51a7f2c53b9fd161b032d0fcb8a85f4f19d
# good: [e9809f8a09147bc27f974caa908b04439c006625] Fix error return values from BIO_ctrl_(w)pending()
git bisect good e9809f8a09147bc27f974caa908b04439c006625
# bad: [da81f1e563c80a1d4ab82e545f3f5ba6e715267e] Expand some comments in the header file
git bisect bad da81f1e563c80a1d4ab82e545f3f5ba6e715267e
# good: [edaab86dc001603741f5b5e406afc1cc3a1c4e6e] aes: add AES-GCM-SIV modes to the FIPS provider
git bisect good edaab86dc001603741f5b5e406afc1cc3a1c4e6e
# bad: [c4edfa220e6d3705a0c6299463c83e61fd5f9d2c] Bump actions/setup-python from 4.4.0 to 4.5.0
git bisect bad c4edfa220e6d3705a0c6299463c83e61fd5f9d2c
# good: [55e2dd8c3162d7313d9408cb20fca8a4fe6e6f5a] 80-test_cms.t: Fix rsapssSaltlen check on MinGW
git bisect good 55e2dd8c3162d7313d9408cb20fca8a4fe6e6f5a
# bad: [3a37c9235de465fe8d557b32f0178bfad0c09908] QUIC: Complete the implementation of the RX depacketiser in terms of QUIC_CHANNEL
git bisect bad 3a37c9235de465fe8d557b32f0178bfad0c09908
# bad: [f71ae05a4d22d52780fc7cfc7e60710b74fd3dd7] QUIC: Dummy Handshake Layer for Prototyping
git bisect bad f71ae05a4d22d52780fc7cfc7e60710b74fd3dd7
# good: [c2ae89148343750e420b72ef1b709ebbc16e47b8] In OSSL_PARAM_set_BN(), make sure that the data_size field is at least 1
git bisect good c2ae89148343750e420b72ef1b709ebbc16e47b8
# good: [7efc653c43851dcbc3ec043baded029c7d31ab9f] Make RSA_generate_multi_prime_key() not segfault if e is NULL.
git bisect good 7efc653c43851dcbc3ec043baded029c7d31ab9f
# bad: [e5d575686efb280af08c3fd307a649ed2a942ce3] QUIC ACKM: Add support for psuedo-loss
git bisect bad e5d575686efb280af08c3fd307a649ed2a942ce3
# bad: [836080a89a1f5e45dac4e0df76b9270587f65d5b] Support all five EdDSA instances from RFC 8032
git bisect bad 836080a89a1f5e45dac4e0df76b9270587f65d5b
# bad: [9fa553247874728cee8ca0ece9aaed476eb0f303] fips: make EdDSA unapproved for FIPS
git bisect bad 9fa553247874728cee8ca0ece9aaed476eb0f303
# first bad commit: [9fa553247874728cee8ca0ece9aaed476eb0f303] fips: make EdDSA unapproved for FIPS

@paulidale
Copy link
Contributor

ed25519 is not FIPS approved. Your default properties is mandated only FIPS approved algorithms are used. Consequently, ed25519 is not being fetched.

There is no fix for this that remains FIPS compliant.

The reason ed25519 is in the FIPS provider is historic and based on NIST changing their mind about permitting it as a vendor affirmed algorithm. For the original 3.0 validation, we were allowed to include it. Late last year, NIST had a change of mind and mandated that it not be included so we had to adapt. The non-compliance is covered in the security policy which is required reading since it imposes other restrictions on the building and use of the FIPS provider.

@junaruga
Copy link
Author

junaruga commented Jul 17, 2023

@paulidale thank you for explaining the context.

So, for example using my reproducer ed25519.c calling BIO_get_mem_ptr at the line 149, how should I implement the logic by using the OpenSSL C APIs to return an error message in the cases of the not-approved encryption algorithms including ed25519 in our OpenSSL Ruby binding?

Do you think the behavior of the BIO_get_mem_ptr aborting with segmentation fault in the cases is correct?

The non-compliance is covered in the security policy which is required reading since it imposes other restrictions on the building and use of the FIPS provider.

Could you share the link of the security policy document? I would like to know why the commits to make the ed25519 unapproved is backported to the openssl-3.1 branch, but not to the openssl-3.0 branch.

@paulidale
Copy link
Contributor

paulidale commented Jul 17, 2023

The downloads page has links to the security policy documents.

OpenSSL 3.0 has banned EdDSA by policy, so a technical fix isn't required. This is why you must read the security policy and check for updates because the policy does change over time.

@paulidale
Copy link
Contributor

As for the segmentation fault, are you certain that everything passed in was correct?
The call shouldn't fault but it doesn't check what the calling application gives it....

@t8m
Copy link
Member

t8m commented Jul 17, 2023

The ossl_pkey_read_generic() should check whether the pkey is non-NULL in between those tries and return early if you already loaded the key. However that is not the problem.

What does the call EVP_PKEY_has(pkey, EVP_PKEY_PUBLIC_KEY) return on the returned pkey?

@junaruga
Copy link
Author

@paulidale thanks for the link! I found the "security policy" links there.

@paulidale and @t8m thanks for the help. As you said, my reproducing program was wrong. And even when the ossl_pkey_read_generic returns the pkey (= NULL), the program proceeds to the next step the ossl_pkey_export_spki calling the ossl_membio2str calling the BIO_get_mem_ptr aborting.

I refactored the program with more strict error handling, and created the ed25519_2.c. And the pkey is already NULL in the ossl_pkey_read_generic in the FIPS case.

What does the call EVP_PKEY_has(pkey, EVP_PKEY_PUBLIC_KEY) return on the returned pkey?

Sure. I will try it. But where is the EVP_PKEY_has function actually defined? The openssl/evp.h? I did grep the header files by the EVP_PKEY_has on the master branch. And I couldn't find it.

@junaruga
Copy link
Author

junaruga commented Jul 17, 2023

I refactored the program with more strict error handling, and created the ed25519_2.c. And the pkey is already NULL in the ossl_pkey_read_generic in the FIPS case.

As a reference, here is the result in the FIPS case on the master branch.

$ gcc \
  -I "/home/jaruga/git/openssl/dest/include/" \
  -L "/home/jaruga/git/openssl/dest/lib/" \
  -O0 -g3 -ggdb3 -gdwarf-5 \
  -o ed25519 ed25519_2.c -lcrypto

$ OPENSSL_CONF="$(pwd)/openssl_fips.cnf" \
  OPENSSL_CONF_INCLUDE="/home/jaruga/git/openssl/dest/ssl" \
  OPENSSL_MODULES="/home/jaruga/git/openssl/dest/lib/ossl-modules" \
  LD_LIBRARY_PATH="/home/jaruga/git/openssl/dest/lib" \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] Failed to get the pkey.
[DEBUG] Could not parse a PKey

$ echo $?
1

@junaruga
Copy link
Author

@paulidale

OpenSSL 3.0 has banned EdDSA by policy, so a technical fix isn't required.

In both security policy documents "openssl-3.0.0-security-policy-2023-01-26.pdf" "openssl-3.0.8-security-policy-2023-05-05.pdf" - "Cryptographic Algorithms", there is the following part. Does it mean the "OpenSSL 3.0 has banned EdDSA by policy", right?

The module implements the following non-Approved algorithms:

Ed448 - Digital Signature Generation
Ed25519 - Digital Signature Generation

So, maybe a C program using OpenSSL APIs needs to handle this case as an error case. How can I implement it, considering the OpenSSL 3.1, 3.0, 1.1, 1.0 cases? A hint is helpful.

@t8m
Copy link
Member

t8m commented Jul 17, 2023

What does the call EVP_PKEY_has(pkey, EVP_PKEY_PUBLIC_KEY) return on the returned pkey?

Sure. I will try it. But where is the EVP_PKEY_has function actually defined? The openssl/evp.h? I did grep the header files by the EVP_PKEY_has on the master branch. And I couldn't find it.

Oops, sorry, this isn't a public function yet.

@t8m
Copy link
Member

t8m commented Jul 17, 2023

So, maybe a C program using OpenSSL APIs needs to handle this case as an error case. How can I implement it, considering the OpenSSL 3.1, 3.0, 1.1, 1.0 cases? A hint is helpful.

I do not see a need to special case anything. You just do proper error handling and if the decode fails it means the key is not supported.

However originally this issue was about some problem with importing/exporting the keys to the FIPS module if the FIPS=yes is not set. I am curious if you can still reproduce the original report if you use FIPS module but do not set FIPS=yes.

@junaruga
Copy link
Author

I do not see a need to special case anything. You just do proper error handling and if the decode fails it means the key is not supported.

@t8m Note my report and my test with ed25519_2.c after reporting since finding the commit by git biscect is for the master branch of the openssl/openssl (OpenSSL 3.2.dev ), not for OpenSSL 3.0.

The problem is the OpenSSL 3.0 (3.0.8) FIPS case. Because in the case, the decode still succeeds, returning a wrong text. So, I thought our program using OpenSSL APIs needs to handle the case as an error.

However originally this issue was about some problem with importing/exporting the keys to the FIPS module if the FIPS=yes is not set. I am curious if you can still reproduce the original report if you use FIPS module but do not set FIPS=yes.

The original issue was for OpenSSL 3.0.8 (3.0). Now when testing with OpenSSL 3.0.9, I can reproduce the issue of the original report.

$ gcc \
  -I "/home/jaruga/.local/openssl-3.0.9-fips-debug/include/" \
  -L "/home/jaruga/.local/openssl-3.0.9-fips-debug//lib/" \
  -O0 -g3 -ggdb3 -gdwarf-5 \
  -o ed25519 ed25519_2.c -lcrypto
  
$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.0.9-fips-debug/ssl/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE="/home/jaruga/.local/openssl-3.0.9-fips-debug/ssl" \
  OPENSSL_MODULES="/home/jaruga/.local/openssl-3.0.9-fips-debug/lib/ossl-modules" \
  LD_LIBRARY_PATH="/home/jaruga/.local/openssl-3.0.9-fips-debug/lib" \
  ./ed25519 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0x9734c0
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-----END PUBLIC KEY-----

$ echo $?
0

@paulidale
Copy link
Contributor

Does it mean the "OpenSSL 3.0 has banned EdDSA by policy", right?

Yes, just as I mentioned earlier.

So, maybe a C program using OpenSSL APIs needs to handle this case as an error case. How can I implement it, considering the OpenSSL 3.1, 3.0, 1.1, 1.0 cases? A hint is helpful.

This is where it gets messy. 1.1 and 1.0 don't know about FIPS, so no problem for them. In 3.1 they are correctly flagged in the FIPS provider, so again no problem so long as you do error checking. 3.0 is the problematic case. If you are using the 3.0 FIPS provider, it is your responsibility to enforce that no EdDSA keys are used. This can be done by loading the key regardless and using EVP_PKEY_is_a, checking for either ed25519 or ed448 and failing if so.

@junaruga
Copy link
Author

@paulidale All right. I just wanted to confirm just in case. And thanks for explaining the suggested implementation. It's helpful.

OpenSSL 3.0 has banned EdDSA by policy, so a technical fix isn't required.

I still have a question for your comment above. This means that we shouldn't fix for this issue in OpenSSL 3.0 (openssl-3.0 branch)? Below is a possible fix on the openssl-3.0 branch. If I send a pull-request to fix this issue with the unit test if it is needed, could you review it?

diff --git a/providers/fips/fipsprov.c b/providers/fips/fipsprov.c
index 6a88039423..72e6fb13cf 100644
--- a/providers/fips/fipsprov.c
+++ b/providers/fips/fipsprov.c
@@ -391,8 +391,8 @@ static const OSSL_ALGORITHM fips_signature[] = {
 #endif
     { PROV_NAMES_RSA, FIPS_DEFAULT_PROPERTIES, ossl_rsa_signature_functions },
 #ifndef OPENSSL_NO_EC
-    { PROV_NAMES_ED25519, FIPS_DEFAULT_PROPERTIES, ossl_ed25519_signature_functions },
-    { PROV_NAMES_ED448, FIPS_DEFAULT_PROPERTIES, ossl_ed448_signature_functions },
+    { PROV_NAMES_ED25519, FIPS_UNAPPROVED_PROPERTIES, ossl_ed25519_signature_functions },
+    { PROV_NAMES_ED448, FIPS_UNAPPROVED_PROPERTIES, ossl_ed448_signature_functions },
     { PROV_NAMES_ECDSA, FIPS_DEFAULT_PROPERTIES, ossl_ecdsa_signature_functions },
 #endif
     { PROV_NAMES_HMAC, FIPS_DEFAULT_PROPERTIES,
@@ -436,9 +436,9 @@ static const OSSL_ALGORITHM fips_keymgmt[] = {
       PROV_DESCS_X25519 },
     { PROV_NAMES_X448, FIPS_DEFAULT_PROPERTIES, ossl_x448_keymgmt_functions,
       PROV_DESCS_X448 },
-    { PROV_NAMES_ED25519, FIPS_DEFAULT_PROPERTIES, ossl_ed25519_keymgmt_functions,
+    { PROV_NAMES_ED25519, FIPS_UNAPPROVED_PROPERTIES, ossl_ed25519_keymgmt_functions,
       PROV_DESCS_ED25519 },
-    { PROV_NAMES_ED448, FIPS_DEFAULT_PROPERTIES, ossl_ed448_keymgmt_functions,
+    { PROV_NAMES_ED448, FIPS_UNAPPROVED_PROPERTIES, ossl_ed448_keymgmt_functions,
       PROV_DESCS_ED448 },
 #endif
     { PROV_NAMES_TLS1_PRF, FIPS_DEFAULT_PROPERTIES, ossl_kdf_keymgmt_functions,

@paulidale
Copy link
Contributor

That patch would fix the problem but with the side effect of not being FIPS compliant. Any change to the source code, no matter how slight, means not FIPS anymore. Currently you must use the released 3.0.0 or 3.0.8 code unmodified.

The technical committee discussed this at length and decided that the paperwork change banning these algorithms was good enough and that a patch to also "fix" the code wasn't required. This approach is fine under the FIPS 140-2 standard that we validated even though it places extra burden on the users of the validation.

@t8m
Copy link
Member

t8m commented Jul 18, 2023

We are really mixing up two issues here which should not be mixed up. One is the problem of whether ED25519 and ED448 are allowed under FIPS or not - simply currently not and an application which wants to be FIPS compliant must not use them. However I do not think this should be solved in Ruby bindings. This is (at least in case of FIPS 140-2 -> 3.0.x FIPS modules) purely application dependent decision and Ruby bindings should NOT try to block it on its own.

However this still does not resolve the problem of the wrong handling of ED25519 keys on import/export in case the support is enabled in the FIPS module - we absolutely need to investigate the cause and fix it.

@t8m t8m changed the title OpenSSL 3.0.8 FIPS: ed25519 a decode from and then encode to a pem file wrong? OpenSSL 3.0.8: ed25519 a decode from and then encode to a pem file corrupts the key if fips+base provider is used Jul 18, 2023
@junaruga
Copy link
Author

junaruga commented Jul 19, 2023

That patch would fix the problem but with the side effect of not being FIPS compliant. Any change to the source code, no matter how slight, means not FIPS anymore. Currently you must use the released 3.0.0 or 3.0.8 code unmodified.

@paulidale Sure. Thanks for explaining the situation.

We are really mixing up two issues here which should not be mixed up. One is the problem of whether ED25519 and ED448 are allowed under FIPS or not - simply currently not and an application which wants to be FIPS compliant must not use them. However I do not think this should be solved in Ruby bindings. This is (at least in case of FIPS 140-2 -> 3.0.x FIPS modules) purely application dependent decision and Ruby bindings should NOT try to block it on its own.

However this still does not resolve the problem of the wrong handling of ED25519 keys on import/export in case the support is enabled in the FIPS module - we absolutely need to investigate the cause and fix it.

@t8m I understood that we shouldn't mix the 2 issues. Thanks for mentioning it. I opened another issue ticket #21493 with a x25519 pem file. Maybe the cause is the same with this issue with an ed25519 pem file in OpenSSL 3.0. However, we can reproduce the issue with x25519 in OpenSSL master branch. I thought that it's more convenient to debug and fix the issue.

My concern about not to fix this issue with ed25519 and ed448 in OpenSSL 3.0 on OpenSSL Ruby binding side is that we may see the repeated false positive issue tickets to be opened by someone if we don't fix it.

@t8m
Copy link
Member

t8m commented Jul 19, 2023

My concern about not to fix this issue with ed25519 and ed448 in OpenSSL 3.0 on OpenSSL Ruby binding side is that we may see the repeated false positive issue tickets to be opened by someone if we don't fix it.

Can you please elaborate what would be reported against Ruby bindings? Because I do not understand what you would like to fix in the bindings.

@junaruga
Copy link
Author

Can you please elaborate what would be reported against Ruby bindings? Because I do not understand what you would like to fix in the bindings.

@t8m Sure. One of the issues reported by me is a mismatch between the original pem file and encoded and decoded text in ed25519 and x25519 in OpenSSL 3.0. ruby/openssl#603 (comment) is the report. As you know, I already opened the issue tickets in this repository. And there are other unit test failures (ruby/openssl#603 (comment)) in the OpenSSL Ruby binding's CI. I am trying to pass all the unit tests in OpenSSL 3.1 and 3.0 FIPS CI cases.

I think a reason that I wanted to implement the error handling for the ed25519 and ed448 is that I still don't notice about how much the demerit of implementing the error handling is. However, rethinking again, I changed my mind I will not implement the error handling of the ed25519 and ed448 case in OpenSSL 3.0 in the OpenSSL Ruby binding by following your suggestion. I can just note the situation (the ed25519 and ed448 crypto is only banned in the security policy document, but still allowed in the code) as a comment in the code of the unit tests of OpenSSL Ruby binding.

@t8m
Copy link
Member

t8m commented Jul 21, 2023

One of the issues reported by me is a mismatch between the original pem file and encoded and decoded text in ed25519 and x25519 in OpenSSL 3.0. ruby/openssl#603 (comment) is the report. As you know, I already opened the issue tickets in this repository. And there are other unit test failures (ruby/openssl#603 (comment)) in the OpenSSL Ruby binding's CI. I am trying to pass all the unit tests in OpenSSL 3.1 and 3.0 FIPS CI cases.

This issue is plain bug in OpenSSL and should be fixed by #21519

@junaruga
Copy link
Author

junaruga commented Jul 21, 2023

This issue is plain bug in OpenSSL and should be fixed by #21519

Thank you for the PR! Let me test the fixes with the OpenSSL on the head of the openssl-3.0 branch.

@junaruga
Copy link
Author

junaruga commented Aug 8, 2023

I think we can close this issue ticket now as the commits on the PR #21519 (comment) were merged to the openssl-3.0 branch.

@t8m
Copy link
Member

t8m commented Aug 8, 2023

Yep, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 severity: regression The issue/pr is a regression from previous released version triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants