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

POLARSSL_ECP_DP_SECPxxxK1_ENABLED in config.h and ecp.c #65

Closed
aido opened this issue Feb 24, 2014 · 12 comments
Closed

POLARSSL_ECP_DP_SECPxxxK1_ENABLED in config.h and ecp.c #65

aido opened this issue Feb 24, 2014 · 12 comments

Comments

@aido
Copy link

aido commented Feb 24, 2014

The following:

!defined(POLARSSL_ECP_DP_SECP192K1_ENABLED) &&                  \
!defined(POLARSSL_ECP_DP_SECP224K1_ENABLED) &&                  \
!defined(POLARSSL_ECP_DP_SECP256K1_ENABLED) &&                  \

should probably be added to the following block in config.h:

#if defined(POLARSSL_ECP_C) && ( !defined(POLARSSL_BIGNUM_C) || (   \
    !defined(POLARSSL_ECP_DP_SECP192R1_ENABLED) &&                  \
    !defined(POLARSSL_ECP_DP_SECP224R1_ENABLED) &&                  \
    !defined(POLARSSL_ECP_DP_SECP256R1_ENABLED) &&                  \
    !defined(POLARSSL_ECP_DP_SECP384R1_ENABLED) &&                  \
    !defined(POLARSSL_ECP_DP_SECP521R1_ENABLED) &&                  \
    !defined(POLARSSL_ECP_DP_BP256R1_ENABLED)   &&                  \
    !defined(POLARSSL_ECP_DP_BP384R1_ENABLED)   &&                  \
    !defined(POLARSSL_ECP_DP_BP512R1_ENABLED) ) )
#error "POLARSSL_ECP_C defined, but not all prerequisites"
#endif

and similar in ecp.c

defined(POLARSSL_ECP_DP_SECP192K1_ENABLED) ||   \
defined(POLARSSL_ECP_DP_SECP224K1_ENABLED) ||   \
defined(POLARSSL_ECP_DP_SECP256K1_ENABLED) ||   \
@mpg
Copy link
Contributor

mpg commented Feb 24, 2014

Thanks for your report! As it happens, the part in config.h is already fixed in an internal branch, but we missed the part in ecp.c.

@aido
Copy link
Author

aido commented Feb 24, 2014

A good test is to comment out all curves from config.h except one of the POLARSSL_ECP_DP_SECPxxxK1_ENABLED curves and then see how make check reacts.

@mpg
Copy link
Contributor

mpg commented Mar 10, 2014

Fixed for the next release. Thanks for the report!

@aido
Copy link
Author

aido commented Apr 29, 2014

Hi Manuel,

I am not sure if this issue is fully fixed yet.

The following (i.e. comment all but one curve) in config.h:

/**
 * \def POLARSSL_ECP_XXXX_ENABLED
 *
 * Enables specific curves within the Elliptic Curve module.
 * By default all supported curves are enabled.
 *
 * Comment macros to disable the curve and functions for it
 */
//#define POLARSSL_ECP_DP_SECP192R1_ENABLED
//#define POLARSSL_ECP_DP_SECP224R1_ENABLED
//#define POLARSSL_ECP_DP_SECP256R1_ENABLED
//#define POLARSSL_ECP_DP_SECP384R1_ENABLED
//#define POLARSSL_ECP_DP_SECP521R1_ENABLED
//#define POLARSSL_ECP_DP_SECP192K1_ENABLED
//#define POLARSSL_ECP_DP_SECP224K1_ENABLED
#define POLARSSL_ECP_DP_SECP256K1_ENABLED
//#define POLARSSL_ECP_DP_BP256R1_ENABLED
//#define POLARSSL_ECP_DP_BP384R1_ENABLED
//#define POLARSSL_ECP_DP_BP512R1_ENABLED
//#define POLARSSL_ECP_DP_M221_ENABLED  // Not implemented yet!
//#define POLARSSL_ECP_DP_M255_ENABLED
//#define POLARSSL_ECP_DP_M383_ENABLED  // Not implemented yet!
//#define POLARSSL_ECP_DP_M511_ENABLED  // Not implemented yet!

seems to cause the following tests to fail:

 - test_suite_x509parse
   X509 Certificate verification #52 (CA keyUsage valid) ............. FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 Certificate verification #53 (CA keyUsage missing cRLSign) ... FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 Certificate verification #54 (CA keyUsage missing cRLSign, no  FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 Certificate verification #55 (CA keyUsage missing keyCertSign  FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 Certificate verification #55 (CA keyUsage plain wrong) ....... FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #1 (no extension, serverAuth) ........... FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #2 (single value, present) .............. FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #3 (single value, absent) ............... FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #4 (two values, first) .................. FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #5 (two values, second) ................. FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #6 (two values, other) .................. FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
X509 crt extendedKeyUsage #7 (any, random) ........................ FAILED
  x509_crt_parse_file( &crt, crt_file ) == 0
FAILED (233 / 245 tests (42 skipped))
**** Failed ***************

 - test_suite_x509write
   Certificate Request check Server5 ECDSA, key_usage ................ FAILED
  pk_parse_keyfile( &key, key_file, NULL ) == 0
FAILED (14 / 15 tests (1 skipped))
**** Failed ***************

These were the only changes made to config.h.

@mpg
Copy link
Contributor

mpg commented Apr 29, 2014

You're right. I forgot to update the dependencies in the X.509 test suites when I "upgraded" the test certificates from secp192r1 to secp256r1 and secp384r1 a while ago. This will be fixed shortly.

As usual, thanks for letting us know.

@mpg mpg reopened this Apr 29, 2014
@mpg
Copy link
Contributor

mpg commented Apr 29, 2014

Hum, actually this is not what I thought: I missed dependencies in test I added in 1.3.6. Anyway, fixing both issues (and checking for more) right now.

@mpg
Copy link
Contributor

mpg commented Apr 29, 2014

Ok, fixed for the next release.

When you're done tuning the config file for picocoin, feel free to send us a copy of the result: that way, we can add it to our sample configs which are tested automatically, in order to really make sure we don't break it again in the future :)

@mpg mpg closed this as completed Apr 29, 2014
@aido
Copy link
Author

aido commented May 1, 2014

Great.

Out of curiosity, when is the next release planned?

@pjbakker
Copy link
Contributor

pjbakker commented May 2, 2014

Has just been released!

@aido
Copy link
Author

aido commented May 2, 2014

Hi Manuel,

Here's an optimised config.h as requested.

It throws a few warnings but other than that seems to work well.

ssl/ssl_client2.c:118:13: warning: ‘my_debug’ defined but not used [-Wunused-function]
ssl/ssl_client2.c:130:12: warning: ‘my_recv’ defined but not used [-Wunused-function]
ssl/ssl_client2.c:147:12: warning: ‘my_send’ defined but not used [-Wunused-function]
ssl/ssl_client2.c:168:12: warning: ‘my_verify’ defined but not used [-Wunused-function]
ssl/ssl_server2.c:143:13: warning: ‘my_debug’ defined but not used [-Wunused-function]
ssl/ssl_server2.c:155:12: warning: ‘my_recv’ defined but not used [-Wunused-function]
ssl/ssl_server2.c:172:12: warning: ‘my_send’ defined but not used [-Wunused-function]

@mpg
Copy link
Contributor

mpg commented May 3, 2014

Thanks! We'll fix the warnings and add your config to our list of reference configs that are automatically tested.

Btw, you may not need to compile the programs at all. This is a detail since it only affects the compile time of picocoin, but anyway. (I think replacing make with make lib && ( cd tests && make ) should do the trick.)

@mpg
Copy link
Contributor

mpg commented Jun 24, 2014

Hi Aido,

I just had a closer look at your config.h file and thought it could be further reduced, see aido/picocoin#1

I'm now going to integrate the result in our config file collection and fix the things that need to be fixed :)

gilles-peskine-arm pushed a commit to gilles-peskine-arm/mbedtls that referenced this issue Mar 1, 2019
psa: Be compatible with deprecated constants
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants