fix(security): bound smbman-ra password buffer and optimize empty string checks

[NathanNeurotic]

Summary

Following an audit of automated tool findings, this PR addresses legitimate security and performance items while discarding non-applicable upstream/hardware traps:

1. **\modules/network/smbman-ra/auth.c**:

   • Bounded password expansion in \NTLM_Password_Hash\ to 255 characters (matching the earlier fix in \smbinit/smbauth.c).
   • Prevents potential buffer overflow in \passwd_buf[512]\ when processing passwords longer than 255 characters.
   • Hoisted \strlen\ out of the loop condition to avoid repeated string evaluation during Unicode conversion.

2. **\src/gui.c\ & \src/config.c**:

   • In \src/gui.c\ (\guiManageCheats), replaced redundant \strlen(gCheats[i].name) == 0\ in the cheat render loop with \gCheats[i].name[0] == '\0'\ to match the entry condition and eliminate per-frame string length calls.
   • In \src/config.c\ (\configKeyValidate), replaced \strlen(key) == 0\ with \key[0] == '\0'\ for zero-overhead key validation.

Verification

• Ran test suite in .github/scripts/\ (7 test runners + full script matrices: all passed).

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of long passwords used for network sharing, helping keep authentication processing reliable.
  • Empty configuration keys and cheat names are still correctly identified during validation and management.
  • These updates maintain existing behavior for configuration and cheat-name validation while improving reliability in edge cases.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 5e08e681-bb50-4dd9-816d-224147e058e3

📥 Commits

Reviewing files that changed from the base of the PR and between f8b739b and 14cd9ff.

📒 Files selected for processing (3)

• modules/network/smbman-ra/auth.c
• src/config.c
• src/gui.c

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (7)

• GitHub Check: check-format
• GitHub Check: build-flavour (OFFICIALROLLING, ghcr.io/ps2homebrew/ps2homebrew:main, no (tracks ps2homebrew:main...
• GitHub Check: build-flavour (PS2DEVPINNED-RA, ps2dev/ps2dev@sha256:8fba50ecc2229acd7f8da63d34302f12939b7d4fa684...
• GitHub Check: build-flavour (OFFICIALPINNED, ghcr.io/ps2homebrew/ps2homebrew@sha256:a1b1f87f09a88f64efbe11356aa...
• GitHub Check: build-flavour (PS2DEVPINNED-DIAG, ps2dev/ps2dev@sha256:8fba50ecc2229acd7f8da63d34302f12939b7d4fa6...
• GitHub Check: build-flavour (PS2DEVROLLING, ps2dev/ps2dev:latest, no (tracks ps2dev/ps2dev:latest), 0)
• GitHub Check: build-flavour (PS2DEVPINNED, ps2dev/ps2dev@sha256:8fba50ecc2229acd7f8da63d34302f12939b7d4fa6848dd...

🔇 Additional comments (2)

src/config.c (1)

221-221: LGTM!

src/gui.c (1)

5075-5075: LGTM!

---

📝 Walkthrough

Walkthrough

The change caps NTLM password processing at 255 bytes. It also replaces empty-string length checks in configuration key validation and cheat-list iteration with first-character checks.

Changes

NTLM password processing

Layer / File(s)	Summary
Cap password processing modules/network/smbman-ra/auth.c	NTLM_Password_Hash measures the password once and caps processing at 255 bytes when expanding it into passwd_buf.

Configuration key validation

Layer / File(s)	Summary
Check for an empty key src/config.c	configKeyValidate checks the key’s first character to detect an empty key. It still rejects keys containing =.

Cheat-list iteration

Layer / File(s)	Summary
Check for an empty cheat name src/gui.c	The cheat-list loop checks the name’s first character and still skips empty names.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 14cd9

The reviewed changes show no actionable issue in the supplied evidence and are ready to merge, subject to normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 14cd9

The change limits writes during SMB password hashing without adding an authentication path. The normal application password is shorter than the new limit. External callers and malformed password inputs are not fully covered, so residual risk is low rather than negligible.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The observed credential source is local application configuration rather than an incoming SMB password supplied by the server. Its 32-byte storage cannot supply a valid over-255-byte password through the normal connection path.

Trust Boundaries and Controls

• observed — The local device-control dispatcher casts the supplied hash-request pointer and does not use arglen before forwarding its password to the hash function. That boundary behavior predates this change; the new write cap does not validate request length or termination.

Resilience and Maintainability Implications

• observed — The capped expansion stays within the declared 512-byte scratch buffer, and MD4 receives the capped expanded length.

Hardening Proposals

• proposed — Validate device-control request size and password termination at the input boundary, and use a bounded length check before hashing. This would address malformed-input handling that is not resolved by the new write cap.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main security fix and the empty-string check optimizations described in the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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