Add support for alternative RSA implementations #1060

hanno-becker · 2017-08-23T15:49:57Z

Goal

The main purpose of this PR is to pave the way for alternative RSA implementations by having the library interact with the RSA module through the RSA interface functions only, as opposed to direct manipulation of structure fields.

@mpg @gilles-peskine-arm @yanesca Please review.

Main parts

Extend RSA interface to allow function-based setup and export of an RSA context.
Adapt library, tests and example programs to use the new functions.
Use the gained flexibility to remove CRT fields DP,DQ,QP,RP,RQ from the RSA context if RSA_NO_CRT is defined (see below for why P,Q are retained nonetheless). After the preparation, this requires changes in the RSA module implementation only. Deprecate direct usage of RSA context fields, paving the way for a structure cleanup at a later stage.

Changes in more detail

Interface changes

The RSA interface is extended by import functions that can be used to setup RSA contexts from RSA core parameters (N, P, Q, D, E). The parameters can be provided as mbed TLS MPI's (mbedtls_rsa_import) or as big endian byte arrays (mbedtls_rsa_import_raw), and import can happen at once or successively.
The interface is extended by export functions that can be used to extract RSA core parameters, again either as MPI's (mbedtls_rsa_export) or as big endian byte arrays (mbedtls_rsa_export_raw).
The interface is extended by a completion function mbedtls_rsa_complete that can be used to finalize the initialization of an RSA context after its core parameters have been imported (e.g., computing any internal helper variables from the core parameters).
The interface is extended by generic (meaning: not depending on the RSA context structure) helper functions for deducing missing but redundant RSA core parameters; specifically, this includes the derivation of the prime moduli from a pair of private and public exponent (mbedtls_rsa_deduce_moduli), and the derivation of the private exponent from the prime moduli and the public exponent (mbedtls_rsa_deduce_private) and the computation of CRT parameters (mbedtls_rsa_deduce_crt). The new completion function uses these helper functions to setup RSA contexts if they have only been partially specified.

These changes are meant to encourage the transition from constructing RSA contexts by hand to building them through interface functions, which is hopefully more stable against future changes and/or drop-in alternative RSA implementations.

The PR does not remove or change the signature or semantics of any existing RSA interface function and is therefore meant to be fully backwards compatible.

Library changes
The library code directly interacting with the RSA module needed adaption in the PK interface layer: pkwrite.c, pkparse.c and pk_wrap.c.

Test changes

The RSA selftest needed adaption to the new interface
The RSA test suites needed adaption to the new interface
The PKCS v15 / v21 test suites needed adaption to the new interface
The RSA test suite has been significantly expanded to test the new interface functions.
all.sh has been extended by a run with MBEDTLS_RSA_NO_CRT set.

Example program changes
Exemplifying the intended use of the new interface, numerous example programs have been modified accordingly: key_app, key_app_writer, gen_key, dh_server, rsa_sign, rsa_genkey, rsa_encrypt, rsa_decrypt.

Comments

Necessity of helper functions

The helper functions, in particular the one computing P,Q from N,D,E, were written with the intent to support implementations using N,D,E only - including our own if MBEDTLS_RSA_NO_CRT is set. After all, however, it seems that removing P,Q introduces problems that outweigh the space benefit it brings - see below. Keeping P,Q (or at least one of both) in the RSA context frees us from the need for the derivation function mbedtls_rsa_deduce_moduli, and we might therefore decide to drop it or to make it optional (disabled by default). This is up to discussion - for now I kept mbedtls_rsa_deduce_moduli.

Reducing the RSA context to N,D,E alone

If neither CRT nor verification are used, mbedtls_rsa_public and mbedtls_rsa_private can be implemented using N,D,E only, and so it is tempting to reduce the RSA context to N,D,E alone. However, the following cumbersome questions arise:

What does mbedtls_rsa_check_privkey check for if only N,D,E are present?
With only N,D,E given there are no simple sanity checks. Instead, to get confidence that they belong to some complete RSA keypair, one of the following computation heavy checks would need to be done:
- Temporarily deduce P,Q from N,D,E and check that N,P,Q,D,E all fit together.
- Do a pair of example encryption and decryption (using N,D,E only) and check that they are indeed inverse to each other.
When setting up an RSA context from a DER encoded RSA key, where to do the checking of the unused fields P,Q,DP,DQ,QP?
To make the problem clear, assume that we're setting up a bare-bones RSA context which uses N,D,E only, and which during setup completely ignores P,Q,DP,DQ,QP. When we're calling mbedtls_rsa_check_privkey afterwards, this will somehow (as discussed above) check if N,D,E can be extended to a valid N,P',Q',D,E, but it will not take the P,Q parameters from the original DER encoded key into consideration. The question is if and where the verification of these original parameters should happen. There are three options:
- The parameters have to be checked by the user before the RSA context is set up. The RSA implementation may completely ignore unused parameters, in particular not validating them against those that are used.
- Even if the RSA context does not use P,Q,DP,DQ,QP, it must accept and verify them at setup.
- No checking at all.
Comments on that:
- The first option is highly redundant if a full-fledged RSA key is used, because we're introducing both an unnecessary copy of the key and an unnecessary check (because for a full RSA context, mbedtls_rsa_check_privkey will contain all the checks).
- For the second, the interface must either enforce all parameters to be passed to the context at once at setup time, or the RSA context has to somehow save them internally until enough parameters are present for verification. In the former case, we're again unnecessarily introducing a full copy of the RSA key during setup when a full-fledged RSA context with all fields is used. In the latter, additional fields have to be added to the RSA context for the temporary storage, counteracting the initial intend of keeping the context small.
- The third option sounds bad at first, but on the other we're probably trusting our own private key in the first place anyway?

The approach followed in this PR uses a mixture of the first and second approach: The RSA context must accept and validate the core parameters N,P,Q,D,E, and the compatibility-check of the parameters DP,DQ,QP with the core parameters is done in the interface function mbedtls_rsa_check_crt after the setup is completed.

I think that the little space gained from removing P,Q from the RSA context is not worth the complexity it introduces, so I sticked with it even when MBEDTLS_RSA_NON_CRT is set.

This commit adds convenience functions to the RSA module for computing a complete RSA private key (with fields N, P, Q, D, E, DP, DQ, QP) from a subset of core parameters, e.g. (N, D, E).

This commit extends the RSA interface by import/export calls that can be used to setup an RSA context from a subset of the core RSA parameters (N,P,Q,D,E). The intended workflow is the following: 1. Call mbedtls_rsa_import one or multiple times to import the core parameters. 2. Call mbedtls_rsa_complete to deduce remaining core parameters as well as any implementation-defined internal helper variables. The RSA context is ready for use after this call. The import function comes in two variants mbedtls_rsa_import and mbedtls_rsa_import_raw, the former taking pointers to MPI's as input, the latter pointers buffers holding to big-endian encoded MPI's. The reason for this splitting is the following: When only providing an import function accepting const MPI's, a user trying to import raw binary data into an RSA context has to convert these to MPI's first which before passing them to the import function, introducing an unnecessary copy of the data in memory. The alternative would be to have another MPI-based import-function with move-semantics, but this would be in contrast to the rest of the library's interfaces. Similarly, there are functions mbedtls_rsa_export and mbedtls_rsa_export_raw for exporting the core RSA parameters, either as MPI's or in big-endian binary format. The main import/export functions deliberately do not include the additional helper values DP, DQ and QP present in ASN.1-encoded RSA private keys. To nonetheless be able to check whether given parameters DP, DQ and QP are in accordance with a given RSA private key, the interface is extended by a function mbedtls_rsa_check_opt (in line with mbedtls_rsa_check_privkey, mbedtls_rsa_check_pubkey and mbedtls_rsa_check_pub_priv). Exporting the optional parameters is taken care of by mbedtls_export_opt (currently MPI format only).

This commit adds tests for the new library function mbedtls_rsa_deduce_private for deducing the private RSA exponent D from the public exponent E and the factorization (P,Q) of the RSA modulus: - Two toy examples with small numbers that can be checked by hand, one working fine and another failing due to bad parameters. - Two real world examples, one fine and one with bad parameters.

This commit adds test for the new library function mbedtls_rsa_deduce_moduli for deducing the prime factors (P,Q) of an RSA modulus N from knowledge of a pair (D,E) of public and private exponent: - Two toy examples that can be checked by hand, one fine and with bad parameters. - Two real world examples, one fine and one with bad parameters.

This commit adds numerous tests for the new library functions mbedtls_rsa_import and mbedtls_rsa_import_raw in conjunction with mbedtls_rsa_complete for importing and completing core sets of core RSA parameters (N,P,Q,D,E) into an RSA context, with the importing accepting either MPI's or raw big endian buffers. Each test is determined by the following parameters: 1) Set of parameters provided We're testing full sets (N,P,Q,D,E), partial sets (N,-,-,D,E) and (N,P,Q,-,E) that are sufficient to generate missing parameters, and the partial and insufficient set (N, -, Q, -, E). 2) Simultaenous or successive importing The functions rsa_import and rsa_import_raw accept importing parameters at once or one after another. We test both. 3) Sanity of parameters

This commit adds tests for the new library function mbedtls_rsa_export. Each test case performs the following steps: - Parse and convert a set of hex-string decoded core RSA parameters into MPI's. - Use these to initialize an RSA context - Export core RSA parameters as MPI's again afterwards - Compare initial MPI's to exported ones. In the private key case, all core parameters are exported and sanity-checked, regardless of whether they were also used during setup. Each test split is performed twice, once with successive and once with simultaneous exporting.

This commit adds tests for the new library function mbedtls_rsa_export_raw. Each test case performs the following steps: - Parse and convert a set of hex-string decoded core RSA parameters into big endian byte arrays. - Use these to initialize an RSA context - Export core RSA parameters as byte arrays again afterwards - Compare byte strings. Each test split is performed twice, once with successive and once with simultaneous exporting.

This commit adds test for the new library function mbedtls_rsa_check_params for checking a set of RSA core parameters. There are some toy example tests with small numbers that can be verified by hand, as well as tests with real world numbers. Complete, partial and corrupted data are tested, as well the check for primality exactly if a PRNG is provided.

This commit replaces direct manipulation of structure fields in the RSA selftest by calls to the extended interface.

This commit replaces direct manipulation of RSA context structure fields by calls to the extended RSA interface in pk_wrap.c.

gilles-peskine-arm

Two more merge issues, in all.sh and config.h.

gilles-peskine-arm · 2018-01-04T18:01:01Z

tests/scripts/all.sh

+cleanup
+cp "$CONFIG_H" "$CONFIG_BAK"
+scripts/config.pl set MBEDTLS_RSA_NO_CRT
+CC=gcc CFLAGS='-Werror -Wall -Werror -O0' make


Because make is now a function in keep-going mode, make must come first on the command line:

make CC=gcc CFLAGS='-Werror -Wall -Werror -O0'

Fixed in d8b3851.

gilles-peskine-arm · 2018-01-04T18:04:53Z

include/mbedtls/config.h

@@ -270,6 +270,7 @@
 //#define MBEDTLS_CMAC_ALT
 //#define MBEDTLS_DES_ALT
 //#define MBEDTLS_RSA_ALT


This list was almost in alphabetical order, please move RSA_ALT and XTEA_ALT to its rightful place and re-run generate_features.pl.

Fixed in d8b3851.

- Adapt the change in all.sh to the new keep-going mode - Restore alphabetical order of configuration flags for alternative implementations in config.h and rebuild library/version_features.c

gilles-peskine-arm · 2018-01-04T20:51:02Z

Running all.sh after the second merge (commit 88683b2) finds one error. The test suite x509parse fails in two configurations: scripts/config.pl full and scripts/config.pl baremetal. I looked at the error in the full config, I presume that the failure in baremetal is the same. Steps to reproduce:

scripts/config.pl full
make CFLAGS=-g
cd tests
./test_suite_x509parse -v

The failure:

X509 Certificate ASN1 (TBSCertificate, pubkey, check failed) ...... FAILED
  mbedtls_x509_crt_parse( &crt, buf, data_len ) == ( result )
  at line 412, suites/test_suite_x509parse.function

The test case parameters:

X509 Certificate ASN1 (TBSCertificate, pubkey, check failed)
depends_on:MBEDTLS_RSA_C:MBEDTLS_MD2_C
x509parse_crt:"30743072a0030201028204deadbeef300d06092a864886f70d0101020500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374301c300d06092A864886F70D0101010500030b0030080202ffff0202ffff":"":MBEDTLS_ERR_PK_INVALID_PUBKEY

The function mbedtls_x509_crt_parse returns MBEDTLS_ERR_X509_INVALID_ALG | MBEDTLS_ERR_ASN1_OUT_OF_DATA instead. I don't know whether this is a bug or an overly picky test.

hanno-becker · 2018-01-05T07:47:52Z

@gilles-peskine-arm This is a change of behavior stemming from the relatively recent change of mbedtls_rsa_complete towards not doing (partly expensive) sanity checks. In particular, check_pubkey was dropped from rsa_complete (despite not being expensive), which is thereby also not called in pk_get_rsapubkey which only calls rsa_complete after reading N and E.

If desired, solutions to restore the original behavior are to either add check_pubpkey at the end of rsa_complete or to add an explicit call to pk_get_rsapubkey.

Looking at the code I noticed that check_pubkey checks for the maximum size of RSA moduli, which in some parts of the library needs to be obeyed because of the use of static buffers of size MPI_MAX_SIZE. As far as I see, there's no danger of overflow because the relevant code paths also contain bounds checks, but still the size check appears necessary in guaranteeing that RSA operations will not fail (regardless if they're sensible), which rsa_complete is supposed to ensure. I'd therefore think that at least the size check should be part of rsa_complete and not of check_pubkey. I'll think this over and update the PR accordingly.

The function `pk_get_rsapubkey` originally performed some basic sanity checks (e.g. on the size of public exponent) on the parsed RSA public key by a call to `mbedtls_rsa_check_pubkey`. This check was dropped because it is not possible to thoroughly check full parameter sanity (i.e. that (-)^E is a bijection on Z/NZ). Still, for the sake of not silently changing existing behavior, this commit puts back the call to `mbedtls_rsa_check_pubkey`.

The function `mbedtls_rsa_complete` is supposed to guarantee that RSA operations will complete without failure. In contrast, it does not ensure consistency of parameters, which is the task of the checking functions `rsa_check_pubkey` and `rsa_check_privkey`. Previously, the maximum allowed size of the RSA modulus was checked in `mbedtls_rsa_check_pubkey`. However, exceeding this size would lead to failure of some RSA operations, hence this check belongs to `mbedtls_rsa_complete` rather than `mbedtls_rsa_check_pubkey`. This commit moves it accordingly.

hanno-becker · 2018-01-05T08:27:07Z

@gilles-peskine-arm Pushed commits to restore the old behavior and move the size check.

gilles-peskine-arm

Please make minor fixes to all.sh (see my comments) and run it (locally or on CI).

gilles-peskine-arm · 2018-01-05T10:18:54Z

tests/scripts/all.sh

+scripts/config.pl set MBEDTLS_RSA_NO_CRT
+make CC=gcc CFLAGS='-Werror -Wall -Werror -O0'
+
+msg "test: MBEDTLS_RSA_NO_CRT - main suites (inc. selftests) (ASan build)"


That's not an ASan build.

gilles-peskine-arm · 2018-01-05T10:19:33Z

tests/scripts/all.sh

+cleanup
+cp "$CONFIG_H" "$CONFIG_BAK"
+scripts/config.pl set MBEDTLS_RSA_NO_CRT
+make CC=gcc CFLAGS='-Werror -Wall -Werror -O0'


Why -O0? And -Werror is repeated, shouldn't that be -Werror -Wall -Wextra or -Werror -Wall?

Thanks for spotting, that's a copy-paste error; the ! MBEDTLS_SSL_CLI_C test above does it this way, too. I change it to -Werror -Wall -Wextra -O0. Regarding the use of optimization, all.sh seems to be inconsistent - I have no strong feelings in one direction or the other.

Correct gcc flags in !MBEDTLS_SSL_CLI_C test (preexisting) and build and test for RSA_NO_CRT in ASan mode.

hanno-becker · 2018-01-05T15:25:09Z

@gilles-peskine-arm Ran all.sh successfully on d485c31.

gilles-peskine-arm · 2018-01-05T15:49:33Z

all.sh passed, CI passed except for compat.sh slow start (known issue). Ok to merge pending a second review of the latest commits (@mpg, can you review the latest commits?).

mpg

I reviewed the latest commits and am happy with them.

simonbutcher · 2018-01-08T19:12:37Z

Reviewed and approved.

Hanno Becker added 30 commits August 23, 2017 14:06

Declare RSA helper functions

a3ebec2

This commit adds convenience functions to the RSA module for computing a complete RSA private key (with fields N, P, Q, D, E, DP, DQ, QP) from a subset of core parameters, e.g. (N, D, E).

Implement RSA helper functions

e2e8b8d

Implement new RSA interface functions

617c1ae

Minor formatting changes

8fd5548

Adapt RSA selftest to new RSA interface

3a70116

This commit replaces direct manipulation of structure fields in the RSA selftest by calls to the extended interface.

Adapt pk_wrap.c to new RSA interface

6a1e7e5

This commit replaces direct manipulation of RSA context structure fields by calls to the extended RSA interface in pk_wrap.c.

Adapt pkparse.c to new RSA interface

d58c5b2

Adapt pkwrite.c to new RSA interface

15f81fa

Adapt PK test suite to use new interface

d71dc15

Adapt PKCS v15 test suite to new RSA interface

6d43f9e

Adapt PKCS v21 test suite to new RSA interface

6326a6d

Adapt RSA test suites to new RSA interface

ceb7a9d

Adapt dh_server example program to new RSA interface

c95fad3

Adapt gen_key example program to new RSA interface

83aad1f

Adapt key_app example program to new RSA interface

54ebf99

Adapt key_app_writer example program to new RSA interface

40371ec

Adapt rsa_decrypt example program to new RSA interface

ccef18c

Adapt rsa_sign example program to new RSA interface

d6ba5e3

Adapt rsa_encrypt example program to new RSA interface

0c26393

Adapt rsa_genkey example program to use new RSA interface

f073de0

Remove CRT fields from RSA context if RSA_NO_CRT is defined

1a59e79

Adapt RSA test suite to deal with RSA_NON_CRT option

131134f

Adapt rsa_complete to deal with RSA_NO_CRT option

23344b5

gilles-peskine-arm requested changes Jan 4, 2018

View reviewed changes

hanno-becker force-pushed the iotssl-1619 branch from d8b3851 to 11f2805 Compare January 4, 2018 18:47

Correct all.sh and config.h after merge commit

88683b2

- Adapt the change in all.sh to the new keep-going mode - Restore alphabetical order of configuration flags for alternative implementations in config.h and rebuild library/version_features.c

hanno-becker force-pushed the iotssl-1619 branch from 11f2805 to 88683b2 Compare January 4, 2018 18:48

gilles-peskine-arm approved these changes Jan 4, 2018

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 4, 2018

Hanno Becker added 3 commits January 5, 2018 08:07

Correct typo in bignum.h

efeef6c

hanno-becker added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jan 5, 2018

gilles-peskine-arm requested changes Jan 5, 2018

View reviewed changes

Make small corrections to all.sh

d485c31

Correct gcc flags in !MBEDTLS_SSL_CLI_C test (preexisting) and build and test for RSA_NO_CRT in ASan mode.

gilles-peskine-arm approved these changes Jan 5, 2018

View reviewed changes

mpg approved these changes Jan 8, 2018

View reviewed changes

hanno-becker added needs-design-approval and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 8, 2018

simonbutcher approved these changes Jan 8, 2018

View reviewed changes

simonbutcher added approved Design and code approved - may be waiting for CI or backports and removed needs-design-approval labels Jan 8, 2018

Patater merged commit d485c31 into Mbed-TLS:development Jan 9, 2018

Patater mentioned this pull request Jan 22, 2018

CC RSA ALT #1229

Closed

4 tasks

simonbutcher mentioned this pull request Jun 5, 2018

Consistent Support for MBEDTLS_RSA_NO_CRT #416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for alternative RSA implementations #1060

Add support for alternative RSA implementations #1060

hanno-becker commented Aug 23, 2017 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Jan 4, 2018

hanno-becker Jan 4, 2018

gilles-peskine-arm Jan 4, 2018

hanno-becker Jan 4, 2018

gilles-peskine-arm commented Jan 4, 2018

hanno-becker commented Jan 5, 2018

hanno-becker commented Jan 5, 2018

gilles-peskine-arm left a comment

gilles-peskine-arm Jan 5, 2018

gilles-peskine-arm Jan 5, 2018

hanno-becker Jan 5, 2018

hanno-becker commented Jan 5, 2018

gilles-peskine-arm commented Jan 5, 2018

mpg left a comment

simonbutcher commented Jan 8, 2018 •

edited

Loading

Add support for alternative RSA implementations #1060

Add support for alternative RSA implementations #1060

Conversation

hanno-becker commented Aug 23, 2017 • edited Loading

Goal

Main parts

Changes in more detail

Comments

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Jan 4, 2018

Choose a reason for hiding this comment

hanno-becker Jan 4, 2018

Choose a reason for hiding this comment

gilles-peskine-arm Jan 4, 2018

Choose a reason for hiding this comment

hanno-becker Jan 4, 2018

Choose a reason for hiding this comment

gilles-peskine-arm commented Jan 4, 2018

hanno-becker commented Jan 5, 2018

hanno-becker commented Jan 5, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Jan 5, 2018

Choose a reason for hiding this comment

gilles-peskine-arm Jan 5, 2018

Choose a reason for hiding this comment

hanno-becker Jan 5, 2018

Choose a reason for hiding this comment

hanno-becker commented Jan 5, 2018

gilles-peskine-arm commented Jan 5, 2018

mpg left a comment

Choose a reason for hiding this comment

simonbutcher commented Jan 8, 2018 • edited Loading

hanno-becker commented Aug 23, 2017 •

edited

Loading

simonbutcher commented Jan 8, 2018 •

edited

Loading