🔒 Hardened /tmp permissions in Nextcloud container#228
Conversation
- Verify /tmp is not a symbolic link to prevent symlink attacks - Restrict permissions to 1770 and ownership to root:www-data where possible - Consolidate into a single docker exec call for atomicity Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, 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 enhances the security posture of the Nextcloud container by addressing insecure Highlights
Changelog
Activity
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
|
|
❌ Lint/Format Check Failed Please run |
![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 effectively hardens the /tmp directory permissions within the Nextcloud container, addressing a potential symlink vulnerability and overly permissive access rights. The change correctly consolidates multiple docker exec calls into a single, more atomic operation. My feedback includes minor suggestions to align the new script block with the repository's bash standards and to improve the readability of the conditional logic.
| if ! sudo docker exec nextcloud sh -c ' | ||
| set -eu | ||
| if [ -L /tmp ]; then | ||
| rm /tmp | ||
| fi | ||
| if ! sudo docker exec nextcloud chmod 1777 /tmp; then | ||
| warn "Failed to chmod /tmp in nextcloud" | ||
| perms_ok=0 | ||
| fi | ||
| if (( perms_ok )); then | ||
| log "Nextcloud permissions fixed" | ||
| mkdir -p /tmp | ||
| if getent group www-data >/dev/null 2>&1; then | ||
| chown root:www-data /tmp | ||
| chmod 1770 /tmp | ||
| else | ||
| warn "Nextcloud permission fix incomplete" | ||
| chown root:root /tmp | ||
| chmod 1777 /tmp | ||
| fi | ||
| '; then | ||
| warn "Failed to fix /tmp permissions in nextcloud" | ||
| else | ||
| warn "Failed to access /tmp in nextcloud container" | ||
| log "Nextcloud permissions fixed" | ||
| fi |
There was a problem hiding this comment.
This is a great security hardening improvement. I have a few suggestions to align it more closely with the repository's style guide and improve readability:
- Use
[[ ... ]]for tests: The style guide (line 117) specifies using[[ ... ]]for tests. The check for the symbolic link should be updated from[ -L /tmp ]to[[ -L /tmp ]]. - Include
pipefail: The style guide (line 108) recommendsset -euo pipefail. Thepipefailoption is missing from theset -eucommand within thedocker execscript. - Improve readability: The
if ! ...; then [fail]; else [success]; filogic is inverted and can be harder to read. It's more conventional to useif ...; then [success]; else [fail]; fi.
Here is a suggested implementation that incorporates these points:
| if ! sudo docker exec nextcloud sh -c ' | |
| set -eu | |
| if [ -L /tmp ]; then | |
| rm /tmp | |
| fi | |
| if ! sudo docker exec nextcloud chmod 1777 /tmp; then | |
| warn "Failed to chmod /tmp in nextcloud" | |
| perms_ok=0 | |
| fi | |
| if (( perms_ok )); then | |
| log "Nextcloud permissions fixed" | |
| mkdir -p /tmp | |
| if getent group www-data >/dev/null 2>&1; then | |
| chown root:www-data /tmp | |
| chmod 1770 /tmp | |
| else | |
| warn "Nextcloud permission fix incomplete" | |
| chown root:root /tmp | |
| chmod 1777 /tmp | |
| fi | |
| '; then | |
| warn "Failed to fix /tmp permissions in nextcloud" | |
| else | |
| warn "Failed to access /tmp in nextcloud container" | |
| log "Nextcloud permissions fixed" | |
| fi | |
| if sudo docker exec nextcloud sh -c ' | |
| set -euo pipefail | |
| if [[ -L /tmp ]]; then | |
| rm /tmp | |
| fi | |
| mkdir -p /tmp | |
| if getent group www-data >/dev/null 2>&1; then | |
| chown root:www-data /tmp | |
| chmod 1770 /tmp | |
| else | |
| chown root:root /tmp | |
| chmod 1777 /tmp | |
| fi | |
| '; then | |
| log "Nextcloud permissions fixed" | |
| else | |
| warn "Failed to fix /tmp permissions in nextcloud" | |
| fi |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
AnalysisThis PR improves Nextcloud
Issues Found:
Files Reviewed (1 files)
Reviewed by minimax-m2.5-20260211 · 205,662 tokens |
There was a problem hiding this comment.
Pull request overview
Updates the Raspberry Pi Fix.sh script’s Nextcloud remediation to reset /tmp inside the running nextcloud container (including removing a /tmp symlink and recreating the directory) before applying ownership and permissions.
Changes:
- Replaces separate
docker execcalls with a single in-containersh -cscript to recreate/tmpsafely. - Applies different
/tmpownership/permission logic depending on whetherwww-datagroup exists.
You can also share your feedback on Copilot code review. Take the survey.
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
❌ Lint/Format Check Failed Please run |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if ! sudo docker exec nextcloud sh -c ' | ||
| set -eu | ||
| if [ -L /tmp ]; then | ||
| if [ -e /tmp ] && [ ! -d /tmp ]; then |
There was a problem hiding this comment.
WARNING: The symlink check [ -e /tmp ] && [ ! -d /tmp ] doesn't catch symlinks pointing to directories. If /tmp is a symlink to an existing directory, [ -d /tmp ] returns true and the symlink won't be removed. Consider using [ -L /tmp ] instead for explicit symlink detection.
| if getent group www-data >/dev/null 2>&1; then | ||
| chown root:www-data /tmp | ||
| chmod 1770 /tmp | ||
| chmod 2770 /tmp |
There was a problem hiding this comment.
SUGGESTION: The setgid bit (2770) is unnecessary for /tmp. The setgid bit only affects newly created files within the directory, but /tmp files are typically owned by the process creating them. Consider using 1770 instead of 2770.
🎯 What: Insecure directory permissions and lack of symlink verification for
⚠️ Risk: The previous
/tmpin the Nextcloud container.chmod 1777followed symlinks, potentially allowing an attacker in the container to make sensitive host-mapped files world-writable via a symlink attack. Additionally,1777provided unnecessary world-writable access in a specialized container environment.🛡️ Solution: Modified
fix_nextcloudinRaspberryPi/Scripts/Fix.shto verify that/tmpis not a symbolic link before applying changes. The fix now restricts permissions to1770withroot:www-dataownership if thewww-datagroup is present, ensuring only the necessary web server user can access temporary files. It maintains a safe fallback to1777for non-standard environments and uses a singledocker execcall for better atomicity.PR created automatically by Jules for task 13751229634068302542 started by @Ven0m0