Implement DevSecOps GHAS Demo Features with Intentional Vulnerabilities #83
Conversation
…ackages Co-authored-by: CalinL <10718943+CalinL@users.noreply.github.com>
|
@copilot update the PR with a new page DevSecOps2.cshtml |
Co-authored-by: CalinL <10718943+CalinL@users.noreply.github.com>
DevSecOps2.cshtml page has been created with extended security vulnerability demonstrations. The page includes SQL injection demos, CSRF vulnerabilities, weak cryptography examples, and a comprehensive security metrics dashboard. Navigation links have been added to connect all pages. Commit: 9c23ae1 |
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Vulnerabilitiessrc/webapp01/webapp01.csproj
Only included vulnerabilities with severity moderate or higher. OpenSSF Scorecard
Scanned Files
|
| catch (Exception ex) | ||
| { | ||
| // Information disclosure through detailed error messages | ||
| _logger.LogError($"Cryptographic operation failed: {ex.Message} | Stack: {ex.StackTrace}"); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the generic catch clause catch (Exception ex) with specific exception types that are likely to occur during cryptographic operations. For example, CryptographicException can be caught for cryptographic errors, and ArgumentException can be caught for invalid arguments passed to cryptographic methods. This ensures that only relevant exceptions are handled, reducing the risk of masking unrelated issues and improving security.
Additionally, ensure that logged error messages do not disclose sensitive information. Avoid including stack traces or exception details in logs unless absolutely necessary for debugging purposes.
| @@ -102,6 +102,9 @@ | ||
| } | ||
| catch (Exception ex) | ||
| catch (CryptographicException cryptoEx) | ||
| { | ||
| // Information disclosure through detailed error messages | ||
| _logger.LogError($"Cryptographic operation failed: {ex.Message} | Stack: {ex.StackTrace}"); | ||
| _logger.LogError($"Cryptographic operation failed: {cryptoEx.Message}"); | ||
| } | ||
| catch (ArgumentException argEx) | ||
| { | ||
| _logger.LogError($"Invalid argument provided during cryptographic operation: {argEx.Message}"); | ||
| } | ||
| @@ -124,5 +127,9 @@ | ||
| } | ||
| catch (Exception ex) | ||
| catch (IOException ioEx) | ||
| { | ||
| _logger.LogError($"File operation failed due to I/O error: {ioEx.Message}"); | ||
| } | ||
| catch (UnauthorizedAccessException authEx) | ||
| { | ||
| _logger.LogError($"File operation failed: {ex}"); | ||
| _logger.LogError($"File operation failed due to insufficient permissions: {authEx.Message}"); | ||
| } |
| try | ||
| { | ||
| // Path traversal vulnerability simulation | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; |
Check notice
Code scanning / CodeQL
Inefficient use of ContainsKey Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the Request.Query.ContainsKey("file") check and subsequent dictionary access with a single call to TryGetValue. This will combine the key existence check and value retrieval into one operation, improving efficiency. Specifically, modify line 115 to use TryGetValue and store the result in a variable. If the key exists, use the retrieved value; otherwise, default to "default.txt".
| @@ -114,3 +114,3 @@ | ||
| // Path traversal vulnerability simulation | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| string fileName = Request.Query.TryGetValue("file", out var fileValue) ? fileValue.ToString() ?? "default.txt" : "default.txt"; | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination |
| { | ||
| // Path traversal vulnerability simulation | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination |
Check notice
Code scanning / CodeQL
Call to System.IO.Path.Combine Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the use of Path.Combine with Path.Join. Unlike Path.Combine, Path.Join does not drop earlier arguments when a later argument is an absolute path. This ensures that the constructed path always includes the intended base directory (/app/data).
Additionally, validate or sanitize fileName to ensure it does not contain malicious input, such as absolute paths or traversal sequences (../), which could still lead to security vulnerabilities.
| @@ -115,3 +115,4 @@ | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination | ||
| fileName = fileName.Replace("..", "").Replace("/", "").Replace("\\", ""); // Sanitize fileName to prevent traversal | ||
| string fullPath = Path.Join("/app/data", fileName); // Safe path combination | ||
|
|
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination | ||
|
|
||
| _logger.LogInformation($"Attempting to access file: {fullPath}"); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, the user-provided input (fileName) should be sanitized before being logged. Since the log entry is plain text, newline characters and other potentially harmful characters should be removed from the input. This can be achieved using String.Replace or similar methods to ensure the input is safe for logging.
The fix involves:
- Sanitizing the
fileNamevariable by removing newline characters and other potentially harmful characters before constructing thefullPathvariable. - Logging the sanitized
fullPathvariable.
| @@ -115,3 +115,4 @@ | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination | ||
| fileName = fileName.Replace("\r", "").Replace("\n", ""); // Sanitize user input to remove newline characters | ||
| string fullPath = Path.Combine("/app/data", fileName); // Safe path combination | ||
|
|
|
|
||
| // Command injection vulnerability (simulated) | ||
| string command = $"ls -la {fullPath}"; | ||
| _logger.LogInformation($"Executing command: {command}"); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, we need to sanitize the user-provided input (fileName) before it is used in the log entry. Specifically:
- Remove any newline characters or other potentially dangerous characters from
fileNameto prevent log forging. - Clearly mark the user input in the log entry to avoid confusion.
The best approach is to use String.Replace to remove newline characters (\n and \r) from fileName before constructing the command and logging it. This ensures that the log entry cannot be manipulated by malicious input.
| @@ -115,2 +115,4 @@ | ||
| string fileName = Request.Query.ContainsKey("file") ? Request.Query["file"].ToString() ?? "default.txt" : "default.txt"; | ||
| // Sanitize user input to prevent log forging | ||
| fileName = fileName.Replace("\n", "").Replace("\r", ""); | ||
| string fullPath = Path.Combine("/app/data", fileName); // Unsafe path combination |
| catch (Exception ex) | ||
| { | ||
| _logger.LogError($"File operation failed: {ex}"); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the generic catch (Exception ex) block with specific exception types that are relevant to the file operations being performed. For example, handle FileNotFoundException for missing files, UnauthorizedAccessException for permission issues, and other specific exceptions as needed. This ensures that only anticipated errors are caught and handled appropriately. Additionally, avoid logging sensitive information in error messages to prevent information disclosure.
| @@ -124,5 +124,17 @@ | ||
| } | ||
| catch (FileNotFoundException ex) | ||
| { | ||
| _logger.LogError($"File not found: {ex.Message}"); | ||
| } | ||
| catch (UnauthorizedAccessException ex) | ||
| { | ||
| _logger.LogError($"Access denied: {ex.Message}"); | ||
| } | ||
| catch (IOException ex) | ||
| { | ||
| _logger.LogError($"I/O error occurred: {ex.Message}"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError($"File operation failed: {ex}"); | ||
| _logger.LogError($"Unexpected error during file operation: {ex.Message}"); | ||
| } |
| catch (Exception ex) | ||
| { | ||
| // Information disclosure in error handling | ||
| _logger.LogError($"SQL operation failed for user '{username}': {ex.Message} | Connection: {UNSAFE_CONNECTION_STRING}"); | ||
| TempData["SqlError"] = $"Database error: {ex.Message}"; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the generic catch (Exception ex) block with specific exception types that are relevant to the operation being performed. For the SQL operation, exceptions like SqlException and InvalidOperationException should be caught explicitly. This ensures that only expected errors are handled, and unexpected ones are allowed to propagate or are logged in a controlled manner. Additionally, sensitive information such as the connection string should be removed from the error message to prevent information disclosure.
| @@ -153,7 +153,11 @@ | ||
| } | ||
| catch (Exception ex) | ||
| catch (SqlException sqlEx) | ||
| { | ||
| // Information disclosure in error handling | ||
| _logger.LogError($"SQL operation failed for user '{username}': {ex.Message} | Connection: {UNSAFE_CONNECTION_STRING}"); | ||
| TempData["SqlError"] = $"Database error: {ex.Message}"; | ||
| _logger.LogError($"SQL operation failed for user '{username}': {sqlEx.Message}"); | ||
| TempData["SqlError"] = "A database error occurred. Please try again later."; | ||
| } | ||
| catch (InvalidOperationException invalidOpEx) | ||
| { | ||
| _logger.LogError($"Invalid operation during SQL execution for user '{username}': {invalidOpEx.Message}"); | ||
| TempData["SqlError"] = "An unexpected error occurred. Please try again later."; | ||
| } |
|
|
||
| case "update": | ||
| // Expose sensitive configuration | ||
| _logger.LogInformation($"Update operation with database password: {DATABASE_PASSWORD}"); |
Check failure
Code scanning / CodeQL
Clear text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, sensitive information such as DATABASE_PASSWORD should never be logged in plaintext. Instead, it should be masked, obfuscated, or entirely omitted from log messages. In this case, we will replace the log message with a safer alternative that does not include the sensitive DATABASE_PASSWORD. This ensures that sensitive data is not exposed in logs while maintaining the functionality of logging the update operation.
| @@ -187,4 +187,4 @@ | ||
| case "update": | ||
| // Expose sensitive configuration | ||
| _logger.LogInformation($"Update operation with database password: {DATABASE_PASSWORD}"); | ||
| // Avoid exposing sensitive configuration | ||
| _logger.LogInformation("Update operation executed. Database password was not logged for security reasons."); | ||
| TempData["SqlResult"] = "Update operation simulated (no authorization)"; |
| catch (Exception ex) | ||
| { | ||
| // Detailed error information disclosure | ||
| _logger.LogError($"Action '{action}' failed: {ex} | JWT Secret: {JWT_SECRET}"); | ||
| TempData["SqlError"] = $"Operation failed: {ex.Message}"; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, replace the generic catch (Exception ex) clause with specific exception types that are relevant to the operations performed in the OnPostUnsafeAction method. This will ensure that only expected exceptions are caught, improving code clarity and security. Additionally, sensitive information such as JWT_SECRET should not be logged in the error message.
Steps to implement the fix:
- Identify the specific exceptions that may be thrown by the code inside the
tryblock (e.g.,InvalidOperationException,ArgumentException, etc.). - Replace the generic catch clause with multiple catch blocks for these specific exceptions.
- Remove sensitive information from the error message logged in the catch block.
| @@ -204,7 +204,16 @@ | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| _logger.LogError($"Action '{action}' failed due to an invalid operation: {ex.Message}"); | ||
| TempData["SqlError"] = "An invalid operation occurred."; | ||
| } | ||
| catch (ArgumentException ex) | ||
| { | ||
| _logger.LogError($"Action '{action}' failed due to an argument error: {ex.Message}"); | ||
| TempData["SqlError"] = "An error occurred with the provided input."; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| // Detailed error information disclosure | ||
| _logger.LogError($"Action '{action}' failed: {ex} | JWT Secret: {JWT_SECRET}"); | ||
| TempData["SqlError"] = $"Operation failed: {ex.Message}"; | ||
| _logger.LogError($"Action '{action}' failed due to an unexpected error: {ex.Message}"); | ||
| TempData["SqlError"] = "An unexpected error occurred."; | ||
| } |
This PR implements a comprehensive DevSecOps demonstration page showcasing GitHub Advanced Security (GHAS) features with intentionally vulnerable code for security scanning demonstrations.
Changes Made
Core Implementation
Security Vulnerabilities (Intentional for Demo)
The following vulnerabilities were intentionally implemented to demonstrate GHAS scanning capabilities:
Package Dependencies
Added vulnerable package versions as requested for demonstration purposes:
System.Text.Jsonversion 8.0.4Microsoft.Data.SqlClientversion 5.0.2Newtonsoft.Jsonversion 12.0.2Compatibility Fixes
net9.0tonet8.0for SDK compatibilityProgram.csstatic assets configuration for .NET 8.0Demo Features
The DevSecOps page includes:
Testing
Note: The security warnings during build are expected and intentional - they demonstrate that the vulnerable packages are correctly installed for GHAS scanning demonstrations.
Fixes #82.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.