Skip to content

crypto: fix memory leaks in cert validation#12089

Closed
Nibbler999 wants to merge 1 commit intonodejs:masterfrom
Nibbler999:fix-cert-leak
Closed

crypto: fix memory leaks in cert validation#12089
Nibbler999 wants to merge 1 commit intonodejs:masterfrom
Nibbler999:fix-cert-leak

Conversation

@Nibbler999
Copy link
Contributor

@Nibbler999 Nibbler999 commented Mar 28, 2017

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 mentioned this pull request Mar 28, 2017
Closed
@addaleax addaleax added the memory Issues and PRs related to the memory management or memory footprint. label Mar 28, 2017
@addaleax
Copy link
Member

addaleax commented Mar 28, 2017

Maybe Fixes: #12033?

CI: https://ci.nodejs.org/job/node-test-commit/8728/

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

indutny
indutny approved these changes Mar 28, 2017
View reviewed changes
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!

src/node_crypto.cc Outdated
Copy link
Member

@indutny indutny Mar 28, 2017

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?

cjihrig
cjihrig approved these changes Mar 28, 2017
View reviewed changes
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.

crypto: fix memory leak if certificate is revoked
785b766 
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

Nibbler999 commented Mar 28, 2017

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.

indutny
indutny approved these changes Mar 28, 2017
View reviewed changes
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

MylesBorins
MylesBorins approved these changes Mar 28, 2017
View reviewed changes
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

MylesBorins commented Mar 28, 2017

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 •
edited
Loading

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
@shigeki
crypto: fix memory leak if certificate is revoked
a6f9494 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Closed
@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
@MylesBorins
crypto: fix memory leak if certificate is revoked
a64aa44 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 29, 2017

This has now been release in v7.8.0.

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

@philippevk philippevk mentioned this pull request Mar 29, 2017
Closed
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
0490736 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
5f644d2 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
@MylesBorins
crypto: fix memory leak if certificate is revoked
9d7fba4 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins added a commit that referenced this pull request Mar 29, 2017
@MylesBorins
2017-04-04, Version 4.8.2 'Argon' (LTS)
006622b 
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
Merged
MylesBorins added a commit that referenced this pull request Mar 29, 2017
@MylesBorins
2017-04-04, Version 6.10.2 'Boron' (LTS)
be03658 
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
Merged
@coox coox mentioned this pull request Mar 31, 2017
Closed
MylesBorins added a commit that referenced this pull request Apr 4, 2017
@MylesBorins
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
ea2ceac 
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
@MylesBorins
2017-04-04, Version 6.10.2 'Boron' (LTS)
fbc9fde 
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
@MylesBorins
2017-04-04, Version 6.10.2 'Boron' (LTS)
28f8b17 
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
@MylesBorins
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
f7a3118 
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
Merged
2 tasks
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
b82c539 
    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 <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-04-04, Version 6.10.2 'Boron' (LTS)
7db63fb 
    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 <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-28, Version 7.8.0 (Current)
e0661dd 
    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 <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
eee59a8 
    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 <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-04-04, Version 6.10.2 'Boron' (LTS)
d8517db 
    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 <ilkka.myller@nodefield.com>
@mjmasn mjmasn mentioned this pull request Apr 25, 2017
Closed
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
@kevinsawicki
crypto: fix memory leak if certificate is revoked
9b1683e 
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 <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@cjihrig cjihrig cjihrig approved these changes

+4 more reviewers

@sam-github sam-github sam-github approved these changes

@indutny indutny indutny approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@shigeki shigeki shigeki approved these changes

Reviewers whose approvals may not affect merge requirements

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. memory Issues and PRs related to the memory management or memory footprint.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.