EC only uses approved curves in FIPS mode. #9081
Conversation
Once there are buildable fips tests, some tests that are data driven from files will need to be modified to exclude non approved curves in fips mode. These changes were tested by temporarily adding #define FIPS_MODE 1 to all the modified source files.
romen
left a comment
There was a problem hiding this comment.
This looks good to me.
I have a style consideration but is totally personal and does not preclude my approval:
in ec_cruve.c, rather than having 2 alternative definitions of curve_list[] duplicating the entries for the fips supported curves, I would probably prefer a single curve_list[] declaration, where the blocks of definitions of curves not supported in FIPS mode are guarded by # ifndef FIPS_MODE, to avoid duplication of code.
@slontis do you think it would be too unreadable in the form I am proposing?
I tried doing that initially, but it looked pretty ugly due to all of the embedded #ifdefs. |
|
ping @openssl/omc |
| * ECC domain parameter validation. | ||
| * See SP800-56A R3 5.5.2 "Assurances of Domain-Parameter Validity" Part 1b. | ||
| */ | ||
| return EC_GROUP_check_named_curve(group, 1) >= 0 ? 1 : 0; |
There was a problem hiding this comment.
The ? 1 : 0 isn't required here. C defines conditional operators as returning 0 or 1.
There was a problem hiding this comment.
If you remove the ?1:0 then what is the conditional operator?
How do you know, for example, that it's not returning -1?
There was a problem hiding this comment.
The conditional operator would be >=.
The C standard defines them as return 0 or 1.
Once there are buildable fips tests, some tests that are data driven from files will need to be modified to exclude non approved curves in fips mode. These changes were tested by temporarily adding #define FIPS_MODE 1 to all the modified source files. Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #9081)
|
Merged. Thanks for the reviews.. |
| int r = 0, len; | ||
|
|
||
| if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp112r1)) | ||
| if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp224r1)) |
There was a problem hiding this comment.
@romen This subtle change has introduced a test failure if I configure with this line
./config --debug no-asm enable-ec_nistp_64_gcc_128
then a test fails...
../test/recipes/15-test_ec.t .......................
1..5
ok 1 - require '../../test/recipes/tconversion.pl';
# Subtest: ../../test/ectest
1..12
# ERROR: (int) 'EC_GROUP_cmp(group, group2, NULL) == 0' failed @ test/ectest.c:1826
# [1] compared to [0]
not ok 1 - parameter_test
# 00:47:EC:93:A4:7F:00:00:error:10071065:elliptic curve routines:EC_POINT_cmp:incompatible objects:crypto/ec/ec_lib.c:890:
# INFO: @ test/ectest.c:184
# Curve defined by Weierstrass equation
# y^2 = x^3 + a*x + b (mod p)
This is because the EC_GROUP_new_by_curve_name uses the EC_GFp_nistp224_method
and the EC_GROUP_new_from_ecparameters0 calls EC_GROUP_new_curve_GFp which uses EC_GFp_nist_method (or potentially EC_GFp_mont_method). SO even though points have the same X,Y they are not compatible because of the method comparison test.
Without the extra config option EC_GROUP_new_by_curve_name() calls EC_GROUP_new_curve_GFp() since there is no custom method, and the test passes.
There was a problem hiding this comment.
Yes, I noticed this yesterday as well: I am opening an issue to discuss it because I don't see an easy fix, given that it was always tested only on a single curve which did not have multiple ec_methods that test has actually been failing for way longer than the pr that made it surface.
Once there are buildable fips tests, some tests that are data driven
from files will need to be modified to exclude non approved curves in
fips mode.
These changes were tested by temporarily adding #define FIPS_MODE 1 to
all the modified source files.
This was a part of the original PR #8524 that was broken into sperate PR's.
Some of the more contentious test changes are still only in the old PR.
Checklist