fix: ParseScpCommand leaks scpBasePathDynamic on repeated -t/-f

[MarkAtwood]

Bug

ParseScpCommand() (src/wolfscp.c) allocates ssh->scpBasePathDynamic in both
the -t and -f cases and assigns straight to the struct member without
freeing a previous allocation. The option scan does not consume the copied
remainder of the command, so a peer command that repeats the direction option
(scp -t a -t b, scp -f a -f b) re-enters the case, allocates again, and
overwrites the pointer, leaking the earlier buffer. The command string is
peer-controlled (wolfSSH_GetSessionCommand). The single teardown free
(internal.c) releases only the final pointer, so every prior allocation leaks
for the connection lifetime.

The cost is quadratic in the command length: both the allocation size
(cmdSz + WOLFSSH_MAX_FILENAME) and the number of repeats scale with the
peer-supplied command. Driving ParseScpCommand() with scp followed by N
repetitions of -f, peak RSS growth:

repeats	command size	before	after
1 000	3 003 B	+3.4 MB	+2.3 MB
10 000	30 003 B	+195 MB	+7.4 MB

One authenticated exec channel request of ~30 KB costs the server ~195 MB
held for the connection lifetime.

Fix

Free and clear ssh->scpBasePathDynamic before each (re)allocation.

Three things fell out of review and are included:

1. ssh->scpBasePath aliases that buffer. It is a separate const char*
   pointing at the same allocation, and it is only reassigned inside the
   idx + 2 < cmdSz branch -- that is, only when the option actually carries a
   path. Freeing without clearing the alias leaves it dangling, and
   ParseScpCommand() still returns WS_SUCCESS with a valid scpDirection,
   so the caller proceeds into DoScpSource()/DoScpSink() and hands the
   dangling pointer to the scp callback. Confirmed under AddressSanitizer with
   the command scp -f /tmp -f:

   ERROR: AddressSanitizer: heap-use-after-free
     freed by:      ParseScpCommand wolfscp.c:1577
     previously allocated by: ParseScpCommand wolfscp.c:1581
   

   Note this does not reproduce on a normal build -- the allocator hands the
   freed block straight back, so the two pointers compare equal and the read
   silently succeeds. It needs a quarantining allocator to surface.

2. A direction option with no path leaves scpBasePath NULL. This is
   pre-existing, not introduced here: on master, scp -t and scp -f already
   return WS_SUCCESS with a NULL base path, which then reaches
   WCHDIR(ssh->fs, basePath) in the default recv callback and any application
   callback registered through wolfSSH_SetScpRecvCallback(). Clearing the
   alias widens the set of commands that reach it, so it is rejected after the
   parse loop with WS_SCP_CMD_E.

   This is a behaviour change beyond the leak. A bare scp -t or scp -f
   with no path now fails where it previously returned success. That is
   deliberate; a direction option with no path is a malformed scp command.

3. scpBasePathSz is kept in lockstep with the pointer, cleared on free and
   set only after the allocation succeeds, matching what wolfSSH_free()
   (internal.c) already does.

Verification

• Built --enable-all against wolfSSL master; compiles clean.
• make check: 10 PASS / 1 SKIP / 0 FAIL, including scripts/scp.test.
• ASAN harness driving ParseScpCommand() directly: use-after-free reproduced
  before the alias clear, clean after. scp -f /tmp -f now yields a NULL base
  path and WS_SCP_CMD_E; scp -f /tmp -f /tmp2 correctly ends on /tmp2;
  scp -f /tmp is unchanged.

No test in tests/ currently covers ParseScpCommand(). Adding a
wolfSSH_TestParseScpCommand wrapper alongside the existing wolfSSH_TestScp*
helpers would close that gap, and is not done here.

Reported by static analysis (Fenrir finding F-6813).

[Copilot]

Pull request overview

Fixes a memory leak in ParseScpCommand() (SCP command parsing) where repeated -t/-f options could overwrite ssh->scpBasePathDynamic without freeing the prior allocation, causing per-connection lifetime leaks on peer-controlled commands.

Changes:

• Free ssh->scpBasePathDynamic before re-allocating it in the -t parsing branch.
• Free ssh->scpBasePathDynamic before re-allocating it in the -f parsing branch.

Comments suppressed due to low confidence (2)

src/wolfscp.c:1526

• After freeing scpBasePathDynamic, ssh->scpBasePath can still point at the freed buffer. If the subsequent WMALLOC fails (or if the option has no trailing path and the later assignment isn’t hit), the session may retain a dangling scpBasePath pointer. Clear scpBasePath when freeing and set it to the newly allocated buffer immediately after allocation/memset so it always points at valid storage.

This issue also appears on line 1547 of the same file.

                        if (ssh->scpBasePathDynamic != NULL) {
                            WFREE(ssh->scpBasePathDynamic, ssh->ctx->heap,
                                    DYNTYPE_BUFFER);
                            ssh->scpBasePathDynamic = NULL;
                        }
                        ssh->scpBasePathDynamic = (char*)WMALLOC(
                                ssh->scpBasePathSz,
                                ssh->ctx->heap, DYNTYPE_BUFFER);
                        if (ssh->scpBasePathDynamic == NULL) {
                            return WS_MEMORY_E;
                        }
                        WMEMSET(ssh->scpBasePathDynamic, 0, ssh->scpBasePathSz);

src/wolfscp.c:1558

• Same as the -t case: after freeing scpBasePathDynamic, ssh->scpBasePath may still reference freed memory. Clearing scpBasePath on free and setting it to the newly allocated buffer avoids leaving a dangling pointer behind if allocation fails or if later code paths don’t reassign scpBasePath.

                        if (ssh->scpBasePathDynamic != NULL) {
                            WFREE(ssh->scpBasePathDynamic, ssh->ctx->heap,
                                    DYNTYPE_BUFFER);
                            ssh->scpBasePathDynamic = NULL;
                        }
                        ssh->scpBasePathDynamic = (char*)WMALLOC(
                                ssh->scpBasePathSz,
                                ssh->ctx->heap, DYNTYPE_BUFFER);
                        if (ssh->scpBasePathDynamic == NULL) {
                            return WS_MEMORY_E;
                        }
                        WMEMSET(ssh->scpBasePathDynamic, 0, ssh->scpBasePathSz);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.