crypto: check for nullptr before dereferencing identity_buf

[gurgunday]

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]

Review requested:

• @nodejs/crypto

[codecov]

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]

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

[gurgunday]

Rebased

[gurgunday]

CI failures seem unrelated?

[gurgunday]

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

[nodejs-github-bot]

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

[tniessen]

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]

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;

[gurgunday]

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

[tniessen]

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]

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