FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: clear error stack in ECDH::Initialize by indutny · Pull Request #4689 · nodejs/node · GitHub

/ node Public

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

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-4686
ConversationCommits (1)ChecksFiles changed
Closed

crypto: clear error stack in ECDH::Initialize#4689
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/gh-4686

Conversation

indutny commented Jan 14, 2016

Copy link
Copy Markdown
Member

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

Fix: #4686

crypto: clear error stack in ECDH::Initialize
db53ee0 
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 mentioned this pull request Jan 14, 2016
Closed

indutny commented Jan 14, 2016

Copy link
Copy Markdown
Member Author

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

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 commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

LGTM if CI is green.

shigeki commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

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 added the crypto Issues and PRs related to the crypto subsystem. label Jan 14, 2016

indutny commented Jan 14, 2016

Copy link
Copy Markdown
Member Author

@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 commented Jan 14, 2016

Copy link
Copy Markdown
Member

thanks!

shigeki pushed a commit that referenced this pull request Jan 15, 2016
crypto: clear error stack in ECDH::Initialize
ebd9add 
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 commented Jan 15, 2016

Copy link
Copy Markdown
Contributor

@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 closed this Jan 15, 2016
indutny deleted the fix/gh-4686 branch January 15, 2016 05:10

indutny commented Jan 15, 2016

Copy link
Copy Markdown
Member Author

Thank you!

evanlucas mentioned this pull request Jan 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
crypto: clear error stack in ECDH::Initialize
8bf5b33 
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
crypto: clear error stack in ECDH::Initialize
acdf5e3 
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
crypto: clear error stack in ECDH::Initialize
5d2f581 
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
crypto: clear error stack in ECDH::Initialize
e5dc2ad 
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 mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
crypto: clear error stack in ECDH::Initialize
2f62644 
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
crypto: clear error stack in ECDH::Initialize
9d4a226 
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
crypto: clear error stack in ECDH::Initialize
5abb4ca 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL