Security hardening: control-socket auth, updater injection, fail-closed option (#31–#36) - #37
Security hardening: control-socket auth, updater injection, fail-closed option (#31–#36)#37shipdocs wants to merge 11 commits into
Conversation
…rcred (#31, #33, #35) - Create the control socket 0o660 owned root:bastion instead of 0o666; the kernel now restricts connect() to root and bastion-group members. - Verify SO_PEERCRED on every connection: allow only root or a member of the bastion group, and reject (fail closed) when credentials cannot be read instead of allowing the connection. - Refuse a second concurrent GUI connection while one is active to prevent control-channel hijacking (#33). - Match cached popup responses by request_id instead of returning an arbitrary pending response, so a response is never delivered to the wrong session (#35).
- Reject any fetched version that does not match ^\d+\.\d+\.\d+$ before it reaches a URL or a subprocess, closing the command-injection hole. - Download the release .deb in Python and install it with an argv list (pkexec apt-get install -y <file>) instead of building a 'pkexec bash -c ...' string, so no version data is interpreted by a shell.
- Add a fail_closed config flag (default false, preserving current fail-open behavior). When set, in enforcement mode the daemon drops packets it cannot parse or inspect instead of accepting them. - Wire the flag through the NFQUEUE rule string: the systemd unit and start_daemon.sh omit --queue-bypass when fail_closed is true, so traffic is dropped rather than passed when the daemon is down. ExecStopPost now removes both rule variants. - Also (#36): report the daemon version from CARGO_PKG_VERSION and correct the GUI-timeout log to the actual 60s value. - Also (#36): add #[serde(default)] to Config.mode so a config missing the mode key falls back to Learning instead of failing to parse.
- preinst: remove only Bastion's own NFQUEUE rules by exact -C/-D match;
never flush the OUTPUT chain or reset its policy.
- postinst: include "mode" in the fallback config.json.
- gui_qt/main_window: revoke the temporary xhost +si:localuser:root grant
once gufw exits.
- gui_qt import_rules: validate keys against path:port|* and values as bool
before merging.
- build_deb.sh: unquote the metainfo and changelog heredocs so ${VERSION}
and $(date) expand.
- install.sh: derive DEB_FILE from VERSION (or glob) instead of a hardcoded
nonexistent 2.0.0 filename.
- com.bastion.firewall.desktop: point Exec at /usr/bin/bastion-gui.
…33, #34) - README/SECURITY: describe the control socket as 0o660 root:bastion with SO_PEERCRED verification, bastion-group restriction, fail-closed on credential-lookup failure, and single-session enforcement. - SECURITY: document the fail_closed config option and its fail-open vs fail-closed trade-off.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR hardens the daemon control socket by restricting permissions to 0o660/root:bastion and adding fail-closed peer credential authorization, introduces a fail_closed config option applied to packet inspection verdicts, fixes GUI response matching, replaces a shell-based GUI update mechanism with a safer threaded downloader, validates imported rules, adds xhost cleanup, and updates related packaging scripts and documentation. ChangesControl Socket Authorization and Fail-Closed Filtering
GUI Update/Install Safety and Rule Import Validation
Packaging Script Fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GUIClient
participant UnixSocket
participant GuiState
participant BastionGroup
GUIClient->>UnixSocket: connect()
UnixSocket->>GuiState: accept + getsockopt(SO_PEERCRED)
alt credentials verified
GuiState->>BastionGroup: peer_is_authorized(peer_uid)
BastionGroup-->>GuiState: authorized true/false
alt authorized
GuiState-->>GUIClient: accept connection
else not authorized
GuiState-->>GUIClient: reject connection
end
else credential lookup failed
GuiState-->>GUIClient: reject connection (fail-closed)
end
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bastion-rs/src/gui.rs (1)
344-392: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPeer-credential fail-closed logic is correct, but
.expect()ontry_clone()can panic and permanently kill the GUI control channel.The authorization/fail-closed logic (lines 361-376) and single-active-GUI enforcement (381-390) correctly implement
#31/#33. However, line 391 usess.try_clone().expect("Failed to clone stream")— iftry_clone()fails (e.g. fd exhaustion), this panics the thread runningrun_socket_server's entire accept loop. Since that loop runs on a single dedicated thread with no supervisor/restart, a single failed clone permanently disables all future GUI connections until the daemon is restarted, even though normal packet filtering keeps running.Notably,
GuiState::set_connection(same file, lines 86-92) already handles this exact failure gracefully viamatch/error!/early-return — the same pattern should be used here instead of.expect().🛡️ Proposed fix to avoid panicking on clone failure
{ let mut state = gui_state.lock(); if state.is_connected() { warn!("Refusing GUI connection: a GUI is already connected"); drop(state); drop(s); continue; } - state.set_connection(s.try_clone().expect("Failed to clone stream")); + match s.try_clone() { + Ok(cloned) => state.set_connection(cloned), + Err(e) => { + error!("Failed to clone stream, refusing GUI connection: {}", e); + drop(state); + drop(s); + continue; + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bastion-rs/src/gui.rs` around lines 344 - 392, The GUI accept loop in run_socket_server should not panic on stream cloning failure; replace the s.try_clone().expect(...) path used before GuiState::set_connection with graceful error handling. Mirror the existing GuiState::set_connection pattern by matching on try_clone(), logging an error or warning, dropping the incoming socket, and continuing the loop so a transient failure does not kill the dedicated accept thread. Keep the peer-credential checks and single-active-GUI logic unchanged, and use the same symbols run_socket_server and GuiState::set_connection to update the right spot.
🧹 Nitpick comments (1)
bastion-rs/src/gui.rs (1)
269-290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
getgrouplist()for supplementary-group membership.Manually scanning
gr_memfromgetgrnam("bastion")only reflects members explicitly listed against that one group entry.getgrouplist()enumerates all groups a user belongs to (following whatever NSS modules — files, LDAP, sssd — are configured) and is the more standard/robust way to check "is this user a member of group X," especially since this same NSS lookup already backsgetgrnam, but going through the user's full group list is less prone to omissions in edge-case NSS configurations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bastion-rs/src/gui.rs` around lines 269 - 290, The supplementary-group check is too narrow because the current membership logic manually scans gr_mem from getgrnam("bastion"), which can miss users resolved through NSS. Update the membership check in the GUI auth/group-validation path to use getgrouplist() for the target user and verify whether the bastion group is present in the returned group list. Keep the existing early false returns for null pointers, but replace the direct gr_mem iteration with the more robust group-enumeration flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bastion-firewall.service`:
- Around line 7-10: Add an ownership marker to the Bastion-installed NFQUEUE
rule before removing it, since the current ExecStopPost cleanup in
bastion-firewall.service can delete unrelated OUTPUT/NEW queue 1 rules. Update
the rule install path in ExecStartPre and mirror the same marker-based matching
in every delete path, including debian/DEBIAN/preinst, debian/DEBIAN/postrm, and
uninstall.sh, so only Bastion’s own rule is targeted.
In `@bastion-gui.py`:
- Around line 520-523: The update error handler in the exception block should
not reference the exception object directly inside the delayed Qt callback,
because the caught variable may be cleared before QTimer.singleShot runs. In the
update install failure path, capture the exception message in a local variable
within the except block and use that captured text in both the print statement
and the QMessageBox.critical callback so the error reporting in the update flow
remains reliable.
- Around line 518-519: The installer call in subprocess.Popen currently relies
on PATH to find pkexec and apt-get and then ignores the return code from
proc.wait(). Update the install flow in bastion-gui.py to use fixed, validated
absolute paths for the privileged commands in the existing installer logic, and
after calling proc.wait() check the exit status and raise or report an error if
the install failed. Keep the change localized around the installer execution
path so the surrounding behavior stays the same.
In `@bastion-rs/start_daemon.sh`:
- Around line 28-29: The NFQUEUE setup in start_daemon.sh can leave an old
OUTPUT rule in place when fail_closed changes, so the daemon keeps using the
previous queue-bypass mode. Update the iptables handling around the OUTPUT chain
to explicitly detect and remove the stale NFQUEUE variant before appending the
current one, using the existing shell logic around the BYPASS variable and the
iptables -C/-A calls. Keep the rule order consistent so the active
--queue-bypass state always matches the current configuration.
In `@bastion/gui_qt.py`:
- Around line 1906-1920: The import validation in gui_qt.py only checks the rule
key shape in the imported_rules loop, so it still accepts invalid ports and
unsafe paths. Update the import path in the rule-loading logic to parse the key
into path and port, reject non-absolute or non-executable paths, and enforce
port values in the valid range 1..65535 or the wildcard *. Keep the existing
QMessageBox.warning flow, and make sure the validation happens before merging
into the rule set using the same imported_rules processing block.
- Around line 1309-1320: The gufw launch path in gui_qt.py can fail after
granting root X access but before the cleanup thread starts, leaving xhost open.
Update the subprocess.Popen(cmd, start_new_session=True) flow to catch launch
failures around the gufw start logic, immediately revoke the temporary xhost
grant in that failure path, and then re-raise the error. Keep the existing
_revoke_xhost cleanup thread for the success path so the grant is removed both
when gufw exits normally and when startup fails.
In `@bastion/gui/dashboard/main_window.py`:
- Around line 933-944: The gufw launch path in main_window.py can fail after the
temporary root xhost grant is applied but before the cleanup thread starts,
leaving access open. Update the subprocess.Popen flow and the surrounding
cleanup logic in the same block so that any exception during launch immediately
revokes the xhost grant before re-raising, while still keeping the existing
_revoke_xhost thread for the successful start case.
In `@debian/DEBIAN/preinst`:
- Around line 25-29: The NFQUEUE cleanup in preinst is too broad because the
iptables deletions in the OUTPUT rule loop match any identical rule, not just
Bastion-owned ones. Update the rule insertion and removal logic in the preinst
script so the Bastion rule is tagged with a unique marker/comment and the
cleanup checks in the same iptables block only delete rules carrying that
marker.
In `@install.sh`:
- Around line 12-16: The fallback in install.sh currently uses ls in the
VERSION-missing path, which can select the wrong bastion-firewall_*_all.deb when
multiple packages exist. Update the DEB_FILE selection logic to avoid parsing ls
and instead use a find-based lookup in the VERSION-absent branch, choosing the
highest-version matching .deb rather than the first alphabetical match. Keep the
change localized to the existing VERSION check and DEB_FILE assignment so the
install flow still uses the same variable.
---
Outside diff comments:
In `@bastion-rs/src/gui.rs`:
- Around line 344-392: The GUI accept loop in run_socket_server should not panic
on stream cloning failure; replace the s.try_clone().expect(...) path used
before GuiState::set_connection with graceful error handling. Mirror the
existing GuiState::set_connection pattern by matching on try_clone(), logging an
error or warning, dropping the incoming socket, and continuing the loop so a
transient failure does not kill the dedicated accept thread. Keep the
peer-credential checks and single-active-GUI logic unchanged, and use the same
symbols run_socket_server and GuiState::set_connection to update the right spot.
---
Nitpick comments:
In `@bastion-rs/src/gui.rs`:
- Around line 269-290: The supplementary-group check is too narrow because the
current membership logic manually scans gr_mem from getgrnam("bastion"), which
can miss users resolved through NSS. Update the membership check in the GUI
auth/group-validation path to use getgrouplist() for the target user and verify
whether the bastion group is present in the returned group list. Keep the
existing early false returns for null pointers, but replace the direct gr_mem
iteration with the more robust group-enumeration flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2011b73-2a27-40e0-a443-9b6cae977c1d
📒 Files selected for processing (15)
README.mdSECURITY.mdbastion-firewall.servicebastion-gui.pybastion-rs/src/config.rsbastion-rs/src/gui.rsbastion-rs/src/main.rsbastion-rs/start_daemon.shbastion/gui/dashboard/main_window.pybastion/gui_qt.pybuild_deb.shcom.bastion.firewall.desktopdebian/DEBIAN/postinstdebian/DEBIAN/preinstinstall.sh
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| except Exception as e: | ||
| print(f"[UPDATE] Failed to install update: {e}") | ||
| QTimer.singleShot(0, lambda: QMessageBox.critical( | ||
| None, "Update Error", f"Failed to install update: {e}")) |
There was a problem hiding this comment.
Suggestion: The lambda scheduled on the Qt event loop closes over exception variable e, but Python clears e after the except block ends. When the callback runs, it can raise a NameError and the error dialog will not be shown. Capture the message string before scheduling the lambda. [incorrect variable usage]
Severity Level: Major ⚠️
- ❌ Update error dialog can crash and not display.
- ⚠️ Users get no GUI feedback on update failures.Steps of Reproduction ✅
1. Start the Qt tray application via `bastion-gui.py` main block (`bastion-gui.py:84-98`),
which constructs `BastionClient` and its tray menu including the update action wired to
`perform_update()` (`bastion-gui.py:11-32,34-82`).
2. Ensure `self.latest_update_version` is set (for example by the existing update-check
thread) so `show_update_notification()` (`bastion-gui.py:11-32`) inserts the "Install
Update" QAction that calls `perform_update()` when selected.
3. Trigger the update by clicking the menu action, causing `perform_update()` to spawn the
`_download_and_install()` thread (`bastion-gui.py:59-82`) which runs the download/install
logic.
4. Induce an exception inside `_download_and_install()` (for example, by breaking network
access so `urllib.request.urlretrieve` fails, or by making `pkexec`/`apt-get` unavailable)
so execution enters the `except Exception as e:` block at `bastion-gui.py:71-74`.
5. Observe that the `QTimer.singleShot(0, lambda: QMessageBox.critical(... f"... {e}"))`
scheduled at `bastion-gui.py:73-74` runs later on the Qt event loop, after leaving the
`except` block; in Python 3 the exception variable `e` is cleared after the `except`, so
the lambda’s reference to `e` can raise a `NameError`/`UnboundLocalError`, preventing the
error dialog from being shown and instead surfacing a runtime error in the event loop.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bastion-gui.py
**Line:** 520:523
**Comment:**
*Incorrect Variable Usage: The lambda scheduled on the Qt event loop closes over exception variable `e`, but Python clears `e` after the `except` block ends. When the callback runs, it can raise a `NameError` and the error dialog will not be shown. Capture the message string before scheduling the lambda.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let user_cstr = std::ffi::CStr::from_ptr(user_name); | ||
| let grp_name = match std::ffi::CString::new("bastion") { | ||
| Ok(n) => n, | ||
| Err(_) => return false, | ||
| }; | ||
| let grp = libc::getgrnam(grp_name.as_ptr()); | ||
| if grp.is_null() { |
There was a problem hiding this comment.
Suggestion: The supplementary-membership loop dereferences members before verifying the pointer itself is non-null. If gr_mem is null for a group entry, this will crash during authorization checks. Check members.is_null() before *members and only dereference inside a guarded loop. [null pointer]
Severity Level: Critical 🚨
- ❌ Control socket authorization can segfault on misconfigured bastion group.
- ❌ Daemon crash terminates firewall control and GUI socket server.Steps of Reproduction ✅
1. Start the Rust daemon, whose `main()` in `bastion-rs/src/main.rs:64-69` spawns the GUI
control socket thread by calling `run_socket_server(gui_state_server, stats_server,
config_server, rules_server)` (`bastion-rs/src/gui.rs:295-304`).
2. From any GUI client (e.g., `bastion-gui.py` or `bastion/gui_qt.py`), connect to the
Unix control socket at `SOCKET_PATH`, causing `run_socket_server`’s accept loop
(`bastion-rs/src/gui.rs:132-203`) to accept a `UnixStream` and, on Unix, call
`getsockopt(... SO_PEERCRED ...)` and then `peer_is_authorized(peer_uid)`
(`gui.rs:152-155,41-84`).
3. Configure the system such that the `bastion` group exists but its `gr_mem` field is
null or otherwise not set to a valid null-terminated array (this matches the libc-level
scenario the comment warns about, and is realistic with misconfigured or empty groups);
`libc::getgrnam` at `gui.rs:70-72` then returns a `grp` where `(*grp).gr_mem` is a null
pointer.
4. When `peer_is_authorized` executes `let mut members = (*grp).gr_mem; while
!(*members).is_null() { ... }` at `bastion-rs/src/gui.rs:74-80`, it dereferences `members`
before checking whether `members` itself is null, triggering undefined behaviour and
typically a segmentation fault that crashes the entire daemon during an authorization
check.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bastion-rs/src/gui.rs
**Line:** 274:280
**Comment:**
*Null Pointer: The supplementary-membership loop dereferences `members` before verifying the pointer itself is non-null. If `gr_mem` is null for a group entry, this will crash during authorization checks. Check `members.is_null()` before `*members` and only dereference inside a guarded loop.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| # Revoke the temporary root display grant once gufw exits so it is | ||
| # not left open indefinitely. | ||
| import threading | ||
|
|
||
| def _revoke_xhost(p=proc): | ||
| try: | ||
| p.wait() | ||
| finally: | ||
| subprocess.run(['xhost', '-si:localuser:root'], capture_output=True) | ||
|
|
||
| threading.Thread(target=_revoke_xhost, daemon=True).start() | ||
| else: |
There was a problem hiding this comment.
Suggestion: The display grant is opened before spawning gufw, but revocation only happens in a thread created after Popen succeeds. If process launch fails, the revoke path is never executed and root display access remains granted. Wrap launch/revoke in a try/finally (or revoke on launch failure) so cleanup is guaranteed. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Root display access may remain granted after launch failure.
- ⚠️ Wayland gufw integration leaves lingering xhost permission.Steps of Reproduction ✅
1. Open the dashboard GUI implemented in `bastion/gui/dashboard/main_window.py`, which
constructs `DashboardWindow` and provides the "Launch gufw" action wired to
`launch_gufw()` (`main_window.py:49-80,949-77`).
2. Run the application in a Wayland session (environment variable
`XDG_SESSION_TYPE=wayland`), and invoke `launch_gufw()`, which checks for `gufw` and calls
`_launch_gufw_process()` (`main_window.py:49-57,915-947`) when installed.
3. Inside `_launch_gufw_process`, on Wayland it first enables root access to the display
via `subprocess.run(['xhost', '+si:localuser:root'], capture_output=True)` and then
constructs the `pkexec env ... gufw` command (`main_window.py:21-33`) before calling `proc
= subprocess.Popen(cmd, start_new_session=True)` at `main_window.py:34`.
4. If `subprocess.Popen` raises (for example because `pkexec` is missing, authentication
fails early, or there is an OS-level resource issue), execution never reaches the
`_revoke_xhost` thread definition and `threading.Thread(...).start()` at
`main_window.py:35-45`, so the compensating `xhost -si:localuser:root` call in
`_revoke_xhost` is never run; the temporary root display grant made in step 3 remains in
effect indefinitely until manually revoked.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bastion/gui/dashboard/main_window.py
**Line:** 934:945
**Comment:**
*Missing Cleanup: The display grant is opened before spawning `gufw`, but revocation only happens in a thread created after `Popen` succeeds. If process launch fails, the revoke path is never executed and root display access remains granted. Wrap launch/revoke in a `try/finally` (or revoke on launch failure) so cleanup is guaranteed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| proc = subprocess.Popen(cmd, start_new_session=True) | ||
| # Revoke the temporary root display grant once gufw exits so it is | ||
| # not left open indefinitely. | ||
| import threading | ||
|
|
||
| def _revoke_xhost(p=proc): | ||
| try: | ||
| p.wait() | ||
| finally: | ||
| subprocess.run(['xhost', '-si:localuser:root'], capture_output=True) | ||
|
|
||
| threading.Thread(target=_revoke_xhost, daemon=True).start() |
There was a problem hiding this comment.
Suggestion: The temporary xhost permission is revoked only from a thread that starts after successful Popen. If launching pkexec env ... gufw throws, cleanup never runs and root keeps display access. Ensure revocation executes on both success and failure paths. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Root display access may linger after gufw launch error.
- ⚠️ Wayland Qt launcher leaves xhost grant on failure.Steps of Reproduction ✅
1. Launch the Qt-based dashboard implemented in `bastion/gui_qt.py`, where
`DashboardWindow.launch_gufw()` (`gui_qt.py:45-76`) provides a "Launch gufw" action that
calls `_launch_gufw_process()` when `gufw` is installed.
2. Run under Wayland (environment `XDG_SESSION_TYPE=wayland`), then trigger
`launch_gufw()` so `_launch_gufw_process()` executes the Wayland branch
(`gui_qt.py:12-24`), calling `subprocess.run(['xhost', '+si:localuser:root'],
capture_output=True)` to grant root display access.
3. `_launch_gufw_process()` builds the `pkexec env ... gufw` command and calls `proc =
subprocess.Popen(cmd, start_new_session=True)` at `bastion/gui_qt.py:30`, which may raise
if `pkexec` is missing, if there is an OS-level resource limit, or if another early
failure occurs before the child process starts.
4. Because the revocation logic `_revoke_xhost` and its background thread are only defined
and started after `Popen` succeeds (`gui_qt.py:31-41`), any exception thrown by `Popen`
prevents the `xhost -si:localuser:root` cleanup from running, leaving the temporary root
display grant active beyond the intended lifetime of the gufw session.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** bastion/gui_qt.py
**Line:** 1309:1320
**Comment:**
*Missing Cleanup: The temporary `xhost` permission is revoked only from a thread that starts after successful `Popen`. If launching `pkexec env ... gufw` throws, cleanup never runs and root keeps display access. Ensure revocation executes on both success and failure paths.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
🤖 Augment PR SummarySummary: Security-hardening pass across the daemon, GUI, and packaging based on the 2026-07-02 audit. Changes:
Technical Notes: The hardening relies on both filesystem permissions and runtime credential checks; 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| return false; | ||
| } | ||
| let mut members = (*grp).gr_mem; | ||
| while !(*members).is_null() { |
There was a problem hiding this comment.
bastion-rs/src/gui.rs:284 members can be null if (*grp).gr_mem is null (e.g., empty member list), and while !(*members).is_null() would dereference a null pointer (UB/crash). Consider guarding members before dereferencing so an empty/NULL list fails closed safely.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| let _ = std::fs::set_permissions(SOCKET_PATH, std::fs::Permissions::from_mode(0o666)); | ||
| // 0o660 + root:bastion ownership: only root and members of the bastion | ||
| // group can connect. The kernel enforces this at connect() time. | ||
| let _ = std::fs::set_permissions(SOCKET_PATH, std::fs::Permissions::from_mode(0o660)); |
There was a problem hiding this comment.
bastion-rs/src/gui.rs:330 set_permissions errors are ignored, so the control socket may keep its default mode/ownership (potentially weakening the intended connect-time restriction or breaking GUI access). Consider handling failures explicitly (at least logging/aborting) since this is part of the hardening.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| [Service] | ||
| Type=simple | ||
| ExecStartPre=/bin/sh -c 'iptables -C OUTPUT -m state --state NEW -j NFQUEUE --queue-num 1 --queue-bypass 2>/dev/null || iptables -I OUTPUT -m state --state NEW -j NFQUEUE --queue-num 1 --queue-bypass' | ||
| ExecStartPre=/bin/sh -c 'BYPASS=--queue-bypass; grep -qs "\"fail_closed\"[[:space:]]*:[[:space:]]*true" /etc/bastion/config.json && BYPASS=; iptables -C OUTPUT -m state --state NEW -j NFQUEUE --queue-num 1 $$BYPASS 2>/dev/null || iptables -I OUTPUT -m state --state NEW -j NFQUEUE --queue-num 1 $$BYPASS' |
There was a problem hiding this comment.
bastion-firewall.service:7 The grep check for "fail_closed": true only matches when true is on the same line as the key; a pretty-printed JSON with a newline after : would silently fall back to --queue-bypass. Other locations where this applies: bastion-rs/start_daemon.sh:25.
Severity: medium
Other Locations
bastion-rs/start_daemon.sh:25
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| # "path:*", and values must be booleans. This prevents a malformed | ||
| # or malicious rules file from injecting junk into the rule set. | ||
| import re | ||
| key_pattern = re.compile(r'^.+:(\d+|\*)$') |
There was a problem hiding this comment.
bastion/gui_qt.py:1910 Import validation accepts any digit port (\d+) without checking range, so keys like app:99999 pass and may later cause unexpected behavior when the daemon/UI assumes a valid TCP/UDP port. Consider rejecting ports outside 1–65535 during import to keep persisted rules consistent.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| # argv list, never a shell string: no version data is | ||
| # interpreted by a shell. | ||
| proc = subprocess.Popen(['pkexec', 'apt-get', 'install', '-y', deb_path]) | ||
| proc.wait() |
There was a problem hiding this comment.
bastion-gui.py:519 proc.wait() ignores the installer exit status, so an apt-get failure (non-zero) won't surface to the user unless it raises an exception. Consider checking proc.returncode and showing an error when install fails.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
…iew) - peer_is_authorized: check members.is_null() before dereferencing gr_mem, guarding each loop iteration to avoid UB/segfault on a null member list. - run_socket_server: replace try_clone().expect() with a match that logs and continues so fd exhaustion no longer panics the accept thread and permanently disables GUI connections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review) - Use absolute /usr/bin/pkexec and /usr/bin/apt-get; verify each exists and is executable before invoking, and raise on a non-zero apt-get exit code. - Capture the exception message into a local before the delayed QMessageBox lambda so the error dialog no longer raises NameError (Python 3 clears the except var after the block). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ules (#37 review) - Wrap the gufw Popen in try/except that revokes the temporary xhost +si:localuser:root grant and re-raises, so a failed launch never leaves root display access open (gui_qt.py and dashboard/main_window.py). - Import-rules validation: parse each key into path:port, reject non-absolute paths and ports outside 1..65535 (allowing '*'), before merging. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e cleanup (#37 review) Insert the queue-1 NFQUEUE rule with -m comment --comment "bastion-firewall" (service ExecStartPre, start_daemon.sh) and require the same comment in every -C/-D check/delete path (service ExecStopPost, preinst, postrm, uninstall.sh, build_rpm.sh) so cleanup only ever targets Bastion's own rule, never an unrelated firewall's identical queue-1 rule. start_daemon.sh now also deletes both variants (with and without --queue-bypass) before appending so a fail_closed flip takes effect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review) The VERSION-absent fallback used ls | head -n1, which picks the alphabetically-first .deb. Use find -printf | sort -V | tail -n1 to select the highest version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d detection (#37 review) - run_socket_server: treat set_permissions(0o660) and chown-to-root:bastion failures as fatal — error! and remove the socket then return, instead of serving a weakly-permissioned control socket. - fail_closed detection: strip whitespace/newlines with tr before grep in both the service ExecStartPre and start_daemon.sh, so a pretty-printed config with a newline after the colon still disables --queue-bypass (was fail-open). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
User description
Fixes from the 2026-07-02 security audit. Non-destructive; verified with
cargo check(clean) andcargo test config(3/3).Critical / High
0o660+chown root:bastion; newpeer_is_authorized()(root orbastiongroup member) viagetgrnam/getpwuid; getsockopt failure now rejects (was fail-open); non-authorized uid rejected. Closes the world-connectable, unauthenticated rule-mutation hole.^\d+\.\d+\.\d+$; download viaurllib+pkexec apt-get install -y <file>as argv — no morepkexec bash -c '...'.ask_guiusespending_responses.remove(&request.request_id)instead of an arbitrary entry.fail_closedconfig flag (default false) — drops uninspectable packets in enforcement mode and omits--queue-bypass; trade-off documented in SECURITY.md.Mechanical hardening (#36)
preinst exact
-C/-Drule removal (no more-F OUTPUT/-P ACCEPT); postinst fallback includesmode+#[serde(default)];xhostgrant revoked after gufw; import-rules validation; build_deb heredocs expand${VERSION}/$(date)(metainfo and changelog); daemon version viaenv!(CARGO_PKG_VERSION);install.shDEB_FILE from VERSION;.desktopExec fix; docs updated.Closes #31, #32, #33, #34, #35. Refs #36 (tracking — systemd sandboxing, group-membership policy, schema convergence, etc. deferred, see checklist).
🤖 Generated with Claude Code
https://claude.ai/code/session_012EvwLaYAzmzyB6G5fw5YQJ
Generated by Claude Code
CodeAnt-AI Description
Harden control access, updater installs, and firewall rule handling
What Changed
bastiongroup members, rejects connections when peer identity cannot be verified, and allows only one GUI connection at a time.fail_closedis enabled; by default, traffic still continues if the daemon cannot inspect it.Impact
✅ Fewer unauthorized firewall changes✅ Lower risk of update command injection✅ Safer firewall recovery after crashes or upgrades💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores