fix: replace MD5 with SHA-256 for template signer fragment

[sandiyochristan]

Proposed Changes

The GetUserFragment() method in pkg/templates/signer/tmpl_signer.go uses MD5 to hash the ECDSA public key for generating signer identification fragments. MD5 is cryptographically broken and should not be used in any security-relevant context.

The Problem

// Before (line 66)
hashed := md5.Sum(t.handler.ecdsaPubKey.X.Bytes())
t.fragment = fmt.Sprintf("%x", hashed)

The signer fragment serves a security-critical purpose — it is used to verify authorization when re-signing code protocol templates (see Sign() method, lines 80-94). When a code template already has a signature, the fragment is compared to determine if the current signer is the original signer:

if fragment != arr[2] {
    return "", errkit.New("re-signing code templates are not allowed for security reasons.")
}

MD5 has been broken since 2004 (Wang et al.) and practical collision attacks exist. While a preimage attack on this specific usage is harder than a collision, using a broken hash in a security-critical code path is a well-known anti-pattern that weakens the overall trust chain.

The Fix

Replace md5.Sum() with sha256.Sum256():

// After
hashed := sha256.Sum256(t.handler.ecdsaPubKey.X.Bytes())
t.fragment = fmt.Sprintf("%x", hashed)

This is a minimal change because:

• crypto/sha256 was already imported in the same file (used for signing on line 118)
• sha256.Sum256() has the same call signature as md5.Sum() — takes []byte, returns a fixed-size byte array
• The crypto/md5 import is removed entirely as it has no other uses in this file

Fragment Length Change

The hex-encoded fragment changes from 32 chars (MD5, 128-bit) to 64 chars (SHA-256, 256-bit). This is a one-time change — any previously signed templates will need to be re-signed, which is the expected behavior when upgrading cryptographic primitives. Templates signed with the old MD5 fragment will still verify correctly since the fragment is only used for the re-signing authorization check, not for signature verification itself.

Files Changed

• pkg/templates/signer/tmpl_signer.go — replaced md5.Sum with sha256.Sum256, removed unused crypto/md5 import, updated comments

Security Impact

• CWE-328: Use of Weak Hash (MD5)
• Severity: High (used in code template signing authorization)
• Attack vector: An attacker could potentially forge a fragment matching a legitimate signer's identity, bypassing the re-signing restriction on code protocol templates

Proof

• Direct API substitution — same function signature, same usage pattern
• crypto/sha256 was already imported and used in the same file for sha256.Sum256 on line 118
• Removing crypto/md5 import eliminates the last usage of MD5 in this package

Checklist

• PR created against dev branch
• All checks passed (lint, unit/integration/regression tests)
• Minimal, focused change — single function modified
• No breaking changes to the public API
• Consistent with existing code — SHA-256 is already the hash used for actual signing in the same file

Summary by CodeRabbit

• Chores
  • Switched to a stronger internal hashing method for user fragments and improved digest/fragment handling to ensure broader compatibility and clearer internal comments.

[neo-by-projectdiscovery-dev]

Neo - PR Security Review

No security issues found

Highlights

• Replaces MD5 with SHA-256 for generating signer identification fragments used in template re-signing authorization
• Updates fragment hash from 32 to 64 hex characters (128-bit to 256-bit)
• Removes crypto/md5 import and updates code comments to reflect SHA-256 usage

Hardening Notes

• Consider adding path sanitization (filepath.Clean) in tmpl_signer.go:99 and :159 before calling os.ReadFile on template.GetFileImports() files to prevent potential path traversal, even though templates are trusted at signing time
• Consider adding fragment format validation in tmpl_signer.go:89 to ensure arr[2] matches expected hex format before comparison, though current string equality check is functionally secure

_{Comment @pdneo help for available commands. · Open in Neo}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d444c352-e1b3-4850-8ad0-79c471cd00ef

📥 Commits

Reviewing files that changed from the base of the PR and between 514fb4b and 8439446.

📒 Files selected for processing (1)

• pkg/templates/signer/tmpl_signer.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/templates/signer/tmpl_signer.go

---

Walkthrough

Replaced MD5 with SHA-256 for generating the user fragment from the ECDSA public key X coordinate. Adjusted digest/fragment handling in Verify to strip the fragment by taking the substring before the last colon instead of trimming by fragment content. Comments and imports updated accordingly.

Changes

Cohort / File(s)	Summary
Signer template changes pkg/templates/signer/tmpl_signer.go	Switched fragment hash from MD5 to SHA-256; removed crypto/md5 usage; updated GetUserFragment comment and implementation to use SHA-256; in Verify, changed fragment stripping to take substring before the last colon (compatible with any fragment hash) and updated related comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐇 I hopped from old MD5's glen,
to SHA‑256's bright den,
stronger bits beneath my paw,
nibbling bugs I never saw,
nibble, sign, and hop again.

🚥 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 title accurately describes the main change: replacing MD5 with SHA-256 for the template signer fragment in the GetUserFragment() function.
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
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

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]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@pkg/templates/signer/tmpl_signer.go`:
- Around line 60-66: GetUserFragment changed the fragment to a SHA256 hex, but
Verify() still removes ":" + t.GetUserFragment(), which breaks verification of
older MD5-suffixed signatures; update Verify() to split the incoming signature
on the last ':' (e.g., find last index of ':'), treat the part before the colon
as the hex-encoded digest and the part after as the fragment, then
hex.DecodeString only the digest part and proceed with verification — make this
change in the Verify method so digest parsing is independent of the fragment
value while leaving TemplateSigner.GetUserFragment unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dd5fc55b-670a-40a9-905b-7506f2a6cd23

📥 Commits

Reviewing files that changed from the base of the PR and between 979c867 and 514fb4b.

📒 Files selected for processing (1)

• pkg/templates/signer/tmpl_signer.go