🛡️ Sentinel: [CRITICAL] Fix insecure go2rtc API binding #44
Conversation
Co-authored-by: GhostTypes <106415648+GhostTypes@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @GhostTypes, 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 addresses a critical security vulnerability by securing 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. 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 aims to address a critical security vulnerability by binding the go2rtc service to localhost and implementing a WebSocket proxy for camera streams. While it successfully mitigates the insecure binding of the go2rtc API, the implementation of the WebSocket proxy introduces a critical Remote Code Execution (RCE) vulnerability due to unvalidated input being passed to the go2rtc service. Furthermore, the use of session tokens in WebSocket query parameters poses a risk of credential leakage. Additionally, there are suggestions in WebSocketManager.ts to improve the graceful shutdown of the new proxy server and to optimize message buffer handling. These security issues must be addressed before merging.
| // Extract query parameters | ||
| const url = new URL(req.url || '', `http://${req.headers.host}`); | ||
| const src = url.searchParams.get('src'); | ||
|
|
||
| if (!src) { | ||
| console.error('[CameraProxy] Missing src parameter'); | ||
| ws.close(1008, 'Missing src parameter'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The camera proxy WebSocket endpoint accepts an arbitrary src parameter from the query string and passes it directly to the internal go2rtc service. Since go2rtc supports various source types including exec:, which allows execution of arbitrary shell commands, an attacker can exploit this to achieve Remote Code Execution (RCE) on the server. Even though authentication is required by default, this allows an authenticated user to execute commands with the privileges of the application, and it becomes an unauthenticated RCE if the user disables WebUI authentication in the settings.
| // Extract query parameters | |
| const url = new URL(req.url || '', `http://${req.headers.host}`); | |
| const src = url.searchParams.get('src'); | |
| if (!src) { | |
| console.error('[CameraProxy] Missing src parameter'); | |
| ws.close(1008, 'Missing src parameter'); | |
| return; | |
| } | |
| // Extract query parameters | |
| const url = new URL(req.url || '', 'http://' + req.headers.host); | |
| const src = url.searchParams.get('src'); | |
| if (!src || !/^printer_[a-zA-Z0-9_-]+$/.test(src)) { | |
| console.error('[CameraProxy] Invalid or missing src parameter'); | |
| ws.close(1008, 'Invalid src parameter'); | |
| return; | |
| } |
| // Append token for authentication | ||
| if (token) { | ||
| wsUrl += `&token=${token}`; | ||
| } |
There was a problem hiding this comment.
The session token is appended as a query parameter to the WebSocket URL. Sensitive information in query parameters can be leaked through web server logs, reverse proxy logs, and browser history. It is recommended to use a more secure method for passing authentication tokens, such as the Sec-WebSocket-Protocol header or a short-lived one-time token.
| this.cameraProxyWss.close(() => { | ||
| console.log('Camera Proxy WebSocket server shut down'); | ||
| }); | ||
| this.cameraProxyWss = null; |
There was a problem hiding this comment.
In the shutdown method, existing client connections on cameraProxyWss are not being closed. The WebSocketServer.close() method only prevents new connections but doesn't terminate existing ones. This can lead to resource leaks as connections may remain open after server shutdown. You should iterate over this.cameraProxyWss.clients and explicitly close each connection, similar to how connections for the main wss server are handled.
for (const client of this.cameraProxyWss.clients) {
client.close(1000, 'Server shutting down');
}
this.cameraProxyWss.close(() => {
console.log('Camera Proxy WebSocket server shut down');
});
this.cameraProxyWss = null;| while (messageBuffer.length > 0) { | ||
| const data = messageBuffer.shift(); | ||
| if (data) upstreamWs.send(data); | ||
| } |
There was a problem hiding this comment.
Using Array.prototype.shift() in a loop to flush the message buffer can be inefficient, as it has a time complexity of O(n) for each element removed. For a potentially large buffer of WebSocket messages, this could introduce performance overhead. A more performant approach is to iterate over the array with a for...of loop and then clear it by setting its length to 0.
for (const data of messageBuffer) {
upstreamWs.send(data);
}
messageBuffer.length = 0;
🚨 Severity: CRITICAL
💡 Vulnerability: The go2rtc camera streaming service was listening on 0.0.0.0, exposing unauthenticated API and streams to the network.
🎯 Impact: Attackers on the local network could view camera feeds and manage streams without authentication.
🔧 Fix: Bound go2rtc API to 127.0.0.1 and implemented a secure WebSocket proxy in the WebUI to allow authorized remote access.
✅ Verification: Verified code binds to localhost and proxy forwards traffic with authentication checks.
PR created automatically by Jules for task 1989952434199591967 started by @GhostTypes