IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Paul Moore]

Hello all,

I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
see that it is currently written to skip the io_uring auditing.
Assuming I'm understanding the patch correctly, and I'll admit that
I've only looked at it for a short time today, my gut feeling is that
we want to audit the FIXED_FD_INSTALL opcode as it could make a
previously io_uring-only fd generally accessible to userspace.

I'm also trying to determine how worried we should be about
io_install_fixed_fd() potentially happening with the current task's
credentials overridden by the io_uring's personality.  Given that this
io_uring operation inserts a fd into the current process, I believe
that we should be checking to see if the current task's credentials,
and not the io_uring's credentials/personality, are allowed to receive
the fd in receive_fd()/security_file_receive().  I don't see an
obvious way to filter/block credential overrides on a per-opcode
basis, but if we don't want to add a mask for io_kiocb::flags in
io_issue_defs (or something similar), perhaps we can forcibly mask out
REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
hear what others think about this.

Of course if I'm reading the commit or misunderstanding the
IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)

-- 
paul-moore.com

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Jens Axboe]

On 1/19/24 9:33 AM, Paul Moore wrote:
> Hello all,
> 
> I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
> see that it is currently written to skip the io_uring auditing.
> Assuming I'm understanding the patch correctly, and I'll admit that
> I've only looked at it for a short time today, my gut feeling is that
> we want to audit the FIXED_FD_INSTALL opcode as it could make a
> previously io_uring-only fd generally accessible to userspace.

We can certainly remove the audit skip, it was mostly done as we're
calling into the security parts anyway later on. But it's not like doing
the extra audit here would cause any concerns on the io_uring front.

> I'm also trying to determine how worried we should be about
> io_install_fixed_fd() potentially happening with the current task's
> credentials overridden by the io_uring's personality.  Given that this
> io_uring operation inserts a fd into the current process, I believe
> that we should be checking to see if the current task's credentials,
> and not the io_uring's credentials/personality, are allowed to receive
> the fd in receive_fd()/security_file_receive().  I don't see an
> obvious way to filter/block credential overrides on a per-opcode
> basis, but if we don't want to add a mask for io_kiocb::flags in
> io_issue_defs (or something similar), perhaps we can forcibly mask out
> REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
> hear what others think about this.
> 
> Of course if I'm reading the commit or misunderstanding the
> IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)

I think if there are concerns for that, the easiest solution would be to
just fail IORING_OP_FIXED_INSTALL if REQ_F_CREDS is set. I don't really
see a good way to have the security side know about the old creds, as
the task itself is running with the assigned creds.

-- 
Jens Axboe

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Paul Moore]

On Fri, Jan 19, 2024 at 12:02 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 1/19/24 9:33 AM, Paul Moore wrote:
> > Hello all,
> >
> > I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
> > see that it is currently written to skip the io_uring auditing.
> > Assuming I'm understanding the patch correctly, and I'll admit that
> > I've only looked at it for a short time today, my gut feeling is that
> > we want to audit the FIXED_FD_INSTALL opcode as it could make a
> > previously io_uring-only fd generally accessible to userspace.
>
> We can certainly remove the audit skip, it was mostly done as we're
> calling into the security parts anyway later on. But it's not like doing
> the extra audit here would cause any concerns on the io_uring front.

Great.  Do you want to put a patch together for that, or should I?

> > I'm also trying to determine how worried we should be about
> > io_install_fixed_fd() potentially happening with the current task's
> > credentials overridden by the io_uring's personality.  Given that this
> > io_uring operation inserts a fd into the current process, I believe
> > that we should be checking to see if the current task's credentials,
> > and not the io_uring's credentials/personality, are allowed to receive
> > the fd in receive_fd()/security_file_receive().  I don't see an
> > obvious way to filter/block credential overrides on a per-opcode
> > basis, but if we don't want to add a mask for io_kiocb::flags in
> > io_issue_defs (or something similar), perhaps we can forcibly mask out
> > REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
> > hear what others think about this.
> >
> > Of course if I'm reading the commit or misunderstanding the
> > IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)
>
> I think if there are concerns for that, the easiest solution would be to
> just fail IORING_OP_FIXED_INSTALL if REQ_F_CREDS is set. I don't really
> see a good way to have the security side know about the old creds, as
> the task itself is running with the assigned creds.

The more I've been thinking about it, yes, I believe there are
concerns around FIXED_FD_INSTALL and io_uring personalities for LSMs.
Assuming an io_uring with stored credentials for task A, yet
accessible via task B, task B could submit an IORING_OP_OPENAT command
to open a file using task A's creds and then FIXED_FD_INSTALL that fd
into its own (task B's) file descriptor table without a problem as the
installer's creds (the io_uring creds, or task A) match the file's
creds (also task A since the io_uring opened the file).  Following
code paths in task B that end up going through
security_file_permission() and similar hooks may very well end up
catching the mismatch between the file's creds and task B (depending
on the LSM), but arguably it is something that should have been caught
at receive_fd() time.

--
paul-moore.com

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Jens Axboe]

On 1/19/24 10:20 AM, Paul Moore wrote:
> On Fri, Jan 19, 2024 at 12:02?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 1/19/24 9:33 AM, Paul Moore wrote:
>>> Hello all,
>>>
>>> I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
>>> see that it is currently written to skip the io_uring auditing.
>>> Assuming I'm understanding the patch correctly, and I'll admit that
>>> I've only looked at it for a short time today, my gut feeling is that
>>> we want to audit the FIXED_FD_INSTALL opcode as it could make a
>>> previously io_uring-only fd generally accessible to userspace.
>>
>> We can certainly remove the audit skip, it was mostly done as we're
>> calling into the security parts anyway later on. But it's not like doing
>> the extra audit here would cause any concerns on the io_uring front.
> 
> Great.  Do you want to put a patch together for that, or should I?

Either way - I'd say if you have time to do it, please do! Probably just
include the REQ_F_CREDS change too. FWIW, I'd add that in
io_uring/openclose.c:io_install_fixed_fd_prep() - just check for
REQ_F_CREDS in there and return -EPERM (I think that would be
appropriate?) and that should disallow any IORING_OP_FIXED_FD_INSTALL if
creds have been reassigned.

>>> I'm also trying to determine how worried we should be about
>>> io_install_fixed_fd() potentially happening with the current task's
>>> credentials overridden by the io_uring's personality.  Given that this
>>> io_uring operation inserts a fd into the current process, I believe
>>> that we should be checking to see if the current task's credentials,
>>> and not the io_uring's credentials/personality, are allowed to receive
>>> the fd in receive_fd()/security_file_receive().  I don't see an
>>> obvious way to filter/block credential overrides on a per-opcode
>>> basis, but if we don't want to add a mask for io_kiocb::flags in
>>> io_issue_defs (or something similar), perhaps we can forcibly mask out
>>> REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
>>> hear what others think about this.
>>>
>>> Of course if I'm reading the commit or misunderstanding the
>>> IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)
>>
>> I think if there are concerns for that, the easiest solution would be to
>> just fail IORING_OP_FIXED_INSTALL if REQ_F_CREDS is set. I don't really
>> see a good way to have the security side know about the old creds, as
>> the task itself is running with the assigned creds.
> 
> The more I've been thinking about it, yes, I believe there are
> concerns around FIXED_FD_INSTALL and io_uring personalities for LSMs.
> Assuming an io_uring with stored credentials for task A, yet
> accessible via task B, task B could submit an IORING_OP_OPENAT command
> to open a file using task A's creds and then FIXED_FD_INSTALL that fd
> into its own (task B's) file descriptor table without a problem as the
> installer's creds (the io_uring creds, or task A) match the file's
> creds (also task A since the io_uring opened the file).  Following
> code paths in task B that end up going through
> security_file_permission() and similar hooks may very well end up
> catching the mismatch between the file's creds and task B (depending
> on the LSM), but arguably it is something that should have been caught
> at receive_fd() time.

If there are any concerns, then I say let's just explicitly disable it
rather than rely on maybe something in the security checking catching
it. Especially because I don't think there's a valid use case for doing
this, other than perhaps trying to bypass checks you'd normally hit.
Better to err on the side of caution then.

See above for a HOWTO, if in doubt.

Thanks for looking into this!

-- 
Jens Axboe

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Paul Moore]

On Fri, Jan 19, 2024 at 12:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/19/24 10:20 AM, Paul Moore wrote:
> > On Fri, Jan 19, 2024 at 12:02?PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/19/24 9:33 AM, Paul Moore wrote:
> >>> Hello all,
> >>>
> >>> I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
> >>> see that it is currently written to skip the io_uring auditing.
> >>> Assuming I'm understanding the patch correctly, and I'll admit that
> >>> I've only looked at it for a short time today, my gut feeling is that
> >>> we want to audit the FIXED_FD_INSTALL opcode as it could make a
> >>> previously io_uring-only fd generally accessible to userspace.
> >>
> >> We can certainly remove the audit skip, it was mostly done as we're
> >> calling into the security parts anyway later on. But it's not like doing
> >> the extra audit here would cause any concerns on the io_uring front.
> >
> > Great.  Do you want to put a patch together for that, or should I?
>
> Either way - I'd say if you have time to do it, please do!

Okay, will do.  Just a heads up that due to personal commitments this
weekend a proper, tested fix may not come until early next week.  With
this only appearing in Linus' tree during this merge window we've got
plenty of time to fix this before v6.8 is tagged.

This would also give people an opportunity to point out any flaws in
my thinking - in particular I'm looking at you other LSM folks.

> Probably just include the REQ_F_CREDS change too.

Sure.

> FWIW, I'd add that in
> io_uring/openclose.c:io_install_fixed_fd_prep() - just check for
> REQ_F_CREDS in there and return -EPERM (I think that would be
> appropriate?) and that should disallow any IORING_OP_FIXED_FD_INSTALL if
> creds have been reassigned.

Yeah, easy enough.  I was originally thinking of masking out
REQ_F_CREDS there, but if you are okay with simply rejecting the
operation in this case it makes everything much easier (and more
predictable from a userspace perspective).

> >>> I'm also trying to determine how worried we should be about
> >>> io_install_fixed_fd() potentially happening with the current task's
> >>> credentials overridden by the io_uring's personality.  Given that this
> >>> io_uring operation inserts a fd into the current process, I believe
> >>> that we should be checking to see if the current task's credentials,
> >>> and not the io_uring's credentials/personality, are allowed to receive
> >>> the fd in receive_fd()/security_file_receive().  I don't see an
> >>> obvious way to filter/block credential overrides on a per-opcode
> >>> basis, but if we don't want to add a mask for io_kiocb::flags in
> >>> io_issue_defs (or something similar), perhaps we can forcibly mask out
> >>> REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
> >>> hear what others think about this.
> >>>
> >>> Of course if I'm reading the commit or misunderstanding the
> >>> IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)
> >>
> >> I think if there are concerns for that, the easiest solution would be to
> >> just fail IORING_OP_FIXED_INSTALL if REQ_F_CREDS is set. I don't really
> >> see a good way to have the security side know about the old creds, as
> >> the task itself is running with the assigned creds.
> >
> > The more I've been thinking about it, yes, I believe there are
> > concerns around FIXED_FD_INSTALL and io_uring personalities for LSMs.
> > Assuming an io_uring with stored credentials for task A, yet
> > accessible via task B, task B could submit an IORING_OP_OPENAT command
> > to open a file using task A's creds and then FIXED_FD_INSTALL that fd
> > into its own (task B's) file descriptor table without a problem as the
> > installer's creds (the io_uring creds, or task A) match the file's
> > creds (also task A since the io_uring opened the file).  Following
> > code paths in task B that end up going through
> > security_file_permission() and similar hooks may very well end up
> > catching the mismatch between the file's creds and task B (depending
> > on the LSM), but arguably it is something that should have been caught
> > at receive_fd() time.
>
> If there are any concerns, then I say let's just explicitly disable it
> rather than rely on maybe something in the security checking catching
> it. Especially because I don't think there's a valid use case for doing
> this, other than perhaps trying to bypass checks you'd normally hit.
> Better to err on the side of caution then.

Agreed.  I'll go ahead and make the change.  Thanks for the quick
responses and understanding of a very security-biased perspective :)

-- 
paul-moore.com

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Jens Axboe]

On 1/19/24 10:54 AM, Paul Moore wrote:
> On Fri, Jan 19, 2024 at 12:41?PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/19/24 10:20 AM, Paul Moore wrote:
>>> On Fri, Jan 19, 2024 at 12:02?PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/19/24 9:33 AM, Paul Moore wrote:
>>>>> Hello all,
>>>>>
>>>>> I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
>>>>> see that it is currently written to skip the io_uring auditing.
>>>>> Assuming I'm understanding the patch correctly, and I'll admit that
>>>>> I've only looked at it for a short time today, my gut feeling is that
>>>>> we want to audit the FIXED_FD_INSTALL opcode as it could make a
>>>>> previously io_uring-only fd generally accessible to userspace.
>>>>
>>>> We can certainly remove the audit skip, it was mostly done as we're
>>>> calling into the security parts anyway later on. But it's not like doing
>>>> the extra audit here would cause any concerns on the io_uring front.
>>>
>>> Great.  Do you want to put a patch together for that, or should I?
>>
>> Either way - I'd say if you have time to do it, please do!
> 
> Okay, will do.  Just a heads up that due to personal commitments this
> weekend a proper, tested fix may not come until early next week.  With
> this only appearing in Linus' tree during this merge window we've got
> plenty of time to fix this before v6.8 is tagged.

No worries on that, there's no rush as we are early in the cycle.

>> FWIW, I'd add that in
>> io_uring/openclose.c:io_install_fixed_fd_prep() - just check for
>> REQ_F_CREDS in there and return -EPERM (I think that would be
>> appropriate?) and that should disallow any IORING_OP_FIXED_FD_INSTALL if
>> creds have been reassigned.
> 
> Yeah, easy enough.  I was originally thinking of masking out
> REQ_F_CREDS there, but if you are okay with simply rejecting the
> operation in this case it makes everything much easier (and more
> predictable from a userspace perspective).

It'd be too late to mask it out, as it may already have been assigned.
And on top of that, it would introduce a weird scenario where the
application thinks that creds have been assigned and it completes
successfully, but in reality it didn't use the specified creds. For
those cases we absolutely must fail the request, as it didn't do exactly
what it was asked to.

>>>>> I'm also trying to determine how worried we should be about
>>>>> io_install_fixed_fd() potentially happening with the current task's
>>>>> credentials overridden by the io_uring's personality.  Given that this
>>>>> io_uring operation inserts a fd into the current process, I believe
>>>>> that we should be checking to see if the current task's credentials,
>>>>> and not the io_uring's credentials/personality, are allowed to receive
>>>>> the fd in receive_fd()/security_file_receive().  I don't see an
>>>>> obvious way to filter/block credential overrides on a per-opcode
>>>>> basis, but if we don't want to add a mask for io_kiocb::flags in
>>>>> io_issue_defs (or something similar), perhaps we can forcibly mask out
>>>>> REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
>>>>> hear what others think about this.
>>>>>
>>>>> Of course if I'm reading the commit or misunderstanding the
>>>>> IORING_OP_FIXED_FD_INSTALL operation, corrections are welcome :)
>>>>
>>>> I think if there are concerns for that, the easiest solution would be to
>>>> just fail IORING_OP_FIXED_INSTALL if REQ_F_CREDS is set. I don't really
>>>> see a good way to have the security side know about the old creds, as
>>>> the task itself is running with the assigned creds.
>>>
>>> The more I've been thinking about it, yes, I believe there are
>>> concerns around FIXED_FD_INSTALL and io_uring personalities for LSMs.
>>> Assuming an io_uring with stored credentials for task A, yet
>>> accessible via task B, task B could submit an IORING_OP_OPENAT command
>>> to open a file using task A's creds and then FIXED_FD_INSTALL that fd
>>> into its own (task B's) file descriptor table without a problem as the
>>> installer's creds (the io_uring creds, or task A) match the file's
>>> creds (also task A since the io_uring opened the file).  Following
>>> code paths in task B that end up going through
>>> security_file_permission() and similar hooks may very well end up
>>> catching the mismatch between the file's creds and task B (depending
>>> on the LSM), but arguably it is something that should have been caught
>>> at receive_fd() time.
>>
>> If there are any concerns, then I say let's just explicitly disable it
>> rather than rely on maybe something in the security checking catching
>> it. Especially because I don't think there's a valid use case for doing
>> this, other than perhaps trying to bypass checks you'd normally hit.
>> Better to err on the side of caution then.
> 
> Agreed.  I'll go ahead and make the change.  Thanks for the quick
> responses and understanding of a very security-biased perspective :)

It's always better to catch weirdness or ambiguities like this early, so
much appreciated!

-- 
Jens Axboe

Re: IORING_OP_FIXED_FD_INSTALL and audit/LSM interactions

[Christian Brauner]

On Fri, Jan 19, 2024 at 11:33:37AM -0500, Paul Moore wrote:
> Hello all,
> 
> I just noticed the recent addition of IORING_OP_FIXED_FD_INSTALL and I
> see that it is currently written to skip the io_uring auditing.
> Assuming I'm understanding the patch correctly, and I'll admit that
> I've only looked at it for a short time today, my gut feeling is that
> we want to audit the FIXED_FD_INSTALL opcode as it could make a
> previously io_uring-only fd generally accessible to userspace.
> 
> I'm also trying to determine how worried we should be about
> io_install_fixed_fd() potentially happening with the current task's
> credentials overridden by the io_uring's personality.  Given that this
> io_uring operation inserts a fd into the current process, I believe
> that we should be checking to see if the current task's credentials,
> and not the io_uring's credentials/personality, are allowed to receive
> the fd in receive_fd()/security_file_receive().  I don't see an
> obvious way to filter/block credential overrides on a per-opcode
> basis, but if we don't want to add a mask for io_kiocb::flags in
> io_issue_defs (or something similar), perhaps we can forcibly mask out
> REQ_F_CREDS in io_install_fixed_fd_prep()?  I'm very interested to
> hear what others think about this.

Right, completely forgot about the creds support in io_uring. Just
disallow this together with FIXED_FD_INSTALL. That's also the gist of
the rest of this thread iiuc.