Fix memory leak in EdDSA PrintableString handling

[olszomal]

Fix a small memory leak in OSSL::byteString2oid() when CKA_EC_PARAMS is decoded as ASN1_PRINTABLESTRING for EdDSA keys.

Also replace strcmp() on ASN.1 string data with length-aware memcmp() using ASN1_STRING_get0_data() and ASN1_STRING_length(), as ASN.1 strings are not guaranteed to be NUL-terminated.

For consistency, also free the temporary ASN1_OBJECT in the OID path.

Detected via Valgrind (ASN1_PRINTABLESTRING leak during C_SignInit with EdDSA keys).

Summary by CodeRabbit

• Bug Fixes
  • Enhanced parsing and validation of cryptographic curve names, including improved support for Edwards25519, Curve25519, Edwards448, and Curve448 elliptic curves.
  • Strengthened memory safety and stability in OpenSSL utilities through proper resource cleanup and comprehensive null-safety checks.

[coderabbitai]

📝 Walkthrough

Walkthrough

The OSSL::byteString2oid() function in OpenSSL utilities was refactored to defer return logic via a nid result variable, add ASN1_PRINTABLESTRING-encoded curve name parsing using length-aware string comparisons, implement null-safety checks, and ensure explicit cleanup of OpenSSL objects.

Changes

Cohort / File(s)	Summary
OpenSSL OID Conversion Enhancement src/lib/crypto/OSSLUtil.cpp	Modified byteString2oid() to introduce deferred-return pattern with nid tracking, added ASN1_PRINTABLESTRING parsing with explicit length validation for edwards25519, curve25519, edwards448, and curve448 curve names, replaced strcmp with null-safe memcmp comparisons, and added explicit cleanup of oid and curve_name OpenSSL objects.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 With whiskers twitching, this coder did see,
Curve names now parsed with care and with glee!
OSSLUtil now cleans what it borrowed with grace,
No memory leaks in this cryptographic space! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly addresses the primary change: fixing a memory leak in EdDSA PrintableString handling by improving string comparison and adding cleanup for ASN1 objects.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/crypto/OSSLUtil.cpp (1)

211-229: Consider using else if chain for minor efficiency.

Since the curve names are mutually exclusive, using else if would skip unnecessary comparisons after a match is found.

♻️ Suggested refactor

-		if (data_len == 12 && memcmp(data, "edwards25519", data_len) == 0)
+		if (data_len == 12 && memcmp(data, "edwards25519", 12) == 0)
 		{
 			nid = EVP_PKEY_ED25519;
 		}
-
-		if (data_len == 10 && memcmp(data, "curve25519", 10) == 0)
+		else if (data_len == 10 && memcmp(data, "curve25519", 10) == 0)
 		{
 			nid = EVP_PKEY_X25519;
 		}
-
-		if (data_len == 10 && memcmp(data, "edwards448", 10) == 0)
+		else if (data_len == 10 && memcmp(data, "edwards448", 10) == 0)
 		{
 			nid = EVP_PKEY_ED448;
 		}
-
-		if (data_len == 8 && memcmp(data, "curve448", 8) == 0)
+		else if (data_len == 8 && memcmp(data, "curve448", 8) == 0)
 		{
 			nid = EVP_PKEY_X448;
 		}

Note: Also changed line 211's memcmp third argument from data_len to 12 for consistency with the other comparisons.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/crypto/OSSLUtil.cpp` around lines 211 - 229, The sequence of
independent if statements comparing curve name bytes is inefficient and can
cause redundant checks; change the four separate ifs that compare data/data_len
and assign nid (currently checking "edwards25519", "curve25519", "edwards448",
"curve448") into an else-if chain so once a match is found subsequent
comparisons are skipped, and also make the first memcmp call use a fixed length
12 (instead of data_len) to match the other comparisons; update the conditions
referencing data, data_len and assign EVP_PKEY_ED25519, EVP_PKEY_X25519,
EVP_PKEY_ED448, EVP_PKEY_X448 accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/lib/crypto/OSSLUtil.cpp`:
- Around line 211-229: The sequence of independent if statements comparing curve
name bytes is inefficient and can cause redundant checks; change the four
separate ifs that compare data/data_len and assign nid (currently checking
"edwards25519", "curve25519", "edwards448", "curve448") into an else-if chain so
once a match is found subsequent comparisons are skipped, and also make the
first memcmp call use a fixed length 12 (instead of data_len) to match the other
comparisons; update the conditions referencing data, data_len and assign
EVP_PKEY_ED25519, EVP_PKEY_X25519, EVP_PKEY_ED448, EVP_PKEY_X448 accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 167f5af6-7c32-4e3c-952a-1aeb5443606d

📥 Commits

Reviewing files that changed from the base of the PR and between 5a3466f and f4235fa.

📒 Files selected for processing (1)

• src/lib/crypto/OSSLUtil.cpp