Skip to content

Comments

Enforce a minimum DH modulus size of 512 bits#9437

Closed
bernd-edlinger wants to merge 1 commit intoopenssl:masterfrom
bernd-edlinger:raise_min_dh_size
Closed

Enforce a minimum DH modulus size of 512 bits#9437
bernd-edlinger wants to merge 1 commit intoopenssl:masterfrom
bernd-edlinger:raise_min_dh_size

Conversation

@bernd-edlinger
Copy link
Member

@bernd-edlinger bernd-edlinger commented Jul 22, 2019

Checklist
  • documentation is added or updated
  • tests are added or updated

@bernd-edlinger bernd-edlinger added the branch: master Applies to master branch label Jul 22, 2019
@bernd-edlinger
Copy link
Member Author

bernd-edlinger commented Jul 22, 2019

As this little exercise clearly shows: #9363 (comment)
is is possible to solve any discrete logarithm when the modulus is not in a range where
factorization is infeasible. Therefore the minumum size of a DH parameter set should be
at least as large as the minimum size of a RSA modulus.

No doubt 512 is still too small, and needs to be raised in the near future.

paulidale
paulidale approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a larger minimum be considered? 512 bits isn't a lot. Under $100 on EC2 several years ago.

@t8m
Copy link
Member

t8m commented Jul 23, 2019

Yeah, the question is what the minimum means - 1024 is probably still within the breakable limits for a three letter agency. And wouldn't it be better to just print a warning from the openssl application if generating anything below 2048 (or 1536 ?) bits that the parameters are breakable?

@kroeckx
Copy link
Member

kroeckx commented Jul 23, 2019

I think at the TLS level we already enforce 1024 at our default security level of 1.

@bernd-edlinger bernd-edlinger added the approval: done This pull request has the required number of approvals label Jul 24, 2019
@bernd-edlinger
Copy link
Member Author

bernd-edlinger commented Jul 24, 2019

Merged to master as 6de1fe9. Thanks!

levitte pushed a commit that referenced this pull request Jul 24, 2019
@bernd-edlinger
Enforce a minimum DH modulus size of 512 bits
6de1fe9 
[extended tests]

Reviewed-by: Paul Dale <[email protected]>
(Merged from #9437)
@bernd-edlinger
Copy link
Member Author

bernd-edlinger commented Jul 24, 2019

Hmm, a very interesting thing happened in the CI build 26769.16 and 26849.16
https://travis-ci.org/openssl/openssl/jobs/563059321

../test/recipes/30-test_evp_extra.t ................ 
1..1
    # RAND SEED 1563830498
    # Subtest: /home/travis/build/openssl/openssl/test/evp_extra_test
    1..13
        # Subtest: test_invalide_ec_char2_pub_range_decode
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 1 - test_invalide_ec_char2_pub_range_decode
        # Subtest: test_d2i_AutoPrivateKey
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 2 - test_d2i_AutoPrivateKey
    ok 3 - test_EVP_SM2_verify
        # Subtest: test_EVP_MD_fetch
        1..5
        ok 1 - iteration 4
        ok 2 - iteration 3
        ok 3 - iteration 2
        ok 4 - iteration 1
        ok 5 - iteration 5
    ok 4 - test_EVP_MD_fetch
/home/travis/build/openssl/openssl/util/shlib_wrap.sh /home/travis/build/openssl/openssl/test/evp_extra_test => 139
not ok 1 - running evp_extra_test

In fact I can reproduce that, and it dead-locks for me:

$ OPENSSL_TEST_RAND_ORDER=1563830498 make test TESTS=test_evp_extra V=1
../test/recipes/30-test_evp_extra.t .. 
1..1
    # RAND SEED 1563830498
    # Subtest: /home/ed/OPC/openssl/test/evp_extra_test
    1..13
        # Subtest: test_invalide_ec_char2_pub_range_decode
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 1 - test_invalide_ec_char2_pub_range_decode
        # Subtest: test_d2i_AutoPrivateKey
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 2 - test_d2i_AutoPrivateKey
    ok 3 - test_EVP_SM2_verify
        # Subtest: test_EVP_MD_fetch
        1..5
        ok 1 - iteration 4
        ok 2 - iteration 3
        ok 3 - iteration 2
        ok 4 - iteration 1
        ok 5 - iteration 5
    ok 4 - test_EVP_MD_fetch
^Cmake[1]: *** [_tests] Interrupt
make: *** [tests] Unterbrechung
$ OPENSSL_TEST_RAND_ORDER=1563972976 make test TESTS=test_evp_extra V=1
make depend && make _tests
make[1]: Entering directory `/home/ed/OPC/openssl'
make[1]: Leaving directory `/home/ed/OPC/openssl'
make[1]: Entering directory `/home/ed/OPC/openssl'
( cd test; \
	  mkdir -p test-runs; \
	  SRCTOP=../. \
	  BLDTOP=../. \
	  RESULT_D=test-runs \
	  PERL="/usr/bin/perl" \
	  EXE_EXT= \
	  OPENSSL_ENGINES=`cd .././engines 2>/dev/null && pwd` \
	  OPENSSL_DEBUG_MEMORY=on \
	    /usr/bin/perl .././test/run_tests.pl test_evp_extra )
../test/recipes/30-test_evp_extra.t .. 
1..1
    # RAND SEED 1563972976
    # Subtest: /home/ed/OPC/openssl/test/evp_extra_test
    1..13
    ok 1 - test_EVP_DigestVerifyInit
    ok 2 - test_EVP_SM2_verify
        # Subtest: test_d2i_AutoPrivateKey
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 3 - test_d2i_AutoPrivateKey
    ok 4 - test_HKDF
    ok 5 - test_EVP_PKCS82PKEY
        # Subtest: test_EVP_MD_fetch
        1..5
        ok 1 - iteration 2
        ok 2 - iteration 4
        ok 3 - iteration 1
        ok 4 - iteration 3
        ok 5 - iteration 5
    ok 6 - test_EVP_MD_fetch
        # Subtest: test_EVP_PKEY_check
        1..5
        ok 1 - iteration 2
        ok 2 - iteration 4
        ok 3 - iteration 1
        ok 4 - iteration 3
        ok 5 - iteration 5
    ok 7 - test_EVP_PKEY_check
        # Subtest: test_invalide_ec_char2_pub_range_decode
        1..3
        ok 1 - iteration 2
        ok 2 - iteration 1
        ok 3 - iteration 3
    ok 8 - test_invalide_ec_char2_pub_range_decode
        # Subtest: test_set_get_raw_keys
        1..7
        ok 1 - iteration 3
        ok 2 - iteration 6
        ok 3 - iteration 2
        ok 4 - iteration 5
        ok 5 - iteration 1
        ok 6 - iteration 4
^Cmake[1]: *** [_tests] Interrupt
make: *** [tests] Unterbrechung

but not every seed exhibits this problem.
So far I have only 1563972976 and 1563830498.

@bernd-edlinger
Copy link
Member Author

bernd-edlinger commented Jul 24, 2019

Call stack for 1563830498:

#0  pthread_rwlock_wrlock ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_rwlock_wrlock.S:85
#1  0x0000155554f85d8d in CRYPTO_THREAD_write_lock (lock=0x62bd70)
    at crypto/threads_pthread.c:73
#2  0x0000155554fb3c43 in ossl_property_write_lock (p=0x62c200)
    at crypto/property/property.c:78
#3  0x0000155554fb4928 in ossl_method_store_cache_set (store=0x62c200, 
    nid=4609, prop_query=0x1555550814f4 "", method=0x662cb0)
    at crypto/property/property.c:472
#4  0x0000155554f5eb51 in evp_generic_fetch (libctx=0x0, operation_id=1, 
    name=0x15555508cae5 "SM3", properties=0x1555550814f4 "", 
    new_method=0x155554f45263 <evp_md_from_dispatch>, 
    up_ref_method=0x155554f455c8 <evp_md_up_ref>, 
    free_method=0x155554f455e2 <evp_md_free>) at crypto/evp/evp_fetch.c:221
#5  0x0000155554f45642 in EVP_MD_fetch (ctx=0x0, 
    algorithm=0x15555508cae5 "SM3", properties=0x1555550814f4 "")
    at crypto/evp/digest.c:685
#6  0x0000155554f43f65 in EVP_DigestInit_ex (ctx=0x82cf60, 
    type=0x155555318360 <sm3_md>, impl=0x0) at crypto/evp/digest.c:177
#7  0x0000155554f43d9c in EVP_DigestInit (ctx=0x82cf60, 
    type=0x155555318360 <sm3_md>) at crypto/evp/digest.c:106
#8  0x0000155554ffc0bf in sm2_compute_z_digest (
    out=0x7fffffffc890 "\360\310\377\377\377\177", 
    digest=0x155555318360 <sm3_md>, id=0x63c430 "\001\002\003\004letter", 
    id_len=10, key=0x62bae0) at crypto/sm2/sm2_sign.c:64
#9  0x0000155554ffbee1 in pkey_sm2_digest_custom (ctx=0x82ce70, mctx=0x82ccb0)
    at crypto/sm2/sm2_pmeth.c:287
#10 0x0000155554f64397 in do_sigver_init (ctx=0x82ccb0, pctx=0x0, 
    type=0x155555318360 <sm3_md>, e=0x0, pkey=0x636770, ver=0)
    at crypto/evp/m_sigver.c:83
#11 0x0000155554f643e1 in EVP_DigestSignInit (ctx=0x82ccb0, pctx=0x0, 
    type=0x155555318360 <sm3_md>, e=0x0, pkey=0x636770)
    at crypto/evp/m_sigver.c:91
#12 0x00000000004052fd in test_EVP_SM2 () at test/evp_extra_test.c:745
#13 0x0000000000407bba in run_tests (
    test_prog_name=0x7fffffffe076 "/home/ed/OPC/openssl/test/evp_extra_test")
    at test/testutil/driver.c:353
#14 0x0000000000408159 in main (argc=1, argv=0x7fffffffdc28)
    at test/testutil/main.c:30

and 1563972976:

#0  pthread_rwlock_wrlock ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_rwlock_wrlock.S:85
#1  0x0000155554f85d8d in CRYPTO_THREAD_write_lock (lock=0x62c9b0)
    at crypto/threads_pthread.c:73
#2  0x0000155554fb3c43 in ossl_property_write_lock (p=0x62bad0)
    at crypto/property/property.c:78
#3  0x0000155554fb4928 in ossl_method_store_cache_set (store=0x62bad0, 
    nid=3841, prop_query=0x1555550814f4 "", method=0x641c90)
    at crypto/property/property.c:472
#4  0x0000155554f5eb51 in evp_generic_fetch (libctx=0x0, operation_id=1, 
    name=0x15555508c64e "SHAKE256", properties=0x1555550814f4 "", 
    new_method=0x155554f45263 <evp_md_from_dispatch>, 
    up_ref_method=0x155554f455c8 <evp_md_up_ref>, 
    free_method=0x155554f455e2 <evp_md_free>) at crypto/evp/evp_fetch.c:221
#5  0x0000155554f45642 in EVP_MD_fetch (ctx=0x0, 
    algorithm=0x15555508c64e "SHAKE256", properties=0x1555550814f4 "")
    at crypto/evp/digest.c:685
#6  0x0000155554f43f65 in EVP_DigestInit_ex (ctx=0x63c9e0, 
    type=0x155555308880 <shake256_md>, impl=0x0) at crypto/evp/digest.c:177
#7  0x0000155554ede755 in oneshot_hash (
    out=0x7fffffffc880 "\300\310\377\377\377\177", outlen=57, 
    in=0x63fb40 "012345678901234567890123456789012345678901234567890123456", 
    inlen=57) at crypto/ec/curve448/eddsa.c:30
#8  0x0000155554ede95f in c448_ed448_derive_public_key (pubkey=0x640ba0 "", 
    privkey=0x63fb40 "012345678901234567890123456789012345678901234567890123456") at crypto/ec/curve448/eddsa.c:93
#9  0x0000155554edf46d in ED448_public_from_private (
    out_public_key=0x640ba0 "", 
    private_key=0x63fb40 "012345678901234567890123456789012345678901234567890123456") at crypto/ec/curve448/eddsa.c:370
#10 0x0000155554f3376f in ecx_key_op (pkey=0x63bd50, id=1088, palg=0x0, 
    p=0x411728 "012345678901234567890123456789012345678901234567890123456", 
    plen=57, op=KEY_OP_PRIVATE) at crypto/ec/ecx_meth.c:113
#11 0x0000155554f34306 in ecx_set_priv_key (pkey=0x63bd50, 
    priv=0x411728 "012345678901234567890123456789012345678901234567890123456", 
    len=57) at crypto/ec/ecx_meth.c:358
#12 0x0000155554f66d84 in EVP_PKEY_new_raw_private_key (type=1088, e=0x0, 
    priv=0x411728 "012345678901234567890123456789012345678901234567890123456", 
    len=57) at crypto/evp/p_lib.c:241
#13 0x000000000040596a in test_set_get_raw_keys_int (tst=6, pub=0)
    at test/evp_extra_test.c:869
#14 0x0000000000405b72 in test_set_get_raw_keys (tst=6)
    at test/evp_extra_test.c:892
#15 0x0000000000407e2c in run_tests (
    test_prog_name=0x7fffffffe076 "/home/ed/OPC/openssl/test/evp_extra_test")
    at test/testutil/driver.c:386
#16 0x0000000000408159 in main (argc=1, argv=0x7fffffffdc28)
    at test/testutil/main.c:30

@mspncp mspncp mentioned this pull request Jul 24, 2019
Closed
mattcaswell added a commit to mattcaswell/openssl that referenced this pull request Mar 4, 2021
@mattcaswell
Fix-up dh_test
c9f54a0 
The dh_test was failing because we now enforce a lower bound on the
modulus size that may be used. A number of locations in dhtest.c were
assuming that a very small modulus is valid.

A 512 bit lower bound was introduced by PR openssl#9437 (commit 6de1fe9) and
subsequently amended by PR openssl#9796 (commit feeb7ec).

The CHANGES entry says this:

 * Enforce a minimum DH modulus size of 512 bits.
levitte pushed a commit to levitte/openssl that referenced this pull request Mar 4, 2021
@mattcaswell @levitte
Fix-up dh_test
7a30a8a 
The dh_test was failing because we now enforce a lower bound on the
modulus size that may be used. A number of locations in dhtest.c were
assuming that a very small modulus is valid.

A 512 bit lower bound was introduced by PR openssl#9437 (commit 6de1fe9) and
subsequently amended by PR openssl#9796 (commit feeb7ec).

The CHANGES entry says this:

 * Enforce a minimum DH modulus size of 512 bits.
@mattcaswell mattcaswell mentioned this pull request Mar 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulidale paulidale paulidale approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bernd-edlinger @t8m @kroeckx @paulidale