fix: Address critical security vulnerabilities from issue #121#134
fix: Address critical security vulnerabilities from issue #121#134sarpit2907 wants to merge 1 commit intosoft-eng-practicum:developfrom
Conversation
|
@sarpit2907 thanks for submitting this PR and sorry for the late reply. Trying to test it, I'm getting the following errors. Did you forget to add some of the new packages in the CSPROJ files? |
|
@sarpit2907 can you also add to the README the necessary changes for |
…acticum#121 **Vulnerability 1: Exposed Authentication Data** - Created UserSafeDTO to sanitize API responses - Removed sensitive fields (passwordHash, securityStamp, concurrencyStamp) from all user endpoints - Updated 5 endpoints: GetUserByID, GetUserByName, GetUserRange, GetUserList, Search - Files: AccountController.cs, ViewModels/Account/UserSafeDTO.cs **Vulnerability 2: Missing File Upload Size Restrictions** - Created FileValidationSettings configuration class - Added per-file size limits: 5MB (profile images), 100MB (project files), 50MB (notebooks) - Implemented configuration via appsettings.json - Added validation checks to all upload endpoints before storing files - Files: FileValidationSettings.cs, appsettings.json, ProjectController.cs, AccountController.cs **Vulnerability 3: No File Type Validation** - Created FileTypeValidator with allowlist-based validation - Implemented file signature/magic number verification - Profile images: .jpg, .jpeg, .png, .gif, .webp only - Project files: .csv, .json, .txt, .xlsx, .pdf, .xml, .tsv, .dat only - Notebooks: .ipynb only - Blocked dangerous extensions: .exe, .bat, .cmd, .sh, .ps1, etc. - Applied validation to all 5 upload endpoints - Files: FileTypeValidator.cs, ProjectController.cs, AccountController.cs **Endpoints Fixed:** - ProjectController: UploadFile, UploadNotebook, UploadNotebookNewVersion, UploadExistingNotebook - AccountController: UploadProfileImage, GetUserByID, GetUserByName, GetUserRange, GetUserList, Search **Security Improvements:** - 100% file validation at API boundary before database storage - Existing user quota checks remain as additional security layer - Configuration-driven approach allows easy adjustment of limits - No breaking changes - only response DTOs and validation logic modified Closes soft-eng-practicum#121
sarpit2907
force-pushed
the
fix/security-vulnerabilities-issue-121
branch
from
6b207a3 to
2332421
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to close issue #121 by preventing sensitive Identity fields from leaking in user-facing API responses and by adding allowlist-based validation (type + size) to file upload endpoints.
Changes:
- Introduces
UserSafeDTOand updates Account user endpoints to return sanitized user data. - Adds configurable
FileValidationSettingsand aFileTypeValidatorhelper. - Wires validation settings via DI and applies validation to profile image, project file, and notebook upload flows; documents new config in
README.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Analysim.Web/ViewModels/Account/UserSafeDTO.cs | Adds a “safe” user DTO and mapping helpers to exclude sensitive fields from API responses. |
| src/Analysim.Web/Startup.cs | Registers FileValidationSettings from configuration for DI. |
| src/Analysim.Web/Controllers/AccountController.cs | Uses UserSafeDTO for user endpoints; adds validation to profile image uploads. |
| src/Analysim.Web/Controllers/ProjectController.cs | Adds validation to project file + notebook upload endpoints (including Colab download flow). |
| src/Analysim.Core/Helper/FileValidationSettings.cs | Defines configurable size limits and extension allow/block lists. |
| src/Analysim.Core/Helper/FileTypeValidator.cs | Implements file extension + content validation logic used by upload endpoints. |
| README.md | Documents the new FileValidation configuration section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If no magic number matched, assume it's okay for formats we can't verify | ||
| // (like CSV, TXT, XML without specific magic numbers) | ||
| if (extension == ".xml") | ||
| return IsValidTextContent(fileContent) && fileContent.ToString().Contains("<"); |
There was a problem hiding this comment.
VerifyFileSignature attempts to validate XML by calling fileContent.ToString().Contains("<"), but byte[].ToString() returns the type name (e.g., "System.Byte[]"), not the decoded content. This will cause all .xml uploads to fail validation. Decode the bytes to text (e.g., UTF-8) and check the decoded string, or reuse IsValidTextContent/a proper XML parse check.
| return IsValidTextContent(fileContent) && fileContent.ToString().Contains("<"); | |
| { | |
| var text = System.Text.Encoding.UTF8.GetString(fileContent); | |
| return IsValidTextContent(fileContent) && text.Contains("<"); | |
| } |
| { new byte[] { 0xFF, 0xD8, 0xFF }, ".jpg" }, // JPEG | ||
| { new byte[] { 0x89, 0x50, 0x4E, 0x47 }, ".png" }, // PNG | ||
| { new byte[] { 0x47, 0x49, 0x46 }, ".gif" }, // GIF | ||
| { new byte[] { 0x52, 0x49, 0x46, 0x46 }, ".webp" }, // WEBP |
There was a problem hiding this comment.
The current WEBP signature check only validates the RIFF header, which is shared by other RIFF-based formats (e.g., WAV/AVI). This allows non-WEBP files renamed to .webp to pass validation. Consider validating both RIFF at bytes 0-3 and WEBP at bytes 8-11 (or equivalent robust WEBP signature checks).
| { new byte[] { 0x52, 0x49, 0x46, 0x46 }, ".webp" }, // WEBP |
| // For binary formats, check magic numbers | ||
| foreach (var kvp in FileMagicNumbers) | ||
| { | ||
| if (fileContent.Length >= kvp.Key.Length && | ||
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | ||
| { | ||
| return kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls"); |
There was a problem hiding this comment.
.xls is allowlisted, but the signature logic effectively treats .xls as acceptable when the content matches the ZIP-based .xlsx signature. This will reject real legacy .xls files (OLE2 signature) and can allow .xlsx content disguised as .xls. Either remove .xls from the allowed list or implement proper .xls signature validation.
| // For binary formats, check magic numbers | |
| foreach (var kvp in FileMagicNumbers) | |
| { | |
| if (fileContent.Length >= kvp.Key.Length && | |
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | |
| { | |
| return kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls"); | |
| // Explicit validation for legacy Excel (.xls) using OLE2 compound file signature | |
| if (extension == ".xls") | |
| { | |
| // OLE2 magic number: D0 CF 11 E0 A1 B1 1A E1 | |
| byte[] ole2Signature = new byte[] { 0xD0, 0xCF, 0x11, 0xE0, 0xA1, 0xB1, 0x1A, 0xE1 }; | |
| if (fileContent.Length < ole2Signature.Length) | |
| return false; | |
| for (int i = 0; i < ole2Signature.Length; i++) | |
| { | |
| if (fileContent[i] != ole2Signature[i]) | |
| return false; | |
| } | |
| return true; | |
| } | |
| // For binary formats, check magic numbers | |
| foreach (var kvp in FileMagicNumbers) | |
| { | |
| if (fileContent.Length >= kvp.Key.Length && | |
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | |
| { | |
| return kvp.Value == extension; |
| // Check if extension is blocked | ||
| if (_settings.BlockedExtensions?.Contains(extension) == true) | ||
| return (false, $"File type '{extension}' is not allowed."); | ||
|
|
||
| // Check if extension is in allowlist | ||
| if (!allowedExtensions.Contains(extension)) |
There was a problem hiding this comment.
allowedExtensions.Contains(extension) is case-sensitive, but extension lists can be configured with different casing in appsettings. This can cause valid uploads to be rejected depending on configuration values. Consider normalizing configured extensions or using a case-insensitive comparison.
| // Check if extension is blocked | |
| if (_settings.BlockedExtensions?.Contains(extension) == true) | |
| return (false, $"File type '{extension}' is not allowed."); | |
| // Check if extension is in allowlist | |
| if (!allowedExtensions.Contains(extension)) | |
| // Check if extension is blocked (case-insensitive) | |
| if (_settings.BlockedExtensions?.Any(e => string.Equals(e, extension, StringComparison.OrdinalIgnoreCase)) == true) | |
| return (false, $"File type '{extension}' is not allowed."); | |
| // Check if extension is in allowlist (case-insensitive) | |
| if (!allowedExtensions.Any(e => string.Equals(e, extension, StringComparison.OrdinalIgnoreCase))) |
| foreach (var kvp in FileMagicNumbers) | ||
| { | ||
| if (fileContent.Length >= kvp.Key.Length && | ||
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | ||
| { | ||
| return kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls"); | ||
| } | ||
| } | ||
|
|
||
| // If no magic number matched, assume it's okay for formats we can't verify | ||
| // (like CSV, TXT, XML without specific magic numbers) | ||
| if (extension == ".xml") | ||
| return IsValidTextContent(fileContent) && fileContent.ToString().Contains("<"); | ||
|
|
||
| return true; // Allow if we can't determine a signature |
There was a problem hiding this comment.
VerifyFileSignature defaults to allowing the file when no known magic number matches. That makes signature validation bypassable for many allowed types (e.g., an arbitrary binary renamed to .png/.pdf will pass if it doesn't match a different known signature). For extensions with known signatures, consider requiring a positive match rather than default-allowing.
| foreach (var kvp in FileMagicNumbers) | |
| { | |
| if (fileContent.Length >= kvp.Key.Length && | |
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | |
| { | |
| return kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls"); | |
| } | |
| } | |
| // If no magic number matched, assume it's okay for formats we can't verify | |
| // (like CSV, TXT, XML without specific magic numbers) | |
| if (extension == ".xml") | |
| return IsValidTextContent(fileContent) && fileContent.ToString().Contains("<"); | |
| return true; // Allow if we can't determine a signature | |
| var signatureMatched = false; | |
| foreach (var kvp in FileMagicNumbers) | |
| { | |
| if (fileContent.Length >= kvp.Key.Length && | |
| fileContent.Take(kvp.Key.Length).SequenceEqual(kvp.Key)) | |
| { | |
| signatureMatched = kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls"); | |
| if (signatureMatched) | |
| { | |
| return true; | |
| } | |
| } | |
| } | |
| // If no magic number matched, and the extension has a known signature, reject the file | |
| var hasKnownSignature = FileMagicNumbers.Any(kvp => | |
| kvp.Value == extension || (kvp.Value == ".xlsx" && extension == ".xls")); | |
| if (hasKnownSignature) | |
| { | |
| return false; | |
| } | |
| // For formats without specific magic numbers (like XML), perform basic sanity checks | |
| if (extension == ".xml") | |
| { | |
| if (!IsValidTextContent(fileContent)) | |
| return false; | |
| var text = System.Text.Encoding.UTF8.GetString(fileContent); | |
| return text.Contains("<"); | |
| } | |
| // Allow if we can't determine a signature and no signature is expected | |
| return true; |
| using var memoryStream = new MemoryStream(); | ||
| await formdata.File.CopyToAsync(memoryStream); | ||
| var fileContent = memoryStream.ToArray(); | ||
|
|
There was a problem hiding this comment.
The upload is copied into a MemoryStream and materialized via ToArray() before any max-size validation. To make the size limit effective against memory pressure/DoS, validate formdata.File.Length against the configured project-file limit before buffering the file.
| using var memoryStream = new MemoryStream(); | ||
| await noteBookData.NotebookFile.CopyToAsync(memoryStream); | ||
| var fileContent = memoryStream.ToArray(); | ||
|
|
There was a problem hiding this comment.
The notebook is fully buffered into memory before enforcing the configured max size. Validate noteBookData.NotebookFile.Length against the configured notebook limit before CopyToAsync/ToArray() to avoid large allocations for oversized uploads.
| using var memoryStream = new MemoryStream(); | ||
| await noteBookData.NotebookFile.CopyToAsync(memoryStream); | ||
| var fileContent = memoryStream.ToArray(); | ||
|
|
There was a problem hiding this comment.
The new notebook version is fully buffered into memory before size validation. Validate noteBookData.NotebookFile.Length against the configured notebook limit before CopyToAsync/ToArray() to avoid large allocations for oversized uploads.
| byte[] fileContent = System.IO.File.ReadAllBytes(filePath); | ||
|
|
There was a problem hiding this comment.
For Colab downloads, the entire file is read into memory (ReadAllBytes) before applying the configured size limit. To prevent large allocations for oversized downloads, check the on-disk size (fileStream.Length/FileInfo.Length) against the configured notebook limit before reading all bytes.
| byte[] fileContent = System.IO.File.ReadAllBytes(filePath); | |
| // Prevent large allocations for oversized notebook downloads by checking on-disk size first | |
| const long MaxNotebookSizeBytes = 50L * 1024L * 1024L; // 50 MB limit for Colab notebook downloads | |
| if (fileStream.Length > MaxNotebookSizeBytes) | |
| { | |
| return BadRequest("The uploaded notebook file is too large."); | |
| } | |
| byte[] fileContent; | |
| using (var memoryStream = new MemoryStream((int)fileStream.Length)) | |
| { | |
| await fileStream.CopyToAsync(memoryStream); | |
| fileContent = memoryStream.ToArray(); | |
| } |
| // Validate notebook file type and size for colab downloads | ||
| var fileValidator = new Core.Helper.FileTypeValidator(_fileValidationSettings); | ||
| var validationResult = fileValidator.ValidateNotebookFile(fileName, fileContent); | ||
| if (!validationResult.IsValid) | ||
| return BadRequest(validationResult.ErrorMessage); |
There was a problem hiding this comment.
In the Colab download flow, validation failures return early inside this block, which bypasses the temp-file deletion later in the method. This can leave invalid/oversized downloads on disk. Consider ensuring cleanup runs in a finally (or delete before returning on validation failure).
|
Hi @cengique , thanks for the review and apologies for the delay on my end! I've addressed both issues:
Config settings now read from appsettings.json (no longer hard-coded):
README updated with the FileValidation section and an explanation of each configurable setting. Build passes with 0 errors. Let me know if anything else needs to be adjusted! |
Summary
This pull request addresses three critical security vulnerabilities identified in issue #121:
passwordHash,securityStamp, andconcurrencyStampwere being exposed in user API endpointsChanges Made
1. User Authentication Data Sanitization
UserSafeDTOclass to return only public-safe user fieldsGetUserByIDGetUserByNameGetUserRangeGetUserListSearchPasswordHash,SecurityStamp,ConcurrencyStamp,NormalizedEmail,NormalizedUserName, and other IdentityUser properties2. File Upload Size Validation
FileValidationSettingsconfiguration class with configurable size limits:appsettings.json3. File Type Validation
FileTypeValidatorwith allowlist-based validation.jpg,.jpeg,.png,.gif,.webp(image types only).csv,.json,.txt,.xlsx,.xls,.pdf,.xml,.tsv,.dat(data formats).ipynbonly.exe,.bat,.cmd,.sh,.ps1,.app,.dll,.so,.dmg,.pkg,.msi,.deb,.rpm,.apk, and scriptsUploadFile(ProjectController)UploadProfileImage(AccountController)UploadNotebook(ProjectController)UploadNotebookNewVersion(ProjectController)UploadExistingNotebook(ProjectController - colab type)Files Modified
New Files:
src/Analysim.Web/ViewModels/Account/UserSafeDTO.cssrc/Analysim.Core/Helper/FileValidationSettings.cssrc/Analysim.Core/Helper/FileTypeValidator.cssrc/Analysim.Web/appsettings.jsonModified Files:
src/Analysim.Web/Controllers/AccountController.cs- Added UserSafeDTO usage and profile image validationsrc/Analysim.Web/Controllers/ProjectController.cs- Added file validation to all upload endpointsSecurity Benefits
Testing Recommendations
User API Response Sanitization:
/api/account/searchendpointpasswordHash,securityStamp,concurrencyStampid,userName,email,bio,dateCreatedFile Size Validation:
File Type Validation:
.exefile → Should return 400 "File type not allowed".shscript → Should return 400 "File type not allowed".csvfile → Should succeed.jpgimage for profile → Should succeed.ipynbnotebook → Should succeedRelated Issue
Closes #121
This pull request improves security by addressing OWASP Top 10 vulnerabilities related to file upload handling and information disclosure.