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.

Sign up for GitHub

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

Jump to bottom

openssl: fix Curl_ossl_seed()'s fallback to a custom seeding of PRNG #1620

Closed
wants to merge 3 commits into from
Closed

openssl: fix Curl_ossl_seed()'s fallback to a custom seeding of PRNG #1620

wants to merge 3 commits into from

Conversation

dmitrykos
Copy link
Contributor

Fix Curl_ossl_seed fallback to a custom seeding of PRNG that may happen for example on Windows platform.

The correction includes numerous fixes:

  1. fix fallback seed of PRNG with a time based hash;
  2. fix return value of Curl_ossl_seed() by returning CURLE_OK on success instead of nread bytes because function wants to return CURLcode and not bytes;
  3. fix Curl_ossl_seed() returning error if RAND_file_name() is not supported by platform, instead check rand_enough() and succeed with CURLE_OK if PRNG is happy with a generated randomness.

As a result of these changes CURL is now able to connect to https server when compiled with < OpenSSL 1.1.0+.

@dmitrykos
openssl: fix fallback seed of PRNG with a time based hash
0d1ad19 
openssl: fix return value of Curl_ossl_seed() by returning CURLE_OK on success instead of nread bytes because function wants to return CURLcode and not bytes
openssl: fix Curl_ossl_seed() returning error if RAND_file_name() is not supported by platform, instead check rand_enough() and succeed with CURLE_OK if PRNG is happy with a generated randomness
@dmitrykos dmitrykos changed the title Fix Curl_ossl_seed fallback to a custom seeding of PRNG openssl: fix Curl_ossl_seed fallback to a custom seeding of PRNG Jun 27, 2017
@dmitrykos dmitrykos changed the title openssl: fix Curl_ossl_seed fallback to a custom seeding of PRNG openssl: fix Curl_ossl_seed()'s fallback to a custom seeding of PRNG Jun 27, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-73.8%) to 0.0% when pulling 0d1ad19 on dmitrykos:fix_Curl_ossl_seed into 467da3a on curl:master.

@bagder
Copy link
Member

bagder commented Jun 27, 2017 

vtls/openssl.c: In function 'Curl_ossl_seed':
vtls/openssl.c:291:5: error: 'for' loop initial declarations are only allowed in C99 mode
vtls/openssl.c:291:5: note: use option -std=c99 or -std=gnu99 to compile your code

@dmitrykos
openssl: moved declarations outside the seed generating loop
e2d1fdc 
openssl: modified seed algorithm to change all bits of the timeval structure
@dmitrykos
Copy link
Contributor Author

Corrected, moved declaration outside the for(). Additionally changed seed generating algorithm in order to mix all available bits of timeval to avoid constant holes with 0s.

@coveralls
Copy link

Coverage Status

Coverage decreased (-73.8%) to 0.0% when pulling e2d1fdc on dmitrykos:fix_Curl_ossl_seed into 467da3a on curl:master.

@bagder
Copy link
Member

bagder commented Jun 28, 2017

I recommend trying make checksrc in the source code root to detect these that the CI now complains on:

./vtls/openssl.c:287:87: warning: Longer than 79 columns (LONGLINE)
   /* fallback to a custom seeding of the PRNG using a hash based on a current time. */
./vtls/openssl.c:291:8: warning: for with space (SPACEBEFOREPAREN)
     for (i = 0, i_max = len / sizeof(struct timeval); i < i_max; ++i) {
        ^
./vtls/openssl.c:296:84: warning: Longer than 79 columns (LONGLINE)
       tv.tv_sec ^= ((curlx_tvnow().tv_sec + curlx_tvnow().tv_usec) * (i + 3)) << 8;
./vtls/openssl.c:297:86: warning: Longer than 79 columns (LONGLINE)
       tv.tv_usec ^= ((curlx_tvnow().tv_sec + curlx_tvnow().tv_usec) * (i + 4)) << 16;
./vtls/openssl.c:316:90: warning: Longer than 79 columns (LONGLINE)
   return (rand_enough() ? CURLE_OK : CURLE_SSL_CONNECT_ERROR /* confusing error code */);

@bagder
Copy link
Member

bagder commented Jun 28, 2017

I suppose you add this extra time-based scrambling because rand_enough() otherwise doesn't think it is random enough after RAND_bytes() and RAND_add() are called?

@dmitrykos
openssl: shortened lines to max 79 columns
bdfd652 
openssl: remove redundant RAND_bytes/RAND_add_bytes (add more random bytes instead if engine is still unsatisfied with accumulated randomness)
@dmitrykos
Copy link
Contributor Author

Cleaned source according make checksrc warnings.

doesn't think it is random enough after RAND_bytes() and RAND_add() are called?

Agree, removed that redundant part. In case engine is not satisfied with randomness (rand_enough() fails) then algorithm will add new random bytes with RAND_add() and so on, looks ok to me now.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 73.791% when pulling bdfd652 on dmitrykos:fix_Curl_ossl_seed into 467da3a on curl:master.

@bagder bagder closed this in 1928770 Jun 30, 2017
@bagder
Copy link
Member

bagder commented Jun 30, 2017 •
edited

Thanks! You'll notice that I squashed, edited the commit message and edited the code ever so slightly.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dmitrykos @coveralls @bagder