Add TFTP server and DHCP bootfile support

[troglobit]

Description

Add support for acting as a TFTP server so an Infix unit can hand out boot images, configs, and firmware to downstream devices on an isolated or provisioning network

Checklist

Tick relevant boxes, this PR is-a or has-a:

• Bugfix
  • Regression tests
  • ChangeLog updates (for next release)
• Feature
  • YANG model change => revision updated?
  • Regression tests added?
  • ChangeLog updates (for next release)
  • Documentation added?
• Test changes
  • Checked in changed Readme.adoc (make test-spec)
  • Added new test to group Readme.adoc and yaml file
• Code style update (formatting, renaming)
• Refactoring (please detail in commit messages)
• Build related changes
• Documentation content changes
  • ChangeLog updated (for major changes)
• Other (please describe):

[mattiaswal]

Security audit

Audited head b88ccde against main. Two findings should be fixed before merge, one deserves a doc note, the rest is minor.

Privilege model

Admins are in wheel, which has passwordless sudo for everything, so for admins the CLI path whitelists are guard rails, not a boundary. The boundaries that matter:

• Non-admin CLI users. Any user whose shell is not /bin/false lands in the klish group and can talk to klishd, which runs as root. The doas wrapper only elevates wheel members, so a doas -u user action is a silent no-op for a non-admin.
• Configuration writers. NACM is itself configuration, so who may write which leaf is deployment-defined. Only full permit-all groups are added to wheel, so a config writer must be assumed to have no root. No config write may yield more than the model expresses. With the factory defaults the gap is concrete: write-default is permit and operator is denied only the password, keystore and truststore paths.
• Network side of TFTP. dnsmasq runs as root (-u root), serves any world-readable file below the root, follows symlinks, no authentication.

Findings

#	Severity	Finding	Location
1	High	TFTP root leaf admits newlines, written verbatim into a config dnsmasq reads as root	infix-services.yang:347, services.c:801
2	Medium	remove runs as root from klishd while the PR widens its allowed paths	infix.c:250, util.c:104
3	Medium (doc)	Configs copied into the TFTP root become world-readable and are served unauthenticated	util.c:106, doc/tftp.md:61
4	Low	Whitelist bypass via a symlinked parent when the destination does not exist yet	util.c:217
5	Low	Operational file list disagrees with what dnsmasq serves, breaks on odd file names	infix_services.py:22
6	Low, pre-existing	dir lists any directory as root	infix.xml:273

1. TFTP root leaf lets a config writer run commands as root

The pattern blocks .. and dotfiles, but [^/] matches newline and every other control character:

pattern '(/var/lib/tftpboot|/media/[^/.][^/]*)(/[^/.][^/]*)*';

tftp_change() writes the value verbatim into /etc/dnsmasq.d/tftp.conf and touches the finit service so dnsmasq restarts and re-reads it. dnsmasq runs as root and is built with script support. A root of /var/lib/tftpboot/x + newline + dhcp-script=/tmp/x produces:

enable-tftp
tftp-root=/var/lib/tftpboot/x
dhcp-script=/tmp/x
tftp-no-fail

and /tmp/x runs as root on the next lease event. Any dnsmasq directive works. Anyone NACM lets write /infix-services:tftp/root becomes root; with the factory defaults that includes every operator.

The existing DHCP leaves already guard against this (string uses [^\p{Cc}]*, names are inet:domain-name, hex is an octet string, and the new boot file leaf excludes \p{Cc} and commas). The root leaf is the one place this PR drops that guard.

Fix: exclude control characters in every component, e.g. (/var/lib/tftpboot|/media/[^/.\p{Cc}][^/\p{Cc}]*)(/[^/.\p{Cc}][^/\p{Cc}]*)*, and have tftp_change() refuse a root containing a newline as a second line of defence.

2. remove runs as root while the PR widens its reach

infix_erase() execs erase -s directly from klishd (root). Copy, rename and path completion in the same file go through doas -u <user>. Since doas is a no-op for non-wheel users, copy and rename fail closed for a non-admin, but erase does not.

The PR extends the allowed roots in util.c from /cfg, /media, $HOME to also cover /var/lib, /var/log, /log, /tmp, /var/tmp. A non-admin CLI user can therefore delete, as root, any file or empty directory under those trees: startup-config, TFTP images, logs, the container store piece by piece. The realpath check in cfg_adjust() holds for erase since the target must exist, so it is confined to those trees, but root ignores every Unix permission inside them. Pre-existing bug, the wider whitelist is what makes it bite.

Fix: run erase via doas -u or the existing run_as_user() helper, like infix_rename().

3. Copying a config into the TFTP root publishes it

Files under /var/lib get 0664 from the location table, dnsmasq serves anything world-readable, TFTP has no auth and listens on all interfaces unless interface is set. copy startup-config /var/lib/tftpboot/ hands password hashes, Wi-Fi PSKs, RADIUS/SNMP secrets and authorized keys to the LAN. doc/tftp.md "Per-Client Directories" actively suggests configuration files there.

Side effect: copy now creates files in /tmp, /var/tmp, /var/log as 0664 instead of 0660.

Fix: IMPORTANT note in doc/tftp.md and doc/dhcp.md that everything in the root is public and Infix configs carry secrets; recommend setting interface. Consider keeping /tmp, /var/tmp, /var/log at 0660.

4. Whitelist bypass via a symlinked parent

In cfg_adjust() a destination whose final component does not exist fails realpath with ENOENT and skips the resolved-path check. /var/lib/tftpboot is 2775 root:wheel, so ln -s /etc /var/lib/tftpboot/e; copy running-config /var/lib/tftpboot/e/x writes outside the whitelist. Runs as the user, so Unix perms still apply. files -c already realpaths the parent; do the same here.

5. Operational file list vs what is served

tftp_files() uses find -type f -perm -004, which skips symlinks while dnsmasq follows them, so a symlink to /etc/passwd in the root is served but never listed, contradicting the doc. A file name with a newline breaks line.split(" ", 2) and the whole infix-services operational subtree disappears. Use NUL separators; either -xtype f or document that symlinks are served but not listed.

6. dir lists any directory as root

The script action runs under klishd as root, so dir /etc/ssh works for any CLI user. Pre-existing; the quoting fix here is good and there is no injection (klish does not re-evaluate parameter values). Path completion for the same command already goes through doas, so the listing could too.

Reviewed and fine

CI label gating (labels need triage rights, expression context only); the dhcp-boot leaf patterns; the tag_prefix buffer (max 127-byte tag, 160-byte buffer); files -c completion (absolute only, rejects .., realpaths the dir, filters through the whitelist, runs as the user, fails closed for non-wheel); the startup-config removal warning; rename; the bash completions; the TFTP root pattern otherwise (.., dotfiles and /var/lib/tftpbootX rejected). The klish bump to 5880300 is an upstream hash change I did not verify.

[mattiaswal]

I think 1 and 2 is scary in the security audit, fix them, everything else look fine.

[troglobit]

I think 1 and 2 is scary in the security audit, fix them, everything else look fine.

Agreed, thanks for the awesome review! 🙇‍♂️

[troglobit]

One correction to the privilege model, since it changes how we fix finding 2.

doas -u user is not a no-op for non-wheel users when the action is a plugin symbol. klishd never drops privileges, the setgid()/setuid() in ktpd_session.c sits inside an #if 0, so a sym="foo@infix" action runs in klishd's own environment, where LOGNAME=root. Only the script plugin sets USER/LOGNAME to the session user, and only for sym="script" actions. Root is a member of wheel, so the id -nG "$LOGNAME" | grep -qw wheel test in doas passes for every CLI user, and sudo then drops to the right one.

So copy, rename and files -c already run as the logged-in user for non-admins too. They do not fail closed, they simply work, but only by accident of klishd's environment.

For that reason remove now goes through the existing run_as_user() instead of doas: it drops privileges unconditionally, whatever LOGNAME says. It also sets USER/LOGNAME/HOME for the child, which matters because the whitelist in util.c reads $HOME.

The same reasoning applies to finding 6, which is in this round as well: dir is a script action, and there LOGNAME is the CLI user, so doas would fail closed for non-admins. It is a plugin action now, through run_as_user().

Findings 1-5 are folded into their respective commits, 6 is a separate one. For finding 5 I kept the field check rather than NUL separated records, busybox stat has no option for it.

[troglobit]

@mattiaswal fixed, all of them, with the previously noted exceptions.

[mattiaswal]

👍