Fixed the wasm integrity checking, xxs, prototype pollution, RCE, and created the utils and scripts to generate and apply the hashes#771
Conversation
…ed the prototype buffer and fixed the RCE. Other improvements - Created the generate hashes and apply hashes scripts - Updated the security tests to include the new security tests - Updated the package.json to include the new generate hashes - And created a generated chassis.jl script to be included as part of the build process.
Summary of ChangesHello @nextep, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly strengthens the security posture of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security enhancements, addressing critical vulnerabilities like Remote Code Execution (RCE) through WASM integrity checks, prototype pollution, and Cross-Site Scripting (XSS) with robust implementations and new utility functions. However, the security hardening is incomplete. The XSS sanitizer is easily bypassed due to restrictive regular expressions, and integrity checks are missing for critical artifacts such as chat configuration and tokenizer files, leaving them vulnerable to cache poisoning. It is recommended to use a robust HTML sanitization library and enforce integrity verification for all remote and cached artifacts.
| export function sanitizeString(str: any): any { | ||
| if (typeof str !== 'string') return str; | ||
| // Strip <script> tags and onXXX event handlers | ||
| return str | ||
| .replace(/<script\b[^>]*>([\s\S]*?)<\/script>/gim, "") | ||
| .replace(/on\w+="[^"]*"/gim, "") | ||
| .replace(/on\w+='[^']*'/gim, "") | ||
| .replace(/on\w+=[^\s>]+/gim, ""); | ||
| } |
There was a problem hiding this comment.
The sanitizeString function's regex-based sanitization is easily bypassed due to its blacklist approach and restrictive regular expressions. For instance, the regexes for stripping event handlers do not account for spaces around the assignment operator, allowing bypasses like <img src=x onmouseover = "alert(1)">. Additionally, it fails to handle other common XSS vectors such as javascript: pseudo-protocols or dangerous tags like <iframe> or <object>. This could lead to Cross-Site Scripting (XSS) if strings are rendered in the UI. Consider using a robust, well-vetted HTML sanitization library like DOMPurify instead of brittle regex-based denylists for security-critical sanitization.
| const fetchedConfig = await configCache.fetchWithCache( | ||
| configUrl, | ||
| "json", | ||
| this.reloadController?.signal, | ||
| ); |
There was a problem hiding this comment.
The pull request introduces integrity verification for WASM binaries to prevent RCE and cache poisoning. However, it fails to implement similar checks for the chat configuration (mlc-chat-config.json), even though the chat_config_integrity field was added to the ModelRecord interface. This leaves the application vulnerable to persistent compromise via cache poisoning of the configuration file. An attacker who can poison the cache could manipulate model settings or inject XSS payloads into fields like system_message.
| const models = []; | ||
| const lines = configContent.split('\n'); | ||
| let currentModelId = null; | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| const idMatch = line.match(/model_id:\s+"(.+?)"/); | ||
| if (idMatch) { | ||
| currentModelId = idMatch[1]; | ||
| } | ||
|
|
||
| if (currentModelId) { | ||
| // Look for wasm file in subsequent lines | ||
| const wasmMatch = line.match(/"\/(.+?\.wasm)"/); | ||
| if (wasmMatch) { | ||
| const wasmPath = wasmMatch[1]; | ||
| models.push({ | ||
| id: currentModelId, | ||
| path: wasmPath, | ||
| // Construct URL using the prefix and version from config | ||
| url: `${modelLibURLPrefix}${modelVersion}/${wasmPath}` | ||
| }); | ||
| currentModelId = null; // Reset | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current approach of parsing src/config.ts with line-by-line string matching is quite fragile and could easily break if the file's formatting changes in the future. A more robust solution would be to refactor the model list into a separate JSON file. This file could then be imported directly by both this script (for hash generation) and src/config.ts (to build the prebuiltAppConfig), eliminating the need for brittle regex parsing.
…ss multiple security vulnerabilities.
|
Hi @nextep, thanks for the contribution! I think we should separate the fixes for each vulnerability into separate PRs, both because they're distinct and because it will make reviewing easier. I'll work on the integrity verification separately, but if you could create separate PRs for vulnerabilities 2-4, that would be great! |
|
Hi @akaashrp, thanks for checking the PR and absolutely I can do that for you. Give me some time to improve the fixes since they were rather simple as I was in a rush. Just remembered that in the integrity verification, I only modified the primary config file but not the sample config despite adding the placeholder for it. |
2 participants
…ed the prototype buffer and fixed the RCE. Other improvements