STOR/RETR in PDS context creates wrong dataset + runs allocation under STC identity + S130 ABEND

[mgrossmann]

Three related bugs in the MVS dataset store/retrieve path (src/ftpd#mvs.c), all triggered by uploading a file into a PDS.

Reproduction

cd cntl
250 The working directory "IBMUSER.CNTL" is a partitioned data set
put iefbr14.jcl
125 Storing data set IBMUSER.CNTL.IEFBR14.JCL      ← WRONG: should be IBMUSER.CNTL(IEFBR14)

On TK5 this escalates to a crash:

RAKF0005 INVALID ATTEMPT TO ACCESS RESOURCE
RAKF000A  FTPD    ,FTPD    ,DATASET ,HERC01.TEST.CNTL.IEF
worker ... ended ABEND S130

Bug 1 — PDS member name derivation. In resolve_dsn() (~line 163) the PDS-member branch is guarded by !strchr(arg, '.'). A client filename like iefbr14.jcl contains a dot, so it falls through to the relative-prefix branch and the extension becomes extra DSN qualifiers → IBMUSER.CNTL.IEFBR14.JCL instead of IBMUSER.CNTL(IEFBR14).

Bug 2 — allocation runs under STC identity, not the logged-in user. In the STOR handler, check_dataset_access() correctly authorizes with sess->acee (the logged-in user), and fopen() is wrapped in racf_set_acee(sess->acee). But __dsalcf() (SVC 99 allocation, ~line 1510) runs BETWEEN them with NO ACEE switch — so new-dataset allocation executes under the STC identity (FTPD). This is why the RAKF denial shows FTPD,FTPD rather than the logged-in user. Allocation must run under the user's ACEE, exactly like fopen().

Bug 3 — S130 ABEND after RAKF denial. When authorization or allocation fails, the handler continues into fopen()/OPEN with an invalid DCB state, causing ABEND S130. Failure paths must reply and return immediately, never reaching OPEN.

Fix

1. Member name derivation in resolve_dsn() (src/ftpd#mvs.c ~line 163). In PDS context an unquoted argument is always a member reference; a dot is a filename extension, not a qualifier separator. Replace the current else if branch:

} else if (sess->in_pds && arg[0] != '\'' && !strchr(arg, '(')) {
    /* Inside a PDS: unquoted argument is a MEMBER reference.
    ** Derive an MVS member name from the client filename:
    **   strip leading path, strip extension (from first '.'),
    **   uppercase, truncate to 8 chars.
    */
    char member[9];
    const char *base = arg;
    const char *slash;
    int m = 0;

    slash = strrchr(base, '/');
    if (slash) base = slash + 1;

    for (i = 0; base[i] && base[i] != '.' && m < 8; i++)
        member[m++] = (char)toupper((unsigned char)base[i]);
    member[m] = '\0';

    if (m == 0)
        return -1;   /* no usable member name, e.g. ".foo" */

    len = snprintf(buf, bufsz, "%s(%s)", sess->pds_name, member);
    if (len >= bufsz)
        return -1;

    return 0;        /* member name already clean; skip generic uppercase/dot-strip */
}

Keep the existing quoted-absolute branch and the relative-prefix (non-PDS) branch unchanged. This one function is shared by RETR, STOR, and DELE, so all three get the correct member handling.

2. Wrap __dsalcf()/__dsfree() in the user ACEE in the STOR handler. The allocation must run under the same identity as the subsequent fopen():

if (/* dataset does not exist, needs allocation */) {
    ACEE *oldacee = sess->acee ? racf_set_acee(sess->acee) : NULL;
    rc = __dsalcf(ddname, "%s", opts);
    if (rc == 0)
        __dsfree(ddname);
    if (sess->acee) racf_set_acee(oldacee);   /* restore STC identity */

    if (rc != 0) {
        ftpd_log(LOG_ERROR, "STOR: __dsalcf failed rc=%d", rc);
        ftpd_session_reply(sess, 550, "Dataset allocation failed");
        return;                                /* do NOT fall through to fopen */
    }
}

Make sure the ACEE is always restored, including on the failure path (don't leak the user ACEE as the STC's active environment).

3. Guarantee early return on every failure path (S130 guard). Audit the STOR/RETR/DELE handlers so that:

• check_dataset_access(...) != 0 → reply 550 Access denied and return immediately.
• allocation failure → reply 550 and return.
• fopen() returning NULL → reply 550/550 Cannot open and return; never continue to read/write.

No failure may reach an OPEN or an I/O loop with a NULL/invalid FILE *. The S130 is OPEN running on a failed allocation state — these early returns eliminate it.

Test matrix (please verify all)

cd 'IBMUSER.CNTL'          (PDS context)
put iefbr14.jcl            → 250, creates IBMUSER.CNTL(IEFBR14), RAKF shows IBMUSER not FTPD
get IEFBR14                → downloads the member
get iefbr14.jcl            → downloads IBMUSER.CNTL(IEFBR14) (extension stripped)
dir                        → listing shows member IEFBR14
delete IEFBR14             → removes the member

cd 'IBMUSER.'              (prefix mode, not a PDS)
put test.data             → creates sequential dataset IBMUSER.TEST.DATA (unchanged)

# Negative / crash-regression:
cd 'SYS1.PARMLIB'          (a PDS the user may not have ALTER on)
put foo.jcl               → 550 Access denied, NO S130 ABEND, STC stays up

Confirm on both MVS/CE (IBMUSER) and TK5 (HERC01) that the RAKF requestor/user in any denial is the logged-in user, not FTPD, and that a denial never crashes the worker.

Ref: resolve_dsn() PDS member branch, ACEE consistency for __dsalcf vs fopen, TK5 ABEND S130

[mgrossmann]

Follow-up: quoted DSN(member) syntax bypasses member sanitization (FileZilla)

The original fix handled the relative PDS case (STOR iefbr14.jcl in PDS context) correctly — CLI ftp works. But FileZilla sends the member explicitly in parentheses:

CWD 'IBMUSER.CNTL'
TYPE I
STOR 'IBMUSER.CNTL(iefbr14.jcl)'
125 Storing data set 'IBMUSER.CNTL(iefbr14.jcl)'
250 Transfer completed successfully.

This created a member literally named IEFBR14. (trailing dot). That argument takes resolve_dsn()'s quoted-absolute branch verbatim, and split_member() then copied the parenthesized text with only an 8-char truncation (IEFBR14.JCL → IEFBR14.) — never uppercased/extension-stripped. The broken member then can't be deleted, because any client input sanitizes the dot away and no longer matches.

Fix (added to PR #62, same branch): centralized member-name cleanup in one helper sanitize_member() (strip extension from the first ., uppercase, truncate to 8) and applied it at both extraction points:

• resolve_dsn() PDS-member branch — replaced the inline loop with the helper.
• split_member() — the raw parenthesized text is now sanitized, so STOR 'IBMUSER.CNTL(iefbr14.jcl)' creates member IEFBR14, matching the CLI result.

Deviation from the proposed patch (flagging for visibility): the follow-up suggested removing the !strchr(arg, '(') guard from resolve_dsn()'s PDS branch. I kept it. FileZilla's argument is quoted, so it never reaches that branch anyway — the fix lives entirely in split_member(). Removing the guard would only change the bare DSN(member) form, and for the worse: sanitize_member() would stop at the first qualifier's dot and derive member IBMUSER from IBMUSER.CNTL(iefbr14.jcl). Today that form fails cleanly (nonexistent doubled-prefix DSN → 550); dropping the guard would silently write to a valid wrong member. So the guard stays, and (-containing args continue to be resolved as a DSN and cleaned by split_member() — which matches the prose intent ("explicit (member) form handled via split_member()").

Leftover cleanup: any member already created with a trailing dot (e.g. IEFBR14. from testing) is unreachable via FTP after this fix — DELE input is sanitized too, so it can never match the physical name. This is inherent, not a regression; that leftover needs manual IEHPROGM/ISPF cleanup on the target. No valid member can have a trailing dot again after this fix.

On TYPE I (not a bug): FileZilla sent TYPE I (binary) before STOR, so no ASCII→EBCDIC translation occurred — correct for image type. The CLI client used TYPE A and got translation. No change needed.

Verification: host build clean (make modules, -Wall -Werror); sanitize_member() and the split_member() path verified in isolation across the follow-up matrix (IEFBR14.JCL → IEFBR14, MyProg.cbl → MYPROG, IEFBR14. → IEFBR14, idempotent IEFBR14 → IEFBR14, .foo/empty rejected). On-MVS matrix (CLI relative + FileZilla quoted STOR/DELE, mixed case) to be run on a live target.

[mgrossmann]

Follow-up 2: LIST path runs __listpd() under the STC identity → ABEND S913 on protected PDS

The #61 fix covered RETR/STOR/DELE/MKD/RMD but missed the LIST path, which has the same identity + authorization gap. __listpd() OPENs the PDS directory with BPAM under whatever ACEE is active; ftpd_mvs_list() never switched to sess->acee, so listing was authorized as the STC (FTPD), not the logged-in user — both a crash source and a security gap.

Reproduction (TK5):

cd 'SYS1.SECURE.CNTL'
ls
RAKF000A  FTPD    ,FTPD    ,DATASET ,SYS1.SECURE.CNTL
IEC150I 913-38,IFG0194C,FTPD,FTPD,...,SYS1.SECURE.CNTL
worker ... ended ABEND S913

Fix (added to PR #62, same branch): applied the established RETR/STOR pattern to the PDS-member branch of ftpd_mvs_list():

• check_dataset_access(sess, prefix, RACF_ATTR_READ) before __listpd(), returning immediately on denial (550 already sent) — the OPEN is never reached for a PDS the user can't read (same mechanism that eliminated the STOR S130).
• __listpd() wrapped in racf_set_acee(sess->acee)/restore so the OPEN-time RACHECK evaluates against the logged-in user.

Both halves are needed: the pre-check alone wouldn't suffice — a PDS where the user is allowed but FTPD is denied would still S913 under FTPD without the ACEE wrap. Together, allow/deny is evaluated against one consistent identity and no residual S913 path survives.

Scope. __listpd() is the only call in the LIST path that OPENs a dataset — the entry formatters render already-fetched list entries, and __listds()/__locate()/__dscbdv() are catalog/VTOC operations with no OPEN. The dataset-listing branch's __listds() is left unwrapped (a prefix like SYS1. isn't a single RACF DATASET resource, so there's no per-dataset READ check to make); a code comment records this and flags it to confirm on TK5.

CWD unchanged (by design). ftpd_mvs_is_pds() uses only __locate() (SVC 26) + __dscbdv() (SVC 27) — no OPEN — so cd 'SYS1.SECURE.CNTL' still returns a 250 PDS context and the denial correctly surfaces on the subsequent ls. This matches z/OS, where CWD sets context without reading contents.

This unblocks the original #61 S130 verification. The S913 on cd/ls of a protected PDS was preventing testers from even reaching the STOR test, so this LIST fix must be deployed before the original #61 STOR/RETR matrix can be run end-to-end.

On-MVS test matrix (TK5 HERC01 + MVS/CE IBMUSER)

# Regression check — MUST still succeed, authorized AS THE USER (run this first):
cd 'SYS1.MACLIB'         → 250
ls                       → member listing renders (user has READ) — NOT a 550

# Security fix — previously leaked a listing under FTPD; now a clean deny:
cd 'SYS1.SECURE.CNTL'    → 250 (PDS context, no I/O) — no crash
ls                       → 550 Access denied, NO S913 ABEND, STC stays up
                           RAKF denial (if logged) shows the user (HERC01), not FTPD

The positive control (SYS1.MACLIB) is the key regression check: LIST now calls check_dataset_access() where it never did before, so confirm a PDS the user can read still lists (it should — RETR checks READ on the same name via the same racf_auth, so anyone who can get a member passes the list check).

Verification so far: host build clean (make modules, -Wall -Werror). No host-testable pure logic here (MVS ACEE/RAKF integration) — the matrix above must be run on a live target.