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

crypto: clear error stack in ECDH::Initialize #4689

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 14, 2016

Clean up OpenSSL error stack in ECDH::Initialize, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: #4686

Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: nodejs#4686
@indutny
Copy link
Member Author

indutny commented Jan 14, 2016

R: @nodejs/crypto
CI: https://ci.nodejs.org/job/node-test-pull-request/1232/

@indutny indutny added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Jan 14, 2016
@thefourtheye
Copy link
Contributor

LGTM if CI is green.

@shigeki
Copy link
Contributor

shigeki commented Jan 14, 2016

Does it mean the Oakley curve is broken in openssl?

$ openssl ecparam -list_curves
 ...
  Oakley-EC2N-3:
        IPSec/IKE/Oakley curve #3 over a 155 bit binary field.
        Not suitable for ECDSA.
        Questionable extension field!
  Oakley-EC2N-4:
        IPSec/IKE/Oakley curve #4 over a 185 bit binary field.
        Not suitable for ECDSA.
        Questionable extension field!

Instead of ignoring it, how about the idea to have ERR_peek_error() and throw an error in ECDH::New?

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jan 14, 2016
@indutny
Copy link
Member Author

indutny commented Jan 14, 2016

@shigeki I don't really want to introduce more ERR_peek_error() checks until we decide what to do with the ERR stack in general. Every check added means that it may throw error on dirty stack.

@deian
Copy link
Member

deian commented Jan 14, 2016

thanks!

shigeki pushed a commit that referenced this pull request Jan 15, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: #4686
PR-URL: #4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@shigeki
Copy link
Contributor

shigeki commented Jan 15, 2016

@indutny I found the error is intentional and ignoring it is a right choice, which comes from

https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/crypto/ec/ec_lib.c#L321-L325

We ignore the return value because some groups have an order 
with factors of two, which makes the Montgomery setup fail. 
|group->mont_data| will be NULL in this case.

CI was green except existing failures on WIn. LGTM and landed in ebd9add. Thanks.

@shigeki shigeki closed this Jan 15, 2016
@indutny indutny deleted the fix/gh-4686 branch January 15, 2016 05:10
@indutny
Copy link
Member Author

indutny commented Jan 15, 2016

Thank you!

evanlucas pushed a commit that referenced this pull request Jan 18, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: #4686
PR-URL: #4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: #4686
PR-URL: #4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: #4686
PR-URL: #4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: nodejs#4686
PR-URL: nodejs#4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: nodejs#4686
PR-URL: nodejs#4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: nodejs#4686
PR-URL: nodejs#4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Clean up OpenSSL error stack in `ECDH::Initialize`, some curves have
faulty implementations that are leaving dangling errors after
initializing the curve.

Fix: nodejs#4686
PR-URL: nodejs#4689
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants