I would love to agree with everything you've said, but I thoroughly
explained the issue/problematic behavior. It doesn't take more than a
glance at the patch to see the work done in nfsd_path.c is on wrappers
around various function call signatures. In the patch commit message it
is clearly stated that I am simply implementing these methods using
helper function nfsd_run_task(). How thorough can an explanation for
abstracting away repetitive conditional logic be?
> And don't just say "there is a security vulnerability". Explain that it
> is because the that real_path happens before that root is prepended.
The issue still has nothing to do with realpath being called before
concatenation of the rootdir, as placing it after is arguably worse and
not a fix. My mention of it came due to concluding you assumed an
alternate ordering of steps (concat -> realpath), hence why you did not
understand the issue.
Cheers,
Christopher Bii
NeilBrown wrote:
On Wed, 08 Jan 2025, Christopher Bii wrote:
It all depends what the desired behavior is. When a rootdir is set, do
we want any exports to possibly fall outside the rootdir path? If no, it
is beneficial to use this approach since resolving paths within chrooted
env will fail if the path does not exist within the chroot. Allowing for
a "sandboxing" mechanism of sort. But with this approach you have
proposed, the issue might arise where someone with access to an nfs
export can create a symlink to say "/". In that scenario the kernel will
end up exporting the system root. Which can be bad. You could of course
do as you have stated and check if it is within the boundaries of the
rootdir, but it would result to the same thing. I think the chrooted
thread that can be repurposed is the best approach here.
The majority of the patch is simplifying what I believed to have been an
unmaintainable approach of spawning tasks within the worker thread is
created to run chrooted.
I'm not at all familiar with the "work queue" and nfs_path.c and chroot
code so I cannot comment on whether the current approach is
unmaintainable. Maybe it is.
But all these details need to appear in the patches that you send.
Don't just tell us what you are changing, tell us why. Say that is is
unmaintainable and give some reason for us to believe you - a reason
that we can check by looking at the code, and then see that your patch
makes it unmaintainable.
And don't just say "there is a security vulnerability". Explain that it
is because the that real_path happens before that root is prepended.
All of that needs to be clear in the patches so that we can review them
properly now, and so that when we look back at them in 2 years because
some issue arose, we can remind our self what they were for and
understand how that all relates to the new issue.
Maybe the code in the patches is already fine. But the explanation in
the patches also needs to be good before we can approve them.
Thanks!
NeilBrown
