Decoder key export fixes #21519

t8m · 2023-07-21T14:39:50Z

This corrects two serious problems - one in decoders where we try to export/import things with 0 selection which is not meaningful and the second in the actual keymgmt export functions which instead of failing on such export call, they produce bogus (empty) keys.

Fixes #21493

This is already correct in the rsa_kmgmt.c but other implementations are wrong.

t8m · 2023-07-21T14:40:25Z

I'll add testcases next.

When decoding 0 as the selection means to decode anything you get. However when exporting and then importing the key data 0 as selection is not meaningful. So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import function export/import everything that we have decoded. Fixes openssl#21493

junaruga · 2023-07-24T10:43:03Z

master branch

I tested this PR with my reproducer, the x25519 public key PEM file, and the OpenSSL on the current latest master branch 06a0d40 plus the commits in this PR. And I can see the encoded and decoded text is same with the original one!

master branch plus this PR's commits

The encoded and decode text is same with the original one. It's okay.

$ cat x25519_pub.pem
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA3p7bfXt9wbTTW2HC7OQ1Nz+DQ8hbeGdNrfx+FG+IK08=
-----END PUBLIC KEY-----

$ gcc \
  -I /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322-plus-pr-21519/include \
  -L /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322-plus-pr-21519/lib \
  -o reproducer_pr_21519 reproducer.c -lcrypto

$ OPENSSL_CONF=$(pwd)/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322-plus-pr-21519/ssl \ 
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322-plus-pr-21519/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322-plus-pr-21519/lib \
  ./reproducer_pr_21519 x25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0xbe9e40
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA3p7bfXt9wbTTW2HC7OQ1Nz+DQ8hbeGdNrfx+FG+IK08=
-----END PUBLIC KEY-----

master branch (without this PR's commits)

And the encoded and decoded text is different from the original one on the current master branch 06a0d40 without this PR's commits. This is the expected behavior. I checked it just in case.

$ gcc \
  -I /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322/include \
  -L /home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322/lib \
  -o reproducer reproducer.c -lcrypto

$ OPENSSL_CONF=$(pwd)/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.2.0.dev-fips-debug-06a0d40322/lib \
  ./reproducer x25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0x83adf0
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-----END PUBLIC KEY-----

junaruga · 2023-07-24T11:22:50Z

I tested this PR on the lastet openssl-3.1 and openssl-3.1 branches too. It looks good to me!

openssl-3.1 branch plus this PR's commits

$ gcc \
  -I /home/jaruga/.local/openssl-3.1.2.dev-fips-debug-cead7e5adf-plus-pr-21519/include \
  -L /home/jaruga/.local/openssl-3.1.2.dev-fips-debug-cead7e5adf-plus-pr-21519/lib \
  -o reproducer_pr_21519_312 reproducer.c -lcrypto

$ OPENSSL_CONF=$(pwd)/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.1.2.dev-fips-debug-cead7e5adf-plus-pr-21519/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.1.2.dev-fips-debug-cead7e5adf-plus-pr-21519/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.1.2.dev-fips-debug-cead7e5adf-plus-pr-21519/lib \
  ./reproducer x25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0x1c6fe40
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA3p7bfXt9wbTTW2HC7OQ1Nz+DQ8hbeGdNrfx+FG+IK08=
-----END PUBLIC KEY-----

openssl-3.0 branch plus this PR's commits

I tested both x25519 and ed25519 cases.

$ gcc \
  -I /home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/include \
  -L /home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/lib \
  -o reproducer_pr_21519_3010 reproducer.c -lcrypto

crypto: x25519

$ OPENSSL_CONF=$(pwd)/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/lib \
  ./reproducer_pr_21519_3010 x25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0x218c550
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA3p7bfXt9wbTTW2HC7OQ1Nz+DQ8hbeGdNrfx+FG+IK08=
-----END PUBLIC KEY-----

crypto: ed25519

$ cat ed25519_pub.pem
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----

$ OPENSSL_CONF=$(pwd)/openssl_fips.cnf \
  OPENSSL_CONF_INCLUDE=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/ssl \
  OPENSSL_MODULES=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/lib/ossl-modules \
  LD_LIBRARY_PATH=/home/jaruga/.local/openssl-3.0.10.dev-fips-debug-6bcb6d297e-plus-pr-21519/lib \
  ./reproducer_pr_21519_3010 ed25519_pub.pem
[DEBUG] Loaded providers:
  fips
  base
[DEBUG] FIPS mode enabled: 1
[DEBUG] data_size: 113
[DEBUG] OSSL_DECODER_from_bio PEM 2 failed.
[DEBUG] Got a pkey! 0xf7c550
[DEBUG] It's held by the provider fips
[DEBUG] ossl_membio2str buf->data:
-----BEGIN PUBLIC KEY-----
MCowBQYDK2VwAyEAPUAXw+hDiVqStwqnTRt+vJyYLM8uxJaMwM1V8Sr0Zgw=
-----END PUBLIC KEY-----

providers/implementations/keymgmt/dh_kmgmt.c

t8m · 2023-07-25T14:57:08Z

@paulidale please reconfirm
@mattcaswell fixup pushed

mattcaswell · 2023-07-25T14:57:10Z

@paulidale - please reconfirm

openssl-machine · 2023-08-01T00:00:23Z

This pull request is ready to merge

This is already correct in the rsa_kmgmt.c but other implementations are wrong. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519)

When decoding 0 as the selection means to decode anything you get. However when exporting and then importing the key data 0 as selection is not meaningful. So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import function export/import everything that we have decoded. Fixes #21493 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519)

This is already correct in the rsa_kmgmt.c but other implementations are wrong. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 1ae4678)

When decoding 0 as the selection means to decode anything you get. However when exporting and then importing the key data 0 as selection is not meaningful. So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import function export/import everything that we have decoded. Fixes #21493 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 2acb0d3)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 4c50610)

tmshort · 2023-08-04T14:17:57Z

Merged to master, openssl-3.1 and openssl-3.0. Thank you!

This is already correct in the rsa_kmgmt.c but other implementations are wrong. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 1ae4678) (cherry picked from commit 8865d7c)

When decoding 0 as the selection means to decode anything you get. However when exporting and then importing the key data 0 as selection is not meaningful. So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import function export/import everything that we have decoded. Fixes #21493 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 2acb0d3) (cherry picked from commit 137ba05)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from #21519) (cherry picked from commit 4c50610) (cherry picked from commit 42f32b4)

junaruga · 2023-08-04T15:02:07Z

Thank you!

junaruga · 2023-08-17T13:12:52Z

Merged to master, openssl-3.1 and openssl-3.0. Thank you!

I just put the note that this PR's commits were merged as 3 commits to master, openssl-3.1 and openssl-3.0 branches for someone who is interested in it.

master branch
openssl-3.1 branch
openssl-3.0 branch

levitte · 2023-08-17T14:05:17Z

... with 0 selection which is not meaningful

Actually, from man OSSL_DECODER_CTX_new_for_pkey:

The search of decoder implementations can also be limited with keytype and selection, which specifies the expected resulting keytype and contents. NULL and zero are valid and signify that the decoder implementations will find out the keytype and key contents on their own from the input they get.

So, the selection 0 means "get me whatever you can", and leaves it to the application to discover (through probing) what it actually got.

t8m · 2023-08-17T19:25:09Z

... with 0 selection which is not meaningful

Actually, from man OSSL_DECODER_CTX_new_for_pkey:

The search of decoder implementations can also be limited with keytype and selection, which specifies the expected resulting keytype and contents. NULL and zero are valid and signify that the decoder implementations will find out the keytype and key contents on their own from the input they get.

So, the selection 0 means "get me whatever you can", and leaves it to the application to discover (through probing) what it actually got.

Yes, it is meaningful for OSSL_DECODER_CTX_new_for_pkey() but not for (at least in the current implementation of providers) for keymgmt import and export functions.

This is already correct in the rsa_kmgmt.c but other implementations are wrong. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl#21519) (cherry picked from commit 1ae4678) (cherry picked from commit 8865d7c)

When decoding 0 as the selection means to decode anything you get. However when exporting and then importing the key data 0 as selection is not meaningful. So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import function export/import everything that we have decoded. Fixes openssl#21493 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl#21519) (cherry picked from commit 2acb0d3) (cherry picked from commit 137ba05)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> (Merged from openssl#21519) (cherry picked from commit 4c50610) (cherry picked from commit 42f32b4)

Avoid exporting bogus (empty) data if empty selection is used

2aba91c

This is already correct in the rsa_kmgmt.c but other implementations are wrong.

t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Jul 21, 2023

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jul 21, 2023

t8m added 2 commits July 21, 2023 17:40

endecode_test.c: Add tests for decoding with 0 selection

9c057ed

t8m force-pushed the decoder-key-export-fixes branch from 007da86 to 9c057ed Compare July 21, 2023 15:46

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: present The PR has suitable tests present labels Jul 21, 2023

This was referenced Jul 21, 2023

OpenSSL 3: x25519 a decode from and then encode to a pem file corrupts the key if fips+base provider is used #21493

Closed

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

fixup! When exporting/importing decoded keys do not use 0 as selection

aa7b64b

paulidale approved these changes Jul 24, 2023

View reviewed changes

paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Jul 24, 2023

t8m requested review from hlandau and mattcaswell July 24, 2023 08:58

mattcaswell reviewed Jul 25, 2023

View reviewed changes

providers/implementations/keymgmt/dh_kmgmt.c Outdated Show resolved Hide resolved

fixup! Avoid exporting bogus (empty) data if empty selection is used

7cb84c5

mattcaswell approved these changes Jul 25, 2023

View reviewed changes

paulidale approved these changes Jul 30, 2023

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jul 30, 2023

junaruga mentioned this pull request Jul 31, 2023

Support OpenSSL 3 FIPS ruby/openssl#603

Open

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 1, 2023

tmshort self-assigned this Aug 4, 2023

tmshort closed this Aug 4, 2023

This was referenced Aug 8, 2023

CI: Add OpenSSL master branch head non-FIPS and FIPS cases. ruby/openssl#658

Merged

OSSL_DECODER_CTX_set_selection doesn't apply the selection value properly #20657

Open

test/openssl/test_pkey.rb: Fix pending tests in FIPS case. ruby/openssl#664

Merged

junaruga mentioned this pull request Aug 24, 2023

ossl_pkey.c: Workaround: Decode with non-zero selections. ruby/openssl#669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoder key export fixes #21519

Decoder key export fixes #21519

t8m commented Jul 21, 2023 •

edited

t8m commented Jul 21, 2023

junaruga commented Jul 24, 2023 •

edited

junaruga commented Jul 24, 2023

t8m commented Jul 25, 2023

mattcaswell commented Jul 25, 2023

openssl-machine commented Aug 1, 2023

tmshort commented Aug 4, 2023

junaruga commented Aug 4, 2023

junaruga commented Aug 17, 2023 •

edited

levitte commented Aug 17, 2023

t8m commented Aug 17, 2023

Decoder key export fixes #21519

Decoder key export fixes #21519

Conversation

t8m commented Jul 21, 2023 • edited

t8m commented Jul 21, 2023

junaruga commented Jul 24, 2023 • edited

master branch

master branch plus this PR's commits

master branch (without this PR's commits)

junaruga commented Jul 24, 2023

openssl-3.1 branch plus this PR's commits

openssl-3.0 branch plus this PR's commits

crypto: x25519

crypto: ed25519

t8m commented Jul 25, 2023

mattcaswell commented Jul 25, 2023

openssl-machine commented Aug 1, 2023

tmshort commented Aug 4, 2023

junaruga commented Aug 4, 2023

junaruga commented Aug 17, 2023 • edited

levitte commented Aug 17, 2023

t8m commented Aug 17, 2023

t8m commented Jul 21, 2023 •

edited

junaruga commented Jul 24, 2023 •

edited

junaruga commented Aug 17, 2023 •

edited