Repository navigation
Conversation
bd7f241 to
19ff53f
Compare
e7a24ae to
13588e9
Compare
0902961 to
8442ad7
Compare
8442ad7 to
29e1a8d
Compare
4a6389d to
5a42342
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 5a42342 "fix: preserve backslashes in path escaping" —
fix:is not theluci-app-veracrypt:scope of the file it touches, and it only fixes code added in 29e1a8d. Squash it into 29e1a8d to keep the single-commit history the PR body describes.
Generated by Claude Code
5a42342 to
1594e47
Compare
d0c4171 to
2c1f1e3
Compare
BKPepe
left a comment
There was a problem hiding this comment.
Request changes
I did a full pass over the current PR, including the rpcd backend, ACL, UCI handling, job lifecycle and the LuCI view.
There are still several blocking issues that need to be addressed before this is ready:
-
The root
runRPC accepts arbitrary absolute filesystem paths.valid_path()only rejects malformed paths; it does not constrainvolume,mountpoint,backup_fileor keyfile creation to the application's namespace. This means an account with theluci-app-veracryptACL can create/overwrite files outside/mntand mount volumes on arbitrary system paths such as/etc,/overlayor/root. The/mnt/<name>/...allowlist used bymkdir/rmshould be applied consistently to all filesystem operations. -
listdirexposes arbitrary root filesystem enumeration. It is in the read ACL, but the backend allows paths such as/etc,/root,/overlayand/www. The browser should be restricted to the same explicitly allowed namespace, or the ACL design needs to be reconsidered. -
The job implementation is not concurrency-safe. All requests share
/tmp/luci-veracrypt-job*, including PID, log, volume, slot and the interactive FIFO. Concurrent requests can overwrite each other's state, causingjob_abort,job_answerorjob_dismountto operate on another request's job. Jobs need an explicit per-job ID and isolated state. -
vc_list_field()matches slot OR volume instead of matching both when both are supplied. A stale slot can therefore select a different mounted volume than the requested path. This is particularly dangerous for fsck/unmount operations. -
veracrypt --listparsing is whitespace-based in both shell and JavaScript.set -- $restandsplit(/\s+/)break paths containing spaces, even though the path validation explicitly allows them. This affects volume/device/mountpoint detection and consequently fsck/unmount behavior. -
createcan report success after the follow-up mount failed. The mount exit status is collected, but$JOB.rcis unconditionally set to0afterwards. -
The favorite keyfile handling is broken for multiple/token keyfiles.
saveFavorite()passes the complete keyfile value throughsafeAbsPath(), which only accepts one absolute POSIX path. Such keyfiles are silently omitted from the saved favorite.
There are also several secondary concerns around simulating VeraCrypt's interactive prompt protocol for --change/--backup-headers/--restore-headers, the overly broad pkg_install privilege in the application ACL, and the robustness of the UCI section-name parsing.
Given that this code runs as the rpcd/root boundary and performs destructive storage operations, I would not merge the PR until the issues above are resolved and the corresponding paths/job lifecycle are covered by tests.
6e02851 to
89bbe81
Compare
|
Pushed 002dc9f. It addresses the Copilot and openwrt-ai comments above, plus a fifth strict review of the backend:
Tests: the backend suite passes 197/197, with new t-97. ESLint and the formality check are clean. A full run on an RT-AX53U (mipsel_24kc, OpenWrt 25.12.5) with real veracrypt passes 42/42 through ubus, including the new checks (single link after hidden create; rm/restore of a mounted container refused; wrong-slot unmount refused). |
002dc9f to
4b6a06f
Compare
|
Pushed 4b6a06f:
The backend suite passes 201/201. A run on an RT-AX53U (mipsel_24kc, 25.12.5) with real veracrypt passes 43/43, including properties of a mounted volume requested with a wrong slot. |
4b6a06f to
1cfc984
Compare
|
Pushed 1cfc984 with fixes from another strict review of the app:
Results:
|
Web UI for the console veracrypt package (packages feed): openwrt/packages#30597 Not bundled into veracrypt; apk add luci-app-veracrypt depends on +veracrypt. Calls veracrypt --text only. The current password is passed on stdin (--stdin), never --password. Other secrets (new password, hidden volume password, token PIN) are answered to veracrypt's prompts one at a time through a private FIFO; values with line breaks are refused, and secrets are kept out of the environment. All paths are confined to /mnt/<name>/... (or a whole disk device) and used in resolved form. Each job has its own random id and state directory under /var/run, and package installation needs the separate luci-app-veracrypt-pkg ACL grant. File names, paths and veracrypt output are always shown as text, never parsed as HTML. tests/run.sh exercises the rpcd backend in an OpenWrt rootfs. Signed-off-by: Ville Takio <ville+git@takio.fi>
1cfc984 to
004c93e
Compare
|
Pushed 004c93e with fixes from another full audit of the app, plus the VC_LIST nit above:
Results:
|
BKPepe
left a comment
There was a problem hiding this comment.
Thanks for the work on this. I can see you put a lot of effort into handling edge cases and security concerns here.
However, after going through the current implementation, I think the backend has become much more complicated than it needs to be. There is a lot of custom logic around path_has_metas(), new_job(), create_pinned() / pin_recover(), cached veracrypt --list state, prompt handling, etc. Some of these mechanisms are quite elaborate for what should essentially be a thin LuCI wrapper around veracrypt --text.
I am also concerned that this complexity is making the code harder to review and maintain. We've already found several correctness and security issues in these additional layers during review, which is a good indication that the implementation is becoming too complex.
I think it would be better to step back and simplify the design rather than continuing to add more handling for individual edge cases. In particular, I'd avoid building a custom security/filesystem/job framework in shell unless there is a clear need for it.
Could you please have another look at the overall approach and see which parts can be removed or significantly simplified? I think this would make the PR much easier to review and maintain in the long term.
|
Thanks, @BKPepe. Agreed: the backend grew a job, path and prompt framework in shell, and that is where the review findings came from. I'll rewrite the implementation to be much smaller and simpler:
I'll make another PR when the rewrite is done and run-tested on the router. |
|
I think this "lite" version is just watered-down mickey-mouse software. I would go with the full version. I'm quite sure it will never be upgraded and if it is, I'll probably be the one doing it anyway. |
|
The reduced version is now a separate package, luci-app-veracrypt-lite: #9119. The two PRs are independent, so either or both can be merged; both depend on the veracrypt package in openwrt/packages#30597. |
A small web UI for the console veracrypt package (packages feed): openwrt/packages#30597 It is an alternative to luci-app-veracrypt (openwrt#9069): mount, unmount and create volumes (container files or whole free disks), create keyfiles, and favorites in /etc/config/veracrypt, which both apps share. Change password, header backup/restore, hidden volumes, security tokens, fsck and package installation are only in luci-app-veracrypt. The two can be installed side by side. The backend is a ucode rpcd plugin. Mount and create take minutes on a router, so they run in the background through a small helper that starts veracrypt --text --non-interactive --stdin with the password on stdin, never in argv or logs; one job at a time. Volumes, keyfiles and mount directories must lie under /mnt on a mounted disk, also after resolving symlinks. tests/run.sh exercises the plugin and the helper in an OpenWrt rootfs. Signed-off-by: Ville Takio <ville+git@takio.fi>
004c93e to
04083d3
Compare
A small web UI for the console veracrypt package (packages feed): openwrt/packages#30597 It is an alternative to luci-app-veracrypt (openwrt#9069): mount, unmount and create volumes (container files or whole free disks), create keyfiles, and favorites in /etc/config/veracrypt, which both apps share and create with a uci-defaults script when it is missing. Change password, header backup/restore, hidden volumes, security tokens, fsck and package installation are only in luci-app-veracrypt. The two can be installed side by side. The backend is a ucode rpcd plugin. Mount and create take minutes on a router, so they run in the background through a small helper that starts veracrypt --text --non-interactive --stdin with the password on stdin, never in argv or logs; one job at a time. Volumes, keyfiles and mount directories must lie under /mnt on a mounted disk, also after resolving symlinks. tests/run.sh exercises the plugin and the helper in an OpenWrt rootfs. Signed-off-by: Ville Takio <ville+git@takio.fi>
04083d3 to
bba2be0
Compare
A small web UI for the console veracrypt package (packages feed): openwrt/packages#30597 It is an alternative to luci-app-veracrypt (openwrt#9069): mount, unmount and create volumes (container files or whole free disks), create keyfiles, and favorites in /etc/config/luci-veracrypt, which both apps share and create with a uci-defaults script when it is missing. Change password, header backup/restore, hidden volumes, security tokens, fsck and package installation are only in luci-app-veracrypt. The two can be installed side by side. The backend is a ucode rpcd plugin. Mount and create take minutes on a router, so they run in the background through a small helper that starts veracrypt --text --non-interactive --stdin with the password on stdin, never in argv or logs; one job at a time. Volumes, keyfiles and mount directories must lie under /mnt on a mounted disk, also after resolving symlinks. tests/run.sh exercises the plugin and the helper in an OpenWrt rootfs. Signed-off-by: Ville Takio <ville+git@takio.fi>
004c93e to
5f84ab2
Compare
BKPepe
left a comment
There was a problem hiding this comment.
The backend is doing a lot of privileged filesystem and process management in one large shell script. I'd like to see this simplified and split into smaller, testable pieces, preferably using ucode for the RPC backend.
Please reconsider the hardcoded nokernelcrypto option and make the backend choice explicit, with FUSE as a sensible default and kernel crypto as an opt-in when its dependencies are available. The path restrictions should remain security-conscious, but the UI should support explicitly configured keyfiles outside /mnt without granting unrestricted filesystem access. I'd also remove file deletion from the file picker and keep destructive operations separate.
5f84ab2 to
004c93e
Compare
|
@BKPepe Thanks. I'll come back to this once openwrt/packages#30597 is merged, since this app depends on its config. Then: an explicit FUSE/kernel-crypto choice (FUSE default), configured keyfiles outside |
Pull request details
Description
Optional LuCI UI for console VeraCrypt. Not bundled into the
veracryptpackage.apk add luci-app-veracryptpulls+veracryptveracrypt --textonly (no VeraCrypt GUI toolkit)--stdin), never--password/mnt/<name>/..., symlinks resolved; volumes may also be whole disks/partitions. Token libraries must be under/usr/libluci-app-veracrypt-pkgACL grant/etc/config/veracrypt, which theveracryptpackage ships; favorites are itsvolumesections. Favorites are mounted on request with the password entered then, never at boot, and passwords are not storedtests/run.sh <openwrt-rootfs.tar.gz>runs the rpcd backend against a fakeveracryptinside an OpenWrt rootfs (unprivileged namespace chroot): 121 checks--mount-options=nokernelcrypto(FUSE3 only; dmsetup/lvm2 is not a dependency)This replaces closed PR #9031 with a clean single-commit history.
Maintainer (preferred)
@flatstik
Tested on
OpenWrt version: OpenWrt 25.12.5 (r33051-f5dae5ece4), ASUS RT-AX53U, ramips/mt7621
LuCI version: LuCI openwrt-25.12 branch (26.253.65060~7f05feb)
Web browser(s): Firefox 150.0.1 (64-bit)
The reworked backend was verified with
tests/run.shin the OpenWrt 25.12 x86_64 rootfs, and on the device above over ubus (tools, status, listdir/listdev confinement, a mount job on a USB disk; the password never appears inps).Checklist