[fix] Mitigate security hotspots by Ghass-M · Pull Request #172 · POW-Software/ByteSync

Ghass-M · 2025-08-14T15:34:18Z

PR Title

Harden regex usage, process invocation, and randomness; reduce potential hotspots and improve safety

Overview

This PR mitigates potential regex-related CPU hotspots (ReDoS), improves robustness against invalid patterns, hardens process invocations by using trusted binary paths, and makes random number generation thread-safe.

Key Changes

1. Regex timeouts and safety

Add explicit Regex timeouts across the codebase to prevent long-running matches.
Replace direct Regex.IsMatch / Regex.Replace calls with pre-constructed Regex instances where appropriate.
Catch ArgumentException (invalid patterns) and treat as non-match instead of throwing in user-facing filter paths.

2. Process invocation hardening

Use absolute, trusted paths for system binaries:
- Windows: Path.Combine(Environment.SystemDirectory, "cmd.exe") and escape &.
- Linux: /usr/bin/xdg-open
- macOS: /usr/bin/open

3. Thread-safe randomness

Replace shared static Random with ThreadLocal<Random> to avoid contention and non-thread-safe usage.

Files:

ByteSync.Common/Helpers/DebugUtils.cs
ByteSync.Common/Helpers/RandomUtils.cs

Behavior Changes

Invalid user-supplied regex in filters now yields “no match” instead of throwing.
Regex matches may time out after 200–500 ms (context-dependent), returning “no match”.

Risks and Mitigations

Risk: Timeouts could be too strict for very large inputs.
Mitigation: Chosen conservative values (200–500 ms). We can tune if we observe false negatives.
Risk: Absolute paths may differ on atypical distributions.
Mitigation: Paths chosen are standard; if needed, we can add existence checks and fallbacks in a follow-up.

Checklist

Summary:
Implemented a comprehensive regex safety pass with timeouts and error handling; hardened system process invocation paths; switched to ThreadLocal<Random> to improve thread safety.

# Conflicts: # tests/ByteSync.Client.Tests/ByteSync.Client.Tests.csproj # tests/ByteSync.Common.Tests/ByteSync.Common.Tests.csproj

sonarqubecloud · 2025-08-20T10:01:55Z

Quality Gate passed

Issues
22 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull Request Overview

This PR hardens security by mitigating regex-related CPU hotspots (ReDoS), hardening process invocations, and improving thread safety of random number generation.

Adds explicit timeouts (200-500ms) to all Regex usage to prevent ReDoS attacks
Replaces direct process invocations with trusted absolute paths for system binaries
Migrates from static Random to cryptographically secure RandomNumberGenerator for thread safety

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/ByteSync.Common/Helpers/RandomUtils.cs	Replaces static Random with RandomNumberGenerator for cryptographic security
src/ByteSync.Common/Helpers/DebugUtils.cs	Removes static Random, uses RandomNumberGenerator with improved floating-point comparison
src/ByteSync.Client/Services/Updates/*.cs	Uses trusted absolute paths for system binaries and adds regex timeouts
src/ByteSync.Client/Services/Communications/*.cs	Hardens process invocation with trusted paths and adds regex timeouts
src/ByteSync.Client/Business/Filtering/*/.cs	Adds comprehensive regex timeouts and error handling to all filtering logic
src/ByteSync.Client/Business/Configurations/ApplicationSettings.cs	Adds timeout to regex pattern used in client ID generation
tests/*/.cs	Adds comprehensive test coverage for new security features

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-20T14:53:46Z

src/ByteSync.Common/Helpers/DebugUtils.cs

        double milliseconds;
-        if (minSeconds == maxSeconds)
+        double epsilon = 1e-10;
+        if (double.Abs(minSeconds - maxSeconds) < epsilon)


This should use Math.Abs instead of double.Abs. The double type does not have a static Abs method.

Suggested change

if (double.Abs(minSeconds - maxSeconds) < epsilon)

if (Math.Abs(minSeconds - maxSeconds) < epsilon)

Copilot · 2025-08-20T14:53:46Z

src/ByteSync.Common/Helpers/RandomUtils.cs

            else
            {
-                return (char) _random.Next('a', 'z');
+                return (char) RandomNumberGenerator.GetInt32('a', 'z');


The upper bound should be 'Z' + 1 since RandomNumberGenerator.GetInt32 uses exclusive upper bound, but the current code will never return 'Z'.

Copilot · 2025-08-20T14:53:47Z

src/ByteSync.Common/Helpers/RandomUtils.cs

            else
            {
-                return (char) _random.Next('a', 'z');
+                return (char) RandomNumberGenerator.GetInt32('a', 'z');


The upper bound should be 'z' + 1 since RandomNumberGenerator.GetInt32 uses exclusive upper bound, but the current code will never return 'z'.

Copilot · 2025-08-20T14:53:47Z

src/ByteSync.Common/Helpers/RandomUtils.cs

            else
            {
-                return (char) _random.Next('a', 'z');
+                return (char) RandomNumberGenerator.GetInt32('a', 'z');


The upper bound should be 'Z' + 1 since RandomNumberGenerator.GetInt32 uses exclusive upper bound, but the current code will never return 'Z'.

Copilot · 2025-08-20T14:53:47Z

src/ByteSync.Common/Helpers/RandomUtils.cs

            else
            {
-                return (char) _random.Next('a', 'z');
+                return (char) RandomNumberGenerator.GetInt32('a', 'z');


The upper bound should be 'z' + 1 since RandomNumberGenerator.GetInt32 uses exclusive upper bound, but the current code will never return 'z'.

Copilot · 2025-08-20T14:53:47Z

src/ByteSync.Common/Helpers/DebugUtils.cs

        var top = (int) Math.Pow(10, decimals);

-        var r = _random.Next(top) + 1;
+        var r = RandomNumberGenerator.GetInt32(top + 1) + 1;


This logic is incorrect. RandomNumberGenerator.GetInt32(top + 1) returns 0 to top inclusive, so adding 1 gives 1 to top+1. This should be RandomNumberGenerator.GetInt32(top) to get 0 to top-1, then add 1 to get 1 to top.

Suggested change

var r = RandomNumberGenerator.GetInt32(top + 1) + 1;

var r = RandomNumberGenerator.GetInt32(top) + 1;

paul-fresquet

Good Job, only one question 👍

paul-fresquet · 2025-08-20T17:26:36Z

src/ByteSync.Client/Services/Communications/WebAccessor.cs

question: why did you change the way processes are started? Is it security-related ?

Yes, it was a security flaw. By launching only through the environment variable, there’s a risk that it could point to a malicious program. That’s why I replaced each variable with the full path.

Ghass-M added 3 commits August 14, 2025 15:36

fix: allow safe Regex testing without timeout to prevent ReDoS

044058b

fix: make pseudorandom number generator thread-safe

edf84f2

fix: prevent malicious command injection

446baf6

Ghass-M requested a review from paul-fresquet August 14, 2025 15:34

Ghass-M and others added 11 commits August 14, 2025 16:55

fix: use a more secure RNG

1101bdd

fix: use range instead of exact equality on floating points

7191bd6

test: improve test coverage

57f68aa

test: improve code coverage

8866e04

test: improve coverage

307622d

test: improve coverage

947dfb0

test: support coverage on Common.Tests

d5af215

test: improve coverage

a3cc059

Merge branch 'master' into fix/regex-hotspot

f36a773

Merge branch 'master' into fix/regex-hotspot

1fb625e

# Conflicts: # tests/ByteSync.Client.Tests/ByteSync.Client.Tests.csproj # tests/ByteSync.Common.Tests/ByteSync.Common.Tests.csproj

Merge branch 'master' into fix/regex-hotspot

67f5025

paul-fresquet requested a review from Copilot August 20, 2025 14:51

Copilot AI reviewed Aug 20, 2025

View reviewed changes

paul-fresquet approved these changes Aug 20, 2025

View reviewed changes

Ghass-M merged commit 6e06d55 into master Aug 21, 2025
45 of 46 checks passed

Ghass-M deleted the fix/regex-hotspot branch August 21, 2025 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Mitigate security hotspots#172

[fix] Mitigate security hotspots#172
Ghass-M merged 14 commits intomasterfrom
fix/regex-hotspot

Ghass-M commented Aug 14, 2025

Uh oh!

sonarqubecloud bot commented Aug 20, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

Copilot AI Aug 20, 2025

Uh oh!

paul-fresquet left a comment

Uh oh!

paul-fresquet Aug 20, 2025

Uh oh!

Ghass-M Aug 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (double.Abs(minSeconds - maxSeconds) < epsilon)
	if (Math.Abs(minSeconds - maxSeconds) < epsilon)

	var r = RandomNumberGenerator.GetInt32(top + 1) + 1;
	var r = RandomNumberGenerator.GetInt32(top) + 1;

Conversation

Ghass-M commented Aug 14, 2025

PR Title

Overview

Key Changes

1. Regex timeouts and safety

2. Process invocation hardening

3. Thread-safe randomness

Behavior Changes

Risks and Mitigations

Checklist

Uh oh!

sonarqubecloud bot commented Aug 20, 2025

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

paul-fresquet left a comment

Choose a reason for hiding this comment

Uh oh!

paul-fresquet Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Ghass-M Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants