[PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Paul Moore]

We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
command to take into account the security implications of making an
io_uring-private file descriptor generally accessible to a userspace
task.

The first change in this patch is to enable auditing of the FD_INSTALL
operation as installing a file descriptor into a task's file descriptor
table is a security relevant operation and something that admins/users
may want to audit.

The second change is to disable the io_uring credential override
functionality, also known as io_uring "personalities", in the
FD_INSTALL command.  The credential override in FD_INSTALL is
particularly problematic as it affects the credentials used in the
security_file_receive() LSM hook.  If a task were to request a
credential override via REQ_F_CREDS on a FD_INSTALL operation, the LSM
would incorrectly check to see if the overridden credentials of the
io_uring were able to "receive" the file as opposed to the task's
credentials.  After discussions upstream, it's difficult to imagine a
use case where we would want to allow a credential override on a
FD_INSTALL operation so we are simply going to block REQ_F_CREDS on
IORING_OP_FIXED_FD_INSTALL operations.

Fixes: dc18b89ab113 ("io_uring/openclose: add support for IORING_OP_FIXED_FD_INSTALL")
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 io_uring/opdef.c     | 1 -
 io_uring/openclose.c | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 6705634e5f52..b1ee3a9c3807 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -471,7 +471,6 @@ const struct io_issue_def io_issue_defs[] = {
 	},
 	[IORING_OP_FIXED_FD_INSTALL] = {
 		.needs_file		= 1,
-		.audit_skip		= 1,
 		.prep			= io_install_fixed_fd_prep,
 		.issue			= io_install_fixed_fd,
 	},
diff --git a/io_uring/openclose.c b/io_uring/openclose.c
index 0fe0dd305546..e3357dfa14ca 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -277,6 +277,10 @@ int io_install_fixed_fd_prep(struct io_kiocb *req, const struct io_uring_sqe *sq
 	if (flags & ~IORING_FIXED_FD_NO_CLOEXEC)
 		return -EINVAL;
 
+	/* ensure the task's creds are used when installing/receiving fds */
+	if (req->flags & REQ_F_CREDS)
+		return -EPERM;
+
 	/* default to O_CLOEXEC, disable if IORING_FIXED_FD_NO_CLOEXEC is set */
 	ifi = io_kiocb_to_cmd(req, struct io_fixed_install);
 	ifi->o_flags = O_CLOEXEC;
-- 
2.43.0

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Paul Moore]

On Tue, Jan 23, 2024 at 4:55 PM Paul Moore <paul@paul-moore.com> wrote:
>
> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
> command to take into account the security implications of making an
> io_uring-private file descriptor generally accessible to a userspace
> task.
>
> The first change in this patch is to enable auditing of the FD_INSTALL
> operation as installing a file descriptor into a task's file descriptor
> table is a security relevant operation and something that admins/users
> may want to audit.
>
> The second change is to disable the io_uring credential override
> functionality, also known as io_uring "personalities", in the
> FD_INSTALL command.  The credential override in FD_INSTALL is
> particularly problematic as it affects the credentials used in the
> security_file_receive() LSM hook.  If a task were to request a
> credential override via REQ_F_CREDS on a FD_INSTALL operation, the LSM
> would incorrectly check to see if the overridden credentials of the
> io_uring were able to "receive" the file as opposed to the task's
> credentials.  After discussions upstream, it's difficult to imagine a
> use case where we would want to allow a credential override on a
> FD_INSTALL operation so we are simply going to block REQ_F_CREDS on
> IORING_OP_FIXED_FD_INSTALL operations.
>
> Fixes: dc18b89ab113 ("io_uring/openclose: add support for IORING_OP_FIXED_FD_INSTALL")
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  io_uring/opdef.c     | 1 -
>  io_uring/openclose.c | 4 ++++
>  2 files changed, 4 insertions(+), 1 deletion(-)

Not having an IORING_OP_FIXED_FD_INSTALL test handy I only did some
basic sanity tests before posting, I would appreciate it if the
io_uring folks could run this through whatever FD_INSTALL tests you
have.

> diff --git a/io_uring/opdef.c b/io_uring/opdef.c
> index 6705634e5f52..b1ee3a9c3807 100644
> --- a/io_uring/opdef.c
> +++ b/io_uring/opdef.c
> @@ -471,7 +471,6 @@ const struct io_issue_def io_issue_defs[] = {
>         },
>         [IORING_OP_FIXED_FD_INSTALL] = {
>                 .needs_file             = 1,
> -               .audit_skip             = 1,
>                 .prep                   = io_install_fixed_fd_prep,
>                 .issue                  = io_install_fixed_fd,
>         },
> diff --git a/io_uring/openclose.c b/io_uring/openclose.c
> index 0fe0dd305546..e3357dfa14ca 100644
> --- a/io_uring/openclose.c
> +++ b/io_uring/openclose.c
> @@ -277,6 +277,10 @@ int io_install_fixed_fd_prep(struct io_kiocb *req, const struct io_uring_sqe *sq
>         if (flags & ~IORING_FIXED_FD_NO_CLOEXEC)
>                 return -EINVAL;
>
> +       /* ensure the task's creds are used when installing/receiving fds */
> +       if (req->flags & REQ_F_CREDS)
> +               return -EPERM;
> +
>         /* default to O_CLOEXEC, disable if IORING_FIXED_FD_NO_CLOEXEC is set */
>         ifi = io_kiocb_to_cmd(req, struct io_fixed_install);
>         ifi->o_flags = O_CLOEXEC;
> --
> 2.43.0

-- 
paul-moore.com

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Jens Axboe]

On 1/23/24 2:57 PM, Paul Moore wrote:
> On Tue, Jan 23, 2024 at 4:55?PM Paul Moore <paul@paul-moore.com> wrote:
>>
>> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
>> command to take into account the security implications of making an
>> io_uring-private file descriptor generally accessible to a userspace
>> task.
>>
>> The first change in this patch is to enable auditing of the FD_INSTALL
>> operation as installing a file descriptor into a task's file descriptor
>> table is a security relevant operation and something that admins/users
>> may want to audit.
>>
>> The second change is to disable the io_uring credential override
>> functionality, also known as io_uring "personalities", in the
>> FD_INSTALL command.  The credential override in FD_INSTALL is
>> particularly problematic as it affects the credentials used in the
>> security_file_receive() LSM hook.  If a task were to request a
>> credential override via REQ_F_CREDS on a FD_INSTALL operation, the LSM
>> would incorrectly check to see if the overridden credentials of the
>> io_uring were able to "receive" the file as opposed to the task's
>> credentials.  After discussions upstream, it's difficult to imagine a
>> use case where we would want to allow a credential override on a
>> FD_INSTALL operation so we are simply going to block REQ_F_CREDS on
>> IORING_OP_FIXED_FD_INSTALL operations.
>>
>> Fixes: dc18b89ab113 ("io_uring/openclose: add support for IORING_OP_FIXED_FD_INSTALL")
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>  io_uring/opdef.c     | 1 -
>>  io_uring/openclose.c | 4 ++++
>>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> Not having an IORING_OP_FIXED_FD_INSTALL test handy I only did some
> basic sanity tests before posting, I would appreciate it if the
> io_uring folks could run this through whatever FD_INSTALL tests you
> have.

You bet, I'm going to augment the existing test case with one that
passes in creds as well just to verify that part fails as it should as
well.

But looking at the patch, this will surely work.

-- 
Jens Axboe

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Jens Axboe]

On Tue, 23 Jan 2024 16:55:02 -0500, Paul Moore wrote:
> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
> command to take into account the security implications of making an
> io_uring-private file descriptor generally accessible to a userspace
> task.
> 
> The first change in this patch is to enable auditing of the FD_INSTALL
> operation as installing a file descriptor into a task's file descriptor
> table is a security relevant operation and something that admins/users
> may want to audit.
> 
> [...]

Applied, thanks!

[1/1] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL
      commit: 16bae3e1377846734ec6b87eee459c0f3551692c

Best regards,
-- 
Jens Axboe

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Jens Axboe]

On 1/23/24 3:35 PM, Jens Axboe wrote:
> 
> On Tue, 23 Jan 2024 16:55:02 -0500, Paul Moore wrote:
>> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
>> command to take into account the security implications of making an
>> io_uring-private file descriptor generally accessible to a userspace
>> task.
>>
>> The first change in this patch is to enable auditing of the FD_INSTALL
>> operation as installing a file descriptor into a task's file descriptor
>> table is a security relevant operation and something that admins/users
>> may want to audit.
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL
>       commit: 16bae3e1377846734ec6b87eee459c0f3551692c

So after doing that and writing the test case and testing it, it dawned
on me that we should potentially allow the current task creds. And to
make matters worse, this is indeed what happens if eg the application
would submit this with IOSQE_ASYNC or if it was part of a linked series
and we marked it async.

While I originally reasoned for why this is fine as it'd be silly to
register your current creds and then proceed to pass in that personality,
I do think that we should probably handle that case and clearly separate
the case of "we assigned creds from the submitting task because we're
handing it to a thread" vs "the submitting task asked for other creds
that were previously registered".

I'll take a look and see what works the best here.

-- 
Jens Axboe

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Jens Axboe]

On 1/23/24 3:40 PM, Jens Axboe wrote:
> On 1/23/24 3:35 PM, Jens Axboe wrote:
>>
>> On Tue, 23 Jan 2024 16:55:02 -0500, Paul Moore wrote:
>>> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
>>> command to take into account the security implications of making an
>>> io_uring-private file descriptor generally accessible to a userspace
>>> task.
>>>
>>> The first change in this patch is to enable auditing of the FD_INSTALL
>>> operation as installing a file descriptor into a task's file descriptor
>>> table is a security relevant operation and something that admins/users
>>> may want to audit.
>>>
>>> [...]
>>
>> Applied, thanks!
>>
>> [1/1] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL
>>       commit: 16bae3e1377846734ec6b87eee459c0f3551692c
> 
> So after doing that and writing the test case and testing it, it dawned
> on me that we should potentially allow the current task creds. And to
> make matters worse, this is indeed what happens if eg the application
> would submit this with IOSQE_ASYNC or if it was part of a linked series
> and we marked it async.
> 
> While I originally reasoned for why this is fine as it'd be silly to
> register your current creds and then proceed to pass in that personality,
> I do think that we should probably handle that case and clearly separate
> the case of "we assigned creds from the submitting task because we're
> handing it to a thread" vs "the submitting task asked for other creds
> that were previously registered".
> 
> I'll take a look and see what works the best here.

Actually, a quick look and it's fine, the usual async offload will do
the right thing. So let's just keep it as-is, I don't think there's any
point to complicating this for some theoretically-valid-but-obscure use
case!

FWIW, the test case is here, and I'll augment it now to add IOSQE_ASYNC
as well just to cover all the bases.

https://git.kernel.dk/cgit/liburing/commit/?id=bc576ca398661b266d3e4a4f5db3a9cf7f33fe62

-- 
Jens Axboe

Re: [PATCH] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL

[Paul Moore]

On Tue, Jan 23, 2024 at 5:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/23/24 3:40 PM, Jens Axboe wrote:
> > On 1/23/24 3:35 PM, Jens Axboe wrote:
> >>
> >> On Tue, 23 Jan 2024 16:55:02 -0500, Paul Moore wrote:
> >>> We need to correct some aspects of the IORING_OP_FIXED_FD_INSTALL
> >>> command to take into account the security implications of making an
> >>> io_uring-private file descriptor generally accessible to a userspace
> >>> task.
> >>>
> >>> The first change in this patch is to enable auditing of the FD_INSTALL
> >>> operation as installing a file descriptor into a task's file descriptor
> >>> table is a security relevant operation and something that admins/users
> >>> may want to audit.
> >>>
> >>> [...]
> >>
> >> Applied, thanks!
> >>
> >> [1/1] io_uring: enable audit and restrict cred override for IORING_OP_FIXED_FD_INSTALL
> >>       commit: 16bae3e1377846734ec6b87eee459c0f3551692c
> >
> > So after doing that and writing the test case and testing it, it dawned
> > on me that we should potentially allow the current task creds. And to
> > make matters worse, this is indeed what happens if eg the application
> > would submit this with IOSQE_ASYNC or if it was part of a linked series
> > and we marked it async.
> >
> > While I originally reasoned for why this is fine as it'd be silly to
> > register your current creds and then proceed to pass in that personality,
> > I do think that we should probably handle that case and clearly separate
> > the case of "we assigned creds from the submitting task because we're
> > handing it to a thread" vs "the submitting task asked for other creds
> > that were previously registered".
> >
> > I'll take a look and see what works the best here.
>
> Actually, a quick look and it's fine, the usual async offload will do
> the right thing. So let's just keep it as-is, I don't think there's any
> point to complicating this for some theoretically-valid-but-obscure use
> case!

Perhaps the one case where REQ_F_CREDS is our friend for FD_INSTALL ;)

> FWIW, the test case is here, and I'll augment it now to add IOSQE_ASYNC
> as well just to cover all the bases.
>
> https://git.kernel.dk/cgit/liburing/commit/?id=bc576ca398661b266d3e4a4f5db3a9cf7f33fe62

Great, thanks!

-- 
paul-moore.com