Re: [PATCH v4] nfsd: enforce export security for NFSv4 filehandles and stateids

[VeroFess <coderzeng@gmail.com>]

Hi Chuck,

Thank you very much for offering to help develop the patches. I would
like to take you up on that: I am happy to run follow-up tests and
confirm the resulting series, but I am not in a position to develop
the split patches myself. I ran isolated tests to answer your
questions; results are inline below.

> The v4 revision of your patch appears to combine the original fix
> with several unrelated changes to SECINFO, READDIR, LOOKUP, and
> stateid processing. [...] Let's split the single patch into a series.

Agreed that it should be split.

> 1. The need_wrongsec_check() change by itself, with
>    "Fixes: 68d93184352f". [...] Please confirm that with your
>    reproducer on a kernel that has only this change.

Confirmed. I built the v4 parent plus only the need_wrongsec_check()
change, with no other NFSD change. Reusing a live krb5p OPEN stateid
over an AUTH_SYS connection, PUTFH; SAVEFH; READ and PUTFH; SAVEFH;
WRITE both returned NFS4ERR_WRONGSEC at PUTFH, the file was unchanged,
and the krb5p owner could still read it. That change alone is enough
for the reported sequence.

> 2. The check_nfsd_access() call in nfsd4_verify_copy(), which covers
>    both COPY and CLONE. [...] the source handle can reach COPY or
>    CLONE unchecked even after patch 1.

Reproduced on the patch-1-only kernel: AUTH_SYS direct READ of the
krb5p source returned WRONGSEC, but PUTFH; SAVEFH; PUTFH; COPY
completed and wrote the source bytes to the destination. Adding only
the nfsd4_verify_copy() check made COPY return NFS4ERR_ACCESS and left
the destination unchanged. CLONE is NFS4ERR_NOTSUPP on this ext4
fixture with or without the check, so I could not demonstrate a
reflink; its justification is the shared source-check path with COPY.
So this is a separate concern from patch 1.

> 3. Anything else, one change per patch, each with its own rationale
>    and only the Fixes: tags that match its code. These do not need
>    to go to stable.

Understood; I have kept each finding below as its own separate
concern.

> COPY_NOTIFY does not carry OP_HANDLES_WRONGSEC, so in PUTFH;
> COPY_NOTIFY the PUTFH already gets the deferred check_nfsd_access()
> call in nfsd4_proc_compound(). [...] How can READ be denied while
> COPY_NOTIFY succeeds? [...] Unless there is another path, I would
> drop the nfs4_check_file() and COPY_NOTIFY hunks.

You are right. That result came from an intermediate tree, before the
selected-export check on LOOKUP; both compounds used PUTROOTFH; LOOKUP,
and only READ took the nfs4_check_file() path. I rebuilt v4 with just
those two guards removed: with a retained stateid after the export
changed to krb5p, READ and COPY_NOTIFY were both rejected with
NFS4ERR_WRONGSEC before the operation, in the direct, SAVEFH, LOOKUP,
and RESTOREFH paths. I found no PUTFH; COPY_NOTIFY path that patch 1
misses, so those two hunks and the COPY_NOTIFY claim do not look
needed.

> With this change, a client using any flavor can learn whether a name
> exists in a directory on a sec=krb5p export. RFC 8881 Section
> 2.6.3.1.1.5 allows that, and your patch description should clarify
> that the code behavior is changed to conform with the spec.

Confirmed. With directory traversal permission, AUTH_SYS SECINFO on a
krb5p export returned OK for an existing name and NOENT for a missing
one; without traversal permission both returned NFS4ERR_ACCESS. I
agree the description should state this is an intentional,
spec-conforming part of security negotiation.

> Also, the new check_xprtsec_policy() call in nfsd_lookup_dentry()
> returns nfserr_wrongsec from SECINFO when it crosses into an export
> with a different xprtsec= policy. Is that not the NFS4ERR_WRONGSEC
> return from SECINFO that the description says is forbidden?

It is, and that is a real inconsistency in v4. Plaintext SECINFO
crossing into an xprtsec=tls child returned NFS4ERR_WRONGSEC on
NFSv4.0/4.1/4.2. I also tested a variant that maps a transport-policy
mismatch to NFS4ERR_ACCESS (RFC 8881 Section 18.29.3) at the starting
and selected-export checks: SECINFO then returned ACCESS instead of
WRONGSEC, while ordinary LOOKUP still returned WRONGSEC. That test
used a plaintext connection, not TLS. The unqualified "forbidden"
wording in my description is wrong either way.

> On a pseudo-root export, nfsd4_encode_entry4_fattr() always crosses
> into child exports. [...] Could the encoder fall back to returning
> the mountpoint's own attributes instead?

I tried that. The unrestricted fallback made size-only READDIR
succeed, but when the client requested FILEHANDLE it received the
covered mountpoint's handle and could then list the hidden underlay
(confirmed with a sentinel placed under the mount). So the
unrestricted fallback is not safe. A restricted variant that falls
back only when FILEHANDLE is not requested did behave: size-only
READDIR succeeded; a FILEHANDLE request still returned ACCESS; an
rdattr_error request reported WRONGSEC in the child entry; direct
LOOKUP stayed WRONGSEC.

> The new check_nfsd_access() call in nfsd_lookup() fixes a mount
> crossing that never checks the child export. The CREATE paths do the
> same crossing [...] If this check goes in, it probably belongs in
> nfsd_cross_mnt() so that every caller gets it.

Agreed, and the CREATE gap is real. On v4, AUTH_SYS NFSv2 CREATE and
NFSv3 CREATE UNCHECKED returned a filehandle from the stricter child
export (a bind-mounted single-file export); NFSv4.0/4.1/4.2 OPEN
UNCHECKED already returned WRONGSEC at OPEN. Moving the selected-export
check into nfsd_cross_mnt_common() made NFSv2/NFSv3 CREATE return
ACCESS while ordinary CREATE in the same parent still succeeded, so
the check does look like it belongs on the common mount-crossing path,
with the SECINFO exception preserved.

> Longer term, I would rather not keep adding check_nfsd_access()
> calls to individual operations. A filehandle whose security check
> PUTFH deferred could instead carry a "check pending" state [...]
> That can come later, it does not need to hold up these fixes.

That design makes sense to me, and I agree it need not block the
split.

Please go ahead and take things from here. I will confirm that the
resulting series addresses the issue and no longer reproduces, and run
any other tests you would like.

Thanks,
VeroFess