Skip to content

fix: remove fabricated SHA-256 hashes from SpeedTestService (SEC-001) - #349

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/speedtest-fake-hashes
May 14, 2026
Merged

fix: remove fabricated SHA-256 hashes from SpeedTestService (SEC-001)#349
laurentiu021 merged 1 commit into
mainfrom
fix/speedtest-fake-hashes

Conversation

@laurentiu021

@laurentiu021 laurentiu021 commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

Remove placeholder SHA-256 hash constants that never matched real Ookla CLI downloads, causing perpetual warning logs and alert fatigue.

Problem

PinnedHashWin64 and PinnedHashWin32 were fabricated values. Every download triggered a hash mismatch warning, making real attacks indistinguishable from normal operation.

Fix

Removed fake hash comparison. Security now relies on:

  1. Authenticode signature verification (checks Ookla subject in cert)
  2. Zip structural integrity (must contain speedtest.exe)
  3. SHA-256 logged for audit (not compared against fake values)

Testing

  • Build: 0 errors
  • No behavioral change for users (speed test still works identically)
  • Eliminates false warning logs on every download

Closes #308

Summary by CodeRabbit

  • Bug Fixes
    • Improved download verification security by implementing Authenticode signature verification for extracted binaries and zip structural integrity checks, replacing the previous static hash verification approach.

Review Change Stack

Remove placeholder PinnedHashWin64/PinnedHashWin32 constants that never
matched real Ookla CLI downloads, causing perpetual warning logs and
alert fatigue. Real attacks were indistinguishable from normal operation.

Security now relies on:
1. Authenticode signature verification (checks Ookla subject in cert)
2. Zip structural integrity (must contain speedtest.exe)
3. SHA-256 logged for audit (but not compared against fake values)

Closes #308
@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR addresses security issue SEC-001 by removing fabricated placeholder SHA-256 hashes from SpeedTestService and shifting verification responsibility to Authenticode signature verification combined with zip structural validation. The implementation change removes hash enforcement logic while retaining audit logging, and the changelog documents this security posture update.

Changes

SpeedTestService verification refactoring

Layer / File(s) Summary
Remove hash pinning and shift to Authenticode verification
SysManager/SysManager/Services/SpeedTestService.cs, CHANGELOG.md
EnsureOoklaAsync no longer enforces pinned SHA-256 hashes on ookla.zip downloads. The computed hash and file size are logged for audit purposes. Zip structural validation (ensuring speedtest.exe exists) and Authenticode signature verification of the extracted binary remain as primary security controls. The changelog documents this fix under version 0.48.2.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • laurentiu021/SystemManager#294: Introduced SHA-256 hash pinning for ookla.zip downloads in SpeedTestService.EnsureOoklaAsync; this PR removes that hash verification in favor of Authenticode-based validation.

Poem

A rabbit hops through the security code,
Tossing out fake hashes down the road. 🐰
Authenticode now guards the way,
With zip checks strong—a safer day! ✓

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. 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 pull request title clearly and concisely summarizes the main change: removing fabricated SHA-256 hashes from SpeedTestService, directly addressing the primary objective of the changeset.
Linked Issues check ✅ Passed The pull request successfully addresses all coding requirements from issue #308: removes the fabricated hash constants, eliminates hash-mismatch checks, preserves Authenticode signature verification and zip integrity validation, and computes SHA-256 for audit logging.
Out of Scope Changes check ✅ Passed All changes are in-scope: SpeedTestService.cs hash verification logic removal, changelog documentation update, and removal of fabricated hash constants are directly aligned with issue #308 remediation objectives.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/speedtest-fake-hashes

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

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@laurentiu021
laurentiu021 merged commit 9efbe70 into main May 14, 2026
3 of 5 checks passed
@laurentiu021
laurentiu021 deleted the fix/speedtest-fake-hashes branch May 14, 2026 07:22
laurentiu021 added a commit that referenced this pull request May 22, 2026
…#349)

Remove placeholder PinnedHashWin64/PinnedHashWin32 constants that never
matched real Ookla CLI downloads, causing perpetual warning logs and
alert fatigue. Real attacks were indistinguishable from normal operation.

Security now relies on:
1. Authenticode signature verification (checks Ookla subject in cert)
2. Zip structural integrity (must contain speedtest.exe)
3. SHA-256 logged for audit (but not compared against fake values)

Closes #308

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: SpeedTestService placeholder SHA-256 hashes undermine security

2 participants