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

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

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dmitrykos
Contributor

dmitrykos commented Jun 27, 2017

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+.

openssl: fix fallback seed of PRNG with a time based hash
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 from Fix Curl_ossl_seed fallback to a custom seeding of PRNG to openssl: fix Curl_ossl_seed fallback to a custom seeding of PRNG Jun 27, 2017

@dmitrykos dmitrykos changed the title from openssl: fix Curl_ossl_seed fallback to a custom seeding of PRNG to openssl: fix Curl_ossl_seed()'s fallback to a custom seeding of PRNG Jun 27, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2017

Coverage Status

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

@bagder

This comment has been minimized.

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
openssl: moved declarations outside the seed generating loop
openssl: modified seed algorithm to change all bits of the timeval structure
@dmitrykos

This comment has been minimized.

Contributor

dmitrykos commented Jun 28, 2017

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

This comment has been minimized.

coveralls commented Jun 28, 2017

Coverage Status

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

@bagder

This comment has been minimized.

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

This comment has been minimized.

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?

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

This comment has been minimized.

Contributor

dmitrykos commented Jun 28, 2017

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

This comment has been minimized.

coveralls commented Jun 28, 2017

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

This comment has been minimized.

Member

bagder commented Jun 30, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment