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: fix memory leaks in cert validation #12089

Closed
wants to merge 1 commit into from
Closed

crypto: fix memory leaks in cert validation #12089

wants to merge 1 commit into from

Conversation

Nibbler999
Copy link
Contributor

The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 28, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 28, 2017

cc/ @nodejs/crypto

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

I will take a look at this right now.

@addaleax addaleax added the memory Issues and PRs related to the memory management or memory footprint. label Mar 28, 2017
@addaleax
Copy link
Member

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

@addaleax Probably, yes.

@seishun
Copy link
Contributor

seishun commented Mar 28, 2017

Have we ever considered adding RAII wrappers for such functions to prevent memory leaks in the future?

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

The fix is good. I will check the memory usage to see if there is no other memory growth.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nit, otherwise LGTM. Good catch!

@@ -2785,8 +2785,12 @@ inline bool CertIsStartComOrWoSign(X509_NAME* name) {
startcom_wosign_data = dn.data;
startcom_wosign_name = d2i_X509_NAME(nullptr, &startcom_wosign_data,
dn.len);
if (X509_NAME_cmp(name, startcom_wosign_name) == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just store the result to int cmp and free before the condition test?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @indutny's suggestion.

The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
@Nibbler999
Copy link
Contributor Author

Updated as suggested.

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

Here are the graph of rss profile up to 100,000 tls.connect to two servers (verify ok and revoked with SmartCom filter). It obviously shows that this fixes the memory leaks.
tls_leaks 001

@Nibbler999 Thanks for finding and fixing this.
A new CI is running on https://ci.nodejs.org/job/node-test-pull-request/7074/.
After it is green, I would like to land this and ask for a new release.
Do we need to wait for 48 hours?

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

CI results are all green.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber Stamp LGTM

CI is green.

@MylesBorins
Copy link
Contributor

I vote that we skip 48 hours and land immediately so we can do a v7.x release with this asap. Then we can backport to v4.x and v6.x and get a new release out for LTS ASAP as well

@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

Okay, @MylesBorins , @sam-github , @addaleax and I agree it and this have enough approvals. I'll make landing this. [Edited]

shigeki pushed a commit that referenced this pull request Mar 28, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@shigeki
Copy link
Contributor

shigeki commented Mar 28, 2017

Thanks for everyone making reviewing so quickly. Landed in a6f9494.
@Nibbler999 I really appreciate your fix.

@MylesBorins I would like to ask you to prepare new releases.

@shigeki shigeki closed this Mar 28, 2017
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

This has now been release in v7.8.0.

Will release v4.8.2-rc.1 and v6.10.2-rc.1 tomorrow

MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 29, 2017
This is a special LTS to fix a memory leak that was introduced
in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
@MylesBorins MylesBorins mentioned this pull request Mar 29, 2017
MylesBorins added a commit that referenced this pull request Mar 29, 2017
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123
@MylesBorins MylesBorins mentioned this pull request Mar 29, 2017
@coox coox mentioned this pull request Mar 31, 2017
MylesBorins added a commit that referenced this pull request Apr 4, 2017
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
MylesBorins added a commit that referenced this pull request Apr 4, 2017
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123
MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) nodejs#12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    nodejs#12123
MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    Notable changes:

    * buffer:
      - do not segfault on out-of-range index (Timothy Gu)
        nodejs/node#11927
    * crypto:
      - Fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      * upgrade npm to 4.2.0 (Kat Marchán)
        nodejs/node#11389
      * fix async await desugaring in V8 (Michaël Zasso)
        nodejs/node#12004
    * readline:
      - add option to stop duplicates in history (Danny Nemer)
        nodejs/node#2982
    * src:
      - add native URL class (James M Snell)
        nodejs/node#11801

    PR-URL: nodejs/node#12104

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <[email protected]>
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: nodejs/node#9469
Fixes: nodejs/node#12033
PR-URL: nodejs/node#12089
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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. memory Issues and PRs related to the memory management or memory footprint.
Projects
None yet
Development

Successfully merging this pull request may close these issues.