user-src code review: missing FD_CLOEXEC + minor cleanup items

[cpsource]

Code review of user-src/ — findings for discussion

Reviewed the full contents of user-src/ on current master, with the
code-review-cpsource skill's 20-section C checklist. Five novel
findings plus one finding that's already being addressed by open
PR #20. Summary table first, then each finding, then a self-review
correction where PR #20 caught something I didn't.

Upstream status before reviewing

Queried gh on 2026-04-23:

• 22 PRs total (6 open, 15 merged, 1 closed); 0 issues.
• PR #20 (open,
  @MarkAtwood) already addresses one of the findings (netlink
  key-length validation).
• Keyword searches for CLOEXEC, SOCK_CLOEXEC, fopen, memzero,
  base64, fork exec, fd leak, parse_public_key returned no
  relevant PRs or issues. The other five findings below appear novel.

Findings

#	Severity	File(s)	Summary	Status
1	Major	config.c, setconf.c, ipc-uapi-unix.h, netlink.h, ipc-openbsd.h	Missing O_CLOEXEC/SOCK_CLOEXEC on fd-creating calls	Novel
2	Major	ipc-linux.h	No length validation on netlink-returned key attrs	Covered by PR #20
3	Minor	pubkey.c	'\0' termination at wrong buffer offset	Novel
4	Minor	encoding.c	wg_from_base64 silently accepts trailing non-aligned bytes	Novel
5	Minor	config.c	parse_public_key uses memset, siblings use memzero_explicit	Novel
6	Minor	genkey.c, pubkey.c, others	Negative wolfCrypt error code returned as CLI exit status	Novel

---

Finding 1 — Major: missing O_CLOEXEC / SOCK_CLOEXEC on fd-creating calls

Same bug class as wolfSSL issue #9753
(where PR #10162 is
the upstream fix). In multithreaded hosts or library-embedding callers
that fork()+exec(), these descriptors leak to children. Some carry
private-key material.

File	Line	Call
config.c	145	f = fopen(path, "r"); — private/preshared key file
setconf.c	124	config_input = fopen(argv[2], "r"); — full config
ipc-uapi-unix.h	44, 83	socket(AF_UNIX, SOCK_STREAM, 0) — UAPI socket
netlink.h	505	socket(AF_NETLINK, SOCK_RAW | flags, bus) — flags==0 at every mnl_socket_open caller
ipc-openbsd.h	28	socket(AF_INET, SOCK_DGRAM, 0) — OpenBSD ioctl socket

netlink.h:505 is the high-leverage site: every mnlg_socket_open() in
ipc-linux.h (kernel_get_*, kernel_set_*, kernel_generate_privkey,
kernel_derive_pubkey, kernel_generate_psk) routes through it with
flags=0. One edit at the mnl_socket_open(bus) wrapper (or the inner
__mnl_socket_open) covers all five kernel-ops sites.

Fix sketch:

/* config.c:145, setconf.c:124 */
- f = fopen(path, "r");
+ f = fopen(path, "re");   /* "e" = O_CLOEXEC, glibc/Linux/BSD */

/* ipc-uapi-unix.h:44, 83 */
- fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0);

/* netlink.h:514 */
- return __mnl_socket_open(bus, 0);
+ return __mnl_socket_open(bus, SOCK_CLOEXEC);

/* ipc-openbsd.h:28 — same SOCK_CLOEXEC bit-or */

Regression test pattern: snapshot /proc/self/fd before/after each
call, assert fcntl(fd, F_GETFD) & FD_CLOEXEC. Model from wolfSSL
issue #9753 reproducer.

Finding 2 — superseded by PR #20

ipc-linux.h's parse_privkey, parse_pubkey, parse_psk
previously used mnl_attr_get_payload_len(attr) as both the allocation
size and the copy length without checking against WG_*_KEY_LEN. PR
#20 adds exactly the symbolic size check I would have recommended. No
further action needed from this review.

Finding 3 — Minor: '\0' termination at wrong buffer offset

pubkey.c:39 and pubkey.c:127:

char privkey_base64[WG_BASE64_LEN(WG_KEY_LEN_MAX)];   /* 89 bytes */
...
fread(privkey_base64, 1, WG_BASE64_LEN(WG_PRIVATE_KEY_LEN) - 1, stdin);
                        /* writes 43 bytes */
privkey_base64[sizeof(privkey_base64) - 1] = '\0';   /* writes at offset 88 */

sizeof(privkey_base64) is WG_BASE64_LEN(WG_KEY_LEN_MAX) = 89, but
the read length is WG_BASE64_LEN(WG_PRIVATE_KEY_LEN) - 1 = 43.
Setting privkey_base64[88] = '\0' leaves indices 43..87 as
uninitialised stack memory.

Not a current bug (downstream uses explicit length), but misleading
and hazardous for future strlen/printf additions. Either delete
the line or write at the right offset:

- privkey_base64[sizeof(privkey_base64) - 1] = '\0';
+ privkey_base64[WG_BASE64_LEN(WG_PRIVATE_KEY_LEN) - 1] = '\0';

Finding 4 — Minor: wg_from_base64 ignores trailing non-aligned bytes

encoding.c:154, 198:

while (inLen > 3) {
    /* consume 4 chars */
    inLen -= 4;
}
return (outLen == i);

If the caller passes inLen % 4 != 0, the remaining 1-3 bytes are
silently discarded. Return success is keyed only on the produced
output matching outLen. Input "AAAA" + "x" decodes to 3 bytes and
returns true for outLen == 3 — the trailing "x" is accepted.

Non-critical in normal config-file use (the tokenizer strips
whitespace first) but a fuzz-surface gap. Adding an
if (inLen != 0 && inLen != 4) return false; tail check — or
rejecting non-multiple-of-4 up front — closes it.

Finding 5 — Minor: parse_public_key uses memset, siblings use memzero_explicit

config.c:122 vs config.c:112, 132:

/* parse_public_key */           memset(key, 0, WG_PUBLIC_KEY_LEN);
/* parse_private_key */    memzero_explicit(key, WG_PRIVATE_KEY_LEN);
/* parse_preshared_key */  memzero_explicit(key, WG_SYMMETRIC_KEY_LEN);

Public keys aren't secret, so memset is functionally fine, but the
inconsistency invites accidental copy-paste from parse_public_key
into a future sensitive-buffer context where the compiler-optimised
memset gets elided. One-line homogenisation:

- memset(key, 0, WG_PUBLIC_KEY_LEN);
+ memzero_explicit(key, WG_PUBLIC_KEY_LEN);

Finding 6 — Minor: negative wolfCrypt error returned as CLI exit status

genkey.c:48, pubkey.c:56, others: ret can be a wolfCrypt error
(e.g. WC_KEY_SIZE_E = -181). Returned via main, the shell
truncates to the low 8 bits: -181 & 0xff = 75. Scripts testing $?
see an unexpected positive value.

Convention elsewhere in the tree is return 0 for success and
return 1 for error. Normalise by returning ret ? 1 : 0; from the
cleanup/out: blocks.

Inherited from upstream wireguard-tools; fix ripples through several
files. Minor.

---

Self-review correction

PR #20 also fixes a bug this review missed: in
kernel_generate_privkey, kernel_derive_pubkey, and
kernel_generate_psk, the out: cleanup block called
mnlg_socket_close(nlg) but did not set nlg = NULL; afterward.
Because each of these functions also has a goto try_again loop on
EINTR, a second-pass failure where the retry's mnlg_socket_open()
itself fails leaves the stale closed pointer in nlg; any later
cleanup that re-checks if (nlg) would double-close.

I missed this because my pattern library only covered single-pass
cleanup, not the retry-loop variant where the cleanup block is
re-entered after goto try_again. Updated the review methodology to
cover it — see cpsource/code-review-cpsource commit
fae0bf4:
"c.md: add Pattern 4 — retry loops that don't NULL after close/free",
plus a matching entry in the §3 Resource Management checklist.

Credit to @MarkAtwood / PR #20 for catching it.

---

Test coverage observation

make check is scan-build --maxloop 100 --view make wg-fips — a
Clang static-analyzer run, not a functional test suite. No
genkey→pubkey round-trip test, no base64 known-answer tests, no
CLOEXEC assertions. Given that the tree handles private-key material
at a kernel/user-space interface, at least a minimal behavioural
harness would pay for itself.

Proposal:

• test-encoding — round-trip wg_{to,from}_{base64,hex} across all
  key sizes, plus KATs from a reference base64 implementation.
• test-genkey — wg-fips genkey | wg-fips pubkey → assert output is
  well-formed base64 of WG_BASE64_LEN(WG_PUBLIC_KEY_LEN) bytes, 100
  iterations.
• test-cloexec — the /proc/self/fd snapshot pattern referenced in
  Finding 1.

---

Asks

1. Review PR fix: validate key lengths from kernel reply and fix try_again loop cleanup #20 with an eye toward merging — it closes Finding 20251007-RHEL8V10 #2
   and the retry-loop bug above.
2. If Finding 1 (CLOEXEC sweep) is agreeable, I'm happy to submit it
   as a separate PR with the regression test described.
3. Findings 3–6 are small; they could be bundled into a single
   cleanup PR if you'd prefer a single commit, or I can leave them
   for the tree's regular cleanup cadence.

Full per-section walk-through (20-section C/C++ checklist output) is
in user-src/README-code-review-user-src.md in my local tree;
happy to push that to a branch if useful.