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

Fixes for using default ciphers #830

Merged
merged 6 commits into from Apr 8, 2017

Conversation

Projects
None yet
4 participants
@kaleb-himes
Copy link
Member

commented Apr 3, 2017

Thanks to Johan Kleuskens for noticing this typo, should be DHE_RSA_WITH, not DHE_WITH_RSA

BUILD_TLS_DHE_WITH_RSA_CAMELLIA_256_CBC_SHA (typo)
BUILD_TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA (fix)

@toddouska
Copy link
Contributor

left a comment

This fix is incorrect. Why does it already work without this fix is the clue?

@ejohnstown

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

I disagree. This typo is in InitSuites(), which affects the cipher suite list used in the hello messages. The automated test is using wolfSSL_CTX_set_cipher_list() which replaces the suite list with anything.

@toddouska

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

Then why does it work when you force the cipher suite on one side of the connection but not the other? Because the same cipher suite is valid a few cipher suites later in InitSuites(). This typo is redundant (that is the whole cipher suite is redundant here) and not necessary.

@dgarske

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

Hey Guys,

Update on my findings.

  1. The typo fix is correct.
  2. The reason for the failure not being detected is because when wolfSSL_CTX_set_cipher_list is called the "suites->setSuites" flag is set, which causes the "InitSuites" to exit early.
  3. As for Todd's comment about redundant, that is incorrect. There is only one section for "BUILD_TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA". There is another one for SHA256 "BUILD_TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA256" right below, which might be the confusion.
  4. This also turned up other failures that I'm correcting. For example the "DHE-RSA-AES128-SHA256" and "DHE-RSA-CHACHA20-POLY1305" tests with old TLS fail. The SetCipherList function looks up using name only and sets the suite. The InitSuites function uses additional information such as tls1_2 flags. This causes these tests to fail with the new code because InitSuites says they are TLS 1.2 only. I'm working with Jacob to identify which is valid or not and either correct test.conf or the InitSuites.

More Info:
Using our example server / client manually:

./examples/server/server -v 1 -l DHE-RSA-CAMELLIA256-SHA
./examples/client/client  -v 1 -l DHE-RSA-CAMELLIA256-SHA

The server finds the cipher suite in the cipher_names array based on name and is done at the WOLFSSL_CTX level. Then when the WOLFSSL object is created the InitSuites is called, but because suites->setSuites is set the section of code with the typo BUILD_TLS_DHE_WITH_RSA_CAMELLIA_256_CBC_SHA isn't hit.

The client example has the same issue. Since the cipher list has been set the the InitSuites doesn't get to the typo line.

The fix for this test is to have the server not set a cipher list and have the client try the cipher suite. This successfully causes a failure.

I should have all fixes pushed shortly.

@dgarske dgarske changed the title Typo in cipher suite pre-processor macro Fixes for using default ciphers Apr 5, 2017

@@ -10315,7 +10318,7 @@ static int BuildCertHashes(WOLFSSL* ssl, Hashes* hashes)
#endif
}
}
#if ! defined( NO_OLD_TLS )
#if !defined(NO_OLD_TLS)
else {
BuildMD5_CertVerify(ssl, hashes->md5);
BuildSHA_CertVerify(ssl, hashes->sha);

This comment has been minimized.

Copy link
@toddouska

toddouska Apr 5, 2017

Contributor

In the WOLFSSL_ALLOW_TLS_SHA1 case with TLS 1.2 and NO_OLD_TLS shouldn't the SHA1 hash be calculated here?

@dgarske dgarske assigned toddouska and unassigned dgarske Apr 6, 2017

@@ -3573,6 +3573,7 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
WOLFSSL_MSG("HS_Hashes Memory error");
return MEMORY_E;
}
XMEMSET(ssl->hsHashes, 0, sizeof(HS_Hashes));

This comment has been minimized.

Copy link
@toddouska

toddouska Apr 7, 2017

Contributor

This looks like the wrong place to memset. Shouldn't it be done right after allocation?

This comment has been minimized.

Copy link
@dgarske

dgarske Apr 7, 2017

Contributor

The allocation is done above at line 3570. This looks like the correct place to me...

This comment has been minimized.

Copy link
@toddouska

toddouska Apr 7, 2017

Contributor

You're right, I thought this was in InitSuites. Thanks.

kaleb-himes and others added some commits Apr 3, 2017

Added test for server to use the default cipher suite list using new …
…“-U” option. This allows the InitSuites logic to be used for determining cipher suites instead of always overriding using the “-l” option. Now both versions are used, so tests are done with wolfSSL_CTX_set_cipher_list and InitSuites. Removed a few cipher suite tests from test.conf that are not valid with old TLS. These were not picked up as failures before because wolfSSL_CTX_set_cipher_list matched on name only, allowing older versions to use the suite.
Fix InitSuites to allow old TLS for DHE_RSA with AES 128/256 for SHA2…
…56. Reverted changes to test.conf and test-dtls.conf.
Fix on server side to make sure SHA hash is setup even with NO_OLD_TL…
…S. Fix to initialize hsHashes to zero. Fix in PickHashSigAlgo to not default to SHA if NO_OLD_TLS is defined (unless WOLFSSL_ALLOW_TLS_SHA1 is set). Fix to allow pre TLS 1.2 for “AES128-SHA256” and “AES256-SHA256”.
Fix to calc BuildSHA_CertVerify if WOLFSSL_ALLOW_TLS_SHA1. Fix to add…
… check for DTLS to not allow stream ciphers. Removed the RC4 tests from the test-dtls.conf. Added support for using default suites on client side. Switched the arg to “-H”. Cleanup of the example server/client args list. Fixes for build with “--disable-sha”.

@dgarske dgarske force-pushed the kaleb-himes:suite-typo branch from 8b7aa49 to 4ff2903 Apr 7, 2017

@dgarske

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

Just pushed a rebase to resolve the merge conflicts in the client/server examples.

@@ -3573,6 +3573,7 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx)
WOLFSSL_MSG("HS_Hashes Memory error");
return MEMORY_E;
}
XMEMSET(ssl->hsHashes, 0, sizeof(HS_Hashes));

This comment has been minimized.

Copy link
@toddouska

toddouska Apr 7, 2017

Contributor

You're right, I thought this was in InitSuites. Thanks.

@toddouska toddouska merged commit e8971c3 into wolfSSL:master Apr 8, 2017

8 checks passed

"Jenkins Clang PR test" Passed clang PR test in Jenkins
Details
"Jenkins Disable Options Test:" Passed known configs test in Jenkins
Details
"Jenkins Enable Options Test:" Passed known configs test in Jenkins
Details
"Jenkins Fips Check status:" Passed fips check in Jenkins
Details
"Jenkins Known Configurations Test:" Passed known configs test in Jenkins
Details
"Jenkins Valgrind status:" Passed Valgrind test in Jenkins
Details
"Jenkins Visual Studio status:" Visual Studio Projects built without Warning(s) or Error(s)
Details
"Jenkins scan-build status:" Passed scan-build test in Jenkins
Details

@kaleb-himes kaleb-himes deleted the kaleb-himes:suite-typo branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.