The file permissions on the fdinfo dir from were changed from
S_IRUSR|S_IXUSR to S_IRUGO|S_IXUGO, and a PTRACE_MODE_READ check was
added for opening the fdinfo files [1]. However, the ptrace permission
check was not added to the directory, allowing anyone to get the open FD
numbers by reading the fdinfo directory.
Add the missing ptrace permission check for opening the fdinfo directory.
The check is also added for readdir, lseek in the case that
an unprivileged process inherits an open FD to the fdinfo dir after an
exec.
For the same reason, similar checks are added for fdinfo files which
previously only checked the ptrace permission in open.
[1] https://lkml.kernel.org/r/[email protected]
Fixes: 7bc3fa0172a4 ("procfs: allow reading fdinfo with PTRACE_MODE_READ")
Signed-off-by: Kalesh Singh <[email protected]>
---
fs/proc/fd.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 172c86270b31..aea59e243bae 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -72,7 +72,7 @@ static int seq_show(struct seq_file *m, void *v)
return 0;
}
-static int seq_fdinfo_open(struct inode *inode, struct file *file)
+static int proc_fdinfo_access_allowed(struct inode *inode)
{
bool allowed = false;
struct task_struct *task = get_proc_task(inode);
@@ -86,13 +86,44 @@ static int seq_fdinfo_open(struct inode *inode, struct file *file)
if (!allowed)
return -EACCES;
+ return 0;
+}
+
+static int seq_fdinfo_open(struct inode *inode, struct file *file)
+{
+ int ret = proc_fdinfo_access_allowed(inode);
+
+ if (ret)
+ return ret;
+
return single_open(file, seq_show, inode);
}
+static ssize_t seq_fdinfo_read(struct file *file, char __user *buf, size_t size,
+ loff_t *ppos)
+{
+ int ret = proc_fdinfo_access_allowed(file_inode(file));
+
+ if (ret)
+ return ret;
+
+ return seq_read(file, buf, size, ppos);
+}
+
+static loff_t seq_fdinfo_lseek(struct file *file, loff_t offset, int whence)
+{
+ int ret = proc_fdinfo_access_allowed(file_inode(file));
+
+ if (ret)
+ return ret;
+
+ return seq_lseek(file, offset, whence);
+}
+
static const struct file_operations proc_fdinfo_file_operations = {
.open = seq_fdinfo_open,
- .read = seq_read,
- .llseek = seq_lseek,
+ .read = seq_fdinfo_read,
+ .llseek = seq_fdinfo_lseek,
.release = single_release,
};
@@ -344,17 +375,43 @@ proc_lookupfdinfo(struct inode *dir, struct dentry *dentry, unsigned int flags)
static int proc_readfdinfo(struct file *file, struct dir_context *ctx)
{
+ int ret = proc_fdinfo_access_allowed(file_inode(file));
+
+ if (ret)
+ return ret;
+
return proc_readfd_common(file, ctx,
proc_fdinfo_instantiate);
}
+static loff_t proc_llseek_fdinfo(struct file *file, loff_t offset, int whence)
+{
+ int ret = proc_fdinfo_access_allowed(file_inode(file));
+
+ if (ret)
+ return ret;
+
+ return generic_file_llseek(file, offset, whence);
+}
+
+static int proc_open_fdinfo(struct inode *inode, struct file *file)
+{
+ int ret = proc_fdinfo_access_allowed(inode);
+
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
const struct inode_operations proc_fdinfo_inode_operations = {
.lookup = proc_lookupfdinfo,
.setattr = proc_setattr,
};
const struct file_operations proc_fdinfo_operations = {
+ .open = proc_open_fdinfo,
.read = generic_read_dir,
.iterate_shared = proc_readfdinfo,
- .llseek = generic_file_llseek,
+ .llseek = proc_llseek_fdinfo,
};
base-commit: e9f1cbc0c4114880090c7a578117d3b9cf184ad4
--
2.32.0.93.g670b81a890-goog
On Thu, Jul 8, 2021 at 8:57 AM Kalesh Singh <[email protected]> wrote:
>
> The file permissions on the fdinfo dir from were changed from
> S_IRUSR|S_IXUSR to S_IRUGO|S_IXUGO, and a PTRACE_MODE_READ check was
> added for opening the fdinfo files [1]. However, the ptrace permission
> check was not added to the directory, allowing anyone to get the open FD
> numbers by reading the fdinfo directory.
>
> Add the missing ptrace permission check for opening the fdinfo directory.
The more I look at this, the more I feel like we should look at
instead changing how "get_proc_task()" works.
That's one of the core functions for /proc, and I wonder if we
couldn't just make it refuse to look up a task that has gone through a
suid execve() since the proc inode was opened.
I don't think it's basically ever ok to open something for one thread,
and then use it after the thread has gone through a suid thing.
In fact, I wonder if we could make it even stricter, and go "any exec
at all", but I think a suid exec might be the minimum we should do.
Then the logic really becomes very simple: we did the permission
checks at open time (like UNIX permission checks should be done), and
"get_proc_task()" basically verifies that "yeah, that open-time
decision is still valid".
Wouldn't that make a lot of sense?
Linus
On Sat, Jul 10, 2021 at 11:21 AM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Jul 8, 2021 at 8:57 AM Kalesh Singh <[email protected]> wrote:
> >
> > The file permissions on the fdinfo dir from were changed from
> > S_IRUSR|S_IXUSR to S_IRUGO|S_IXUGO, and a PTRACE_MODE_READ check was
> > added for opening the fdinfo files [1]. However, the ptrace permission
> > check was not added to the directory, allowing anyone to get the open FD
> > numbers by reading the fdinfo directory.
> >
> > Add the missing ptrace permission check for opening the fdinfo directory.
>
> The more I look at this, the more I feel like we should look at
> instead changing how "get_proc_task()" works.
>
> That's one of the core functions for /proc, and I wonder if we
> couldn't just make it refuse to look up a task that has gone through a
> suid execve() since the proc inode was opened.
>
> I don't think it's basically ever ok to open something for one thread,
> and then use it after the thread has gone through a suid thing.
>
> In fact, I wonder if we could make it even stricter, and go "any exec
> at all", but I think a suid exec might be the minimum we should do.
>
> Then the logic really becomes very simple: we did the permission
> checks at open time (like UNIX permission checks should be done), and
> "get_proc_task()" basically verifies that "yeah, that open-time
> decision is still valid".
>
> Wouldn't that make a lot of sense?
I think checking that the last open is after the last exec works, but
there are a few cases I’m not clear on:
Process A opens /proc/A/*/<file>. (Given it has the required
permissions - checked in open())
Process A Start exec time = T1
Proc inode open time /proc/A/*/<file> = T2
T2 > T1: --> Process A can access /proc/A/*/<file> (FD 4) -- OK
Process A does a fork and exec Process B
Process B Start exec time = T3
Proc inode open time /proc/A/*/<file> = T2
T2 < T3: --> Process B can’t access /proc/A/*/<file> (by the copied FD 4) -- OK
Process B opens /proc/B/*/<file> (Given it has the required
permissions - checked in open())
Process B Start exec time = T3
Proc inode open time /proc/B/*/<file> = T4.
T4 > T3: --> Process B can access /proc/B/*/<file> (FD 5) -- OK
Process A opens /proc/A/*/<file> (Given it has the required
permissions - checked in open())
Process A Start exec time = T1
Proc inode open time /proc/A/*/<file> = T5.
T5 > T1: --> Process A can access /proc/A/*/<file> (FD 5) -- OK
But,
Process B Start exec time = T3
Proc inode open time /proc/A/*/<file> = T5.
T5 > T3: --> Process B can access /proc/A/*/<file> (by the copied FD
4) -- NOT OK
I think for the case above we could add a map to track the inode open
times per task at the cost of some added complexity.
For tracking the last exec times, I thought we could maybe reuse the
task_struct -> struct sched_entity se -> u64 exec_start /
sum_exec_runtime as indicators. These are relative to the task and set
to 0 on fork. But the inode open time needs to be comparable across
tasks in the case of a fork-exec as above. As I understand, we may
need a per-task field like last_exec_time, but I’m not sure we want to
incur the extra memory overhead for adding more fields to task_struct?
Please let me know if my understanding is not correct or if there is
something I overlooked here.
Thanks,
Kalesh
>
> Linus
Linus Torvalds <[email protected]> writes:
> On Thu, Jul 8, 2021 at 8:57 AM Kalesh Singh <[email protected]> wrote:
>>
>> The file permissions on the fdinfo dir from were changed from
>> S_IRUSR|S_IXUSR to S_IRUGO|S_IXUGO, and a PTRACE_MODE_READ check was
>> added for opening the fdinfo files [1]. However, the ptrace permission
>> check was not added to the directory, allowing anyone to get the open FD
>> numbers by reading the fdinfo directory.
>>
>> Add the missing ptrace permission check for opening the fdinfo directory.
>
> The more I look at this, the more I feel like we should look at
> instead changing how "get_proc_task()" works.
The practical implementation that I can see is to add a
exec_id attribute into the proc inode and to modify proc_pid_make_inode
to take a new exec_id parameter.
There are some directories like /proc/PPP/, /proc/PPP/task/TTT/,
/proc/PPP/net where it is both safe and appropriate to allow caching the
reference over a suid exec.
To handle that I would have a flag somewhere (possibly a special exec_id
value) that indicates we don't care about the exec id.
Once get_proc_task is taught to handle both cases and the appropriate
exec_id is passed to proc_pid_make_inode proc_pid_invalidate works
automatically. So I think that is all we really need to do.
> That's one of the core functions for /proc, and I wonder if we
> couldn't just make it refuse to look up a task that has gone through a
> suid execve() since the proc inode was opened.
>
> I don't think it's basically ever ok to open something for one thread,
> and then use it after the thread has gone through a suid thing.
>
> In fact, I wonder if we could make it even stricter, and go "any exec
> at all", but I think a suid exec might be the minimum we should do.
>
> Then the logic really becomes very simple: we did the permission
> checks at open time (like UNIX permission checks should be done), and
> "get_proc_task()" basically verifies that "yeah, that open-time
> decision is still valid".
>
> Wouldn't that make a lot of sense?
Roughly. I want to use reuse exec_id but that seems a bit strong for
have the permissions changed. Checking ->cred is too sensitive.
So it is a bit fiddly to get right.
Limiting this to suid-exec (and equivalent) seems like the proper
filter, because it is when the permissions have fundamentally changed.
I just don't think this should be blanket for everything that uses
get_prock_task.
Eric