Stop server-provided values from reaching a shell or escaping /dsc/seafile
Commands were joined into a string and passed to "su -c", so a login shell parsed every interpolated value. A library name is chosen on the server, and "sync -d /dsc/seafile/<name>" put it straight into that string: a library named 'Docs; touch /tmp/pwned; #' ran as an arbitrary command. Confirmed against the built image before and after this change. Every command is now an argument list executed by "runuser --", which execs directly with no shell. runuser -u sets the same HOME and a PATH that still finds the seaf-cli wrapper, so the daemon environment is unchanged. The password no longer needs its surrounding quotes, which also fixes passwords containing quotes, dollar signs, and backticks. Library names are untrusted, so dsc/paths.py reduces each one to a single path component and proves the result stays under the libraries directory with symlinks resolved. Empty, ".", "..", and names containing a separator are skipped with a warning; names that would collide are suffixed with their library ID rather than sharing a directory. Tests cover quotes, command substitutions, separators, newlines, absolute paths, parent traversal, Unicode, and colliding names. They run in a separate CI job so application failures stay distinguishable from packaging failures.
This commit is contained in:
1 parent
50ede0b2f9
commit
c15763d9f8
12 files changed
+478
-38
No files matched your search
+21
-12
@@ -37,15 +37,19 @@ Acceptance criteria:
|
||||
|
||||
### Remove shell and path injection risks
|
||||
|
||||
- [ ] Pass subprocess arguments as arrays instead of joining values into shell
|
||||
command strings in `dsc/client.py` and `dsc/misc.py`.
|
||||
- [ ] Replace `su -c` with a mechanism such as `runuser --` that does not need a
|
||||
shell.
|
||||
- [ ] Treat server-provided library names as untrusted input.
|
||||
- [ ] Resolve every target directory and prove that it remains under
|
||||
`/dsc/seafile` before creating or writing it.
|
||||
- [ ] Define how duplicate, absolute, empty, and traversal-style library names
|
||||
are handled.
|
||||
- [x] Pass subprocess arguments as arrays instead of joining values into shell
|
||||
command strings in `dsc/client.py` and `dsc/misc.py`. The password is no
|
||||
longer wrapped in quotes to survive a shell, which also fixes passwords
|
||||
containing quotes, `$`, and backticks.
|
||||
- [x] Replace `su -c` with a mechanism such as `runuser --` that does not need a
|
||||
shell. `runuser -u` sets the same `HOME` and a `PATH` that still finds the
|
||||
`seaf-cli` wrapper, so the daemon environment is unchanged.
|
||||
- [x] Treat server-provided library names as untrusted input.
|
||||
- [x] Resolve every target directory and prove that it remains under
|
||||
`/dsc/seafile` before creating or writing it. Symlinks are resolved first,
|
||||
so an existing symlink in the libraries directory cannot redirect a write.
|
||||
- [x] Define how duplicate, absolute, empty, and traversal-style library names
|
||||
are handled. See `dsc/paths.py` and the "Some notes" section of the README.
|
||||
|
||||
Acceptance criteria:
|
||||
|
||||
@@ -88,6 +92,8 @@ Acceptance criteria:
|
||||
|
||||
- [ ] Check every important subprocess return code.
|
||||
- [ ] Add timeouts to daemon startup, shutdown, and readiness polling.
|
||||
Confirmed: `start_daemon` and `stop_daemon` poll `while True` with no bound,
|
||||
so a daemon that never changes state hangs the container silently.
|
||||
- [ ] Handle `SIGTERM` and `SIGINT` and stop the daemon in `finally`.
|
||||
- [ ] Return a nonzero container exit code on configuration or daemon failure.
|
||||
- [ ] Produce actionable errors instead of waiting forever.
|
||||
@@ -101,13 +107,16 @@ Acceptance criteria:
|
||||
### Add real tests
|
||||
|
||||
- [ ] Add unit tests for configuration parsing, command construction, library
|
||||
resolution, path validation, retries, and status parsing.
|
||||
resolution, path validation, retries, and status parsing. Command
|
||||
construction and path validation are covered in `tests/`; configuration
|
||||
parsing, retries, and status parsing are not.
|
||||
- [ ] Add regression tests for externally reported password and deletion
|
||||
threshold issues.
|
||||
- [ ] Add a container smoke test for imports, installed CLI version, startup,
|
||||
and shutdown using controlled fakes.
|
||||
- [ ] Keep image construction as a separate CI job so packaging failures are
|
||||
distinguishable from application failures.
|
||||
- [x] Keep image construction as a separate CI job so packaging failures are
|
||||
distinguishable from application failures. The `unit-test` job does not
|
||||
depend on the image build.
|
||||
|
||||
Acceptance criteria:
|
||||
|
||||
|
||||
Reference in new issue
Block a user