Skip to content

crypto: check for nullptr before dereferencing identity_buf #57071

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

crypto: check for nullptr before dereferencing identity_buf #57071

wants to merge 3 commits into from

Conversation

gurgunday
Copy link
Member

The js-side already checks for this so the test already passes, so I don't think it's a vulnerability

Just in case, added the check for future

Fix #56665

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/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. needs-ci PRs that need a full CI run. labels Feb 15, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 15, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.26%. Comparing base (e2bc395) to head (3d9278e).
Report is 193 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_tls.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57071      +/-   ##
==========================================
- Coverage   90.37%   90.26%   -0.11%     
==========================================
  Files         629      630       +1     
  Lines      184365   184634     +269     
  Branches    36016    36131     +115     
==========================================
+ Hits       166612   166666      +54     
- Misses      10918    11025     +107     
- Partials     6835     6943     +108
Files with missing lines Coverage Δ
src/crypto/crypto_tls.cc 78.16% <0.00%> (ø)

... and 51 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gurgunday
Copy link
Member Author

Don't know how to test this since any js code would have its argument validated before moving to the C++ side

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@gurgunday
Copy link
Member Author

Rebased

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@gurgunday
Copy link
Member Author

CI failures seem unrelated?

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@gurgunday
Copy link
Member Author

Yeah seems to be random this CI failure, it was linux and now it's macos

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65746/

tniessen
tniessen reviewed Mar 18, 2025
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Can you please clarify how these tests are related to the actual patch? In particular, do any of the tests bypass these existing checks?

node/src/crypto/crypto_tls.cc

Lines 1540 to 1541 in a20acd4

if (ret.IsEmpty() || !ret->IsObject()) [[unlikely]]
return 0;

node/src/crypto/crypto_tls.cc

Lines 1547 to 1548 in a20acd4

if (psk_val.IsEmpty() || !psk_val->IsArrayBufferView()) [[unlikely]]
return 0;

node/src/crypto/crypto_tls.cc

Lines 1556 to 1557 in a20acd4

if (identity_val.IsEmpty() || !identity_val->IsString()) [[unlikely]]
return 0;

I must be missing or misunderstanding something about the issue itself, too. Could you please clarify why identity_buf could contain a nullptr at this point?

tniessen
tniessen requested changes Mar 18, 2025
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

The test appears to be completely unrelated to the patch. Even if the validation in JS was removed, both tests would fail the following check before ever reaching the modified location:

node/src/crypto/crypto_tls.cc

Lines 1540 to 1541 in a20acd4

if (ret.IsEmpty() || !ret->IsObject()) [[unlikely]]
return 0;

@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2025
@gurgunday
Copy link
Member Author

There is no other way you can test this input type from JS side, even if it gets checked before, tomorrow it might change

But anyway I don't think this is import right now

@gurgunday gurgunday closed this Mar 18, 2025
@tniessen
Copy link
Member

I did not mean for this to be closed, I'd just like to understand what bug this patch is supposed to catch. I'd be happy to remove my request for changes upon an explanation. I might simply be missing some detail here.

@gurgunday
Copy link
Member Author

Yeah so the passed value is being dereferenced but it can be nullptr in theory

Currently, it's type is checked on the JS side but this C++ code would be problematic if tomorrow the JS side is changed

So I don't know if this is truly important to land right now

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

@lpinca lpinca lpinca left review comments

@tniessen tniessen tniessen requested changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@mertcanaltin mertcanaltin mertcanaltin approved these changes

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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible Null Pointer Dereference in TLSWrap::PskClientCallback
7 participants
@gurgunday @nodejs-github-bot @tniessen @jasnell @addaleax @lpinca @mertcanaltin