Possible bug or unintended behaviour using bpf_ima_file_hash

Possible bug or unintended behaviour using bpf_ima_file_hash

[Isaac Matthews]

Using bpf_ima_file_hash() from kernel 6.0.

When using bpf_ima_file_hash() with the lsm.s/file_open hook, a hash
of the file is only sometimes returned.  This is because the
FMODE_CAN_READ flag is set after security_file_open() is already
called, and ima_calc_file_hash() only checks for FMODE_READ not
FMODE_CAN_READ in order to decide if a new instance needs to be
opened. Because of this, if a file passes the FMODE_READ check  it
will fail to be hashed as FMODE_CAN_READ has not yet been set.

To demonstrate: if the file is opened for write for example, when
ima_calc_file_hash() is called and the file->f_mode is checked against
FMODE_READ, a new file instance is opened with the correct flags and a
hash is returned. If the file is opened for read, a new file instance
is not returned in ima_calc_file_hash() as (!(file->f_mode &
FMODE_READ)) is now false. When __kernel_read() is called as part of
ima_calc_file_hash_tfm() it will fail on if (!(file->f_mode &
FMODE_CAN_READ)) and so no hash will be returned by
bpf_ima_file_hash().

If possible could someone please advise me as to whether this is
intended behaviour, and is it possible to either modify the flags with
eBPF or to open a new instance with the correct flags set as IMA does
currently?

Alternatively, would a better solution be adding a check for
FMODE_CAN_READ to ima_calc_file_hash()? I noticed in the comment above
the conditional in ima_calc_file_hash() that the conditional should be
checking whether the file can be read, but only checks the FMODE_READ
flag which is not the only requirement for __kernel_read() to be able
to read a file.

Thanks for your help.
Isaac

Re: Possible bug or unintended behaviour using bpf_ima_file_hash

[Roberto Sassu]

On Mon, 2022-10-31 at 16:25 +0000, Isaac Matthews wrote:
> Using bpf_ima_file_hash() from kernel 6.0.
> 
> When using bpf_ima_file_hash() with the lsm.s/file_open hook, a hash
> of the file is only sometimes returned.  This is because the
> FMODE_CAN_READ flag is set after security_file_open() is already
> called, and ima_calc_file_hash() only checks for FMODE_READ not
> FMODE_CAN_READ in order to decide if a new instance needs to be
> opened. Because of this, if a file passes the FMODE_READ check  it
> will fail to be hashed as FMODE_CAN_READ has not yet been set.
> 
> To demonstrate: if the file is opened for write for example, when
> ima_calc_file_hash() is called and the file->f_mode is checked
> against
> FMODE_READ, a new file instance is opened with the correct flags and
> a
> hash is returned. If the file is opened for read, a new file instance
> is not returned in ima_calc_file_hash() as (!(file->f_mode &
> FMODE_READ)) is now false. When __kernel_read() is called as part of
> ima_calc_file_hash_tfm() it will fail on if (!(file->f_mode &
> FMODE_CAN_READ)) and so no hash will be returned by
> bpf_ima_file_hash().
> 
> If possible could someone please advise me as to whether this is
> intended behaviour, and is it possible to either modify the flags
> with
> eBPF or to open a new instance with the correct flags set as IMA does
> currently?

Hi Isaac

I think this is the intended behavior, as IMA is supposed to be called
when the file descriptor is ready to use.

If we need to call ima_file_hash() from lsm.s/file_open, I think it
should not be a problem to create a new fd just for eBPF, in
__ima_inode_hash().

Mimi, what do you think?

Thanks

Roberto

> Alternatively, would a better solution be adding a check for
> FMODE_CAN_READ to ima_calc_file_hash()? I noticed in the comment
> above
> the conditional in ima_calc_file_hash() that the conditional should
> be
> checking whether the file can be read, but only checks the FMODE_READ
> flag which is not the only requirement for __kernel_read() to be able
> to read a file.
> 
> Thanks for your help.
> Isaac

Re: Possible bug or unintended behaviour using bpf_ima_file_hash

[Mimi Zohar]

On Wed, 2022-11-02 at 09:08 +0100, Roberto Sassu wrote:
> On Mon, 2022-10-31 at 16:25 +0000, Isaac Matthews wrote:
> > Using bpf_ima_file_hash() from kernel 6.0.
> > 
> > When using bpf_ima_file_hash() with the lsm.s/file_open hook, a hash
> > of the file is only sometimes returned.  This is because the
> > FMODE_CAN_READ flag is set after security_file_open() is already
> > called, and ima_calc_file_hash() only checks for FMODE_READ not
> > FMODE_CAN_READ in order to decide if a new instance needs to be
> > opened. Because of this, if a file passes the FMODE_READ check  it
> > will fail to be hashed as FMODE_CAN_READ has not yet been set.
> > 
> > To demonstrate: if the file is opened for write for example, when
> > ima_calc_file_hash() is called and the file->f_mode is checked
> > against
> > FMODE_READ, a new file instance is opened with the correct flags and
> > a
> > hash is returned. If the file is opened for read, a new file instance
> > is not returned in ima_calc_file_hash() as (!(file->f_mode &
> > FMODE_READ)) is now false. When __kernel_read() is called as part of
> > ima_calc_file_hash_tfm() it will fail on if (!(file->f_mode &
> > FMODE_CAN_READ)) and so no hash will be returned by
> > bpf_ima_file_hash().
> > 
> > If possible could someone please advise me as to whether this is
> > intended behaviour, and is it possible to either modify the flags
> > with
> > eBPF or to open a new instance with the correct flags set as IMA does
> > currently?
> 
> Hi Isaac
> 
> I think this is the intended behavior, as IMA is supposed to be called
> when the file descriptor is ready to use.
> 
> If we need to call ima_file_hash() from lsm.s/file_open, I think it
> should not be a problem to create a new fd just for eBPF, in
> __ima_inode_hash().
> 
> Mimi, what do you think?

Who/what is checking that this is a regular file and we have permission
to open the file?  Are we relying on eBPF to do this?  Will opening a
file circumvent all of the LSM checks?

> 
> > Alternatively, would a better solution be adding a check for
> > FMODE_CAN_READ to ima_calc_file_hash()? I noticed in the comment
> > above
> > the conditional in ima_calc_file_hash() that the conditional should
> > be
> > checking whether the file can be read, but only checks the FMODE_READ
> > flag which is not the only requirement for __kernel_read() to be able
> > to read a file.
> > 
> > Thanks for your help.
> > Isaac
> 

Re: Possible bug or unintended behaviour using bpf_ima_file_hash

[Roberto Sassu]

On Wed, 2022-11-02 at 07:55 -0400, Mimi Zohar wrote:
> On Wed, 2022-11-02 at 09:08 +0100, Roberto Sassu wrote:
> > On Mon, 2022-10-31 at 16:25 +0000, Isaac Matthews wrote:
> > > Using bpf_ima_file_hash() from kernel 6.0.
> > > 
> > > When using bpf_ima_file_hash() with the lsm.s/file_open hook, a
> > > hash
> > > of the file is only sometimes returned.  This is because the
> > > FMODE_CAN_READ flag is set after security_file_open() is already
> > > called, and ima_calc_file_hash() only checks for FMODE_READ not
> > > FMODE_CAN_READ in order to decide if a new instance needs to be
> > > opened. Because of this, if a file passes the FMODE_READ
> > > check  it
> > > will fail to be hashed as FMODE_CAN_READ has not yet been set.
> > > 
> > > To demonstrate: if the file is opened for write for example, when
> > > ima_calc_file_hash() is called and the file->f_mode is checked
> > > against
> > > FMODE_READ, a new file instance is opened with the correct flags
> > > and
> > > a
> > > hash is returned. If the file is opened for read, a new file
> > > instance
> > > is not returned in ima_calc_file_hash() as (!(file->f_mode &
> > > FMODE_READ)) is now false. When __kernel_read() is called as part
> > > of
> > > ima_calc_file_hash_tfm() it will fail on if (!(file->f_mode &
> > > FMODE_CAN_READ)) and so no hash will be returned by
> > > bpf_ima_file_hash().
> > > 
> > > If possible could someone please advise me as to whether this is
> > > intended behaviour, and is it possible to either modify the flags
> > > with
> > > eBPF or to open a new instance with the correct flags set as IMA
> > > does
> > > currently?
> > 
> > Hi Isaac
> > 
> > I think this is the intended behavior, as IMA is supposed to be
> > called
> > when the file descriptor is ready to use.
> > 
> > If we need to call ima_file_hash() from lsm.s/file_open, I think it
> > should not be a problem to create a new fd just for eBPF, in
> > __ima_inode_hash().
> > 
> > Mimi, what do you think?
> 
> Who/what is checking that this is a regular file and we have
> permission
> to open the file?  Are we relying on eBPF to do this?  Will opening a
> file circumvent all of the LSM checks?

Opening the file again will cause another permission request to be sent
to LSMs, and thus to the eBPF program implementing lsm.s/file_open.
Maybe it is not a good idea to use this hook.

In the future, if IMA/EVM stacking is successful, we might introduce
the file_post_open hook, which I believe could be suitable for calling 
bpf_ima_file_hash().

Roberto

> > > Alternatively, would a better solution be adding a check for
> > > FMODE_CAN_READ to ima_calc_file_hash()? I noticed in the comment
> > > above
> > > the conditional in ima_calc_file_hash() that the conditional
> > > should
> > > be
> > > checking whether the file can be read, but only checks the
> > > FMODE_READ
> > > flag which is not the only requirement for __kernel_read() to be
> > > able
> > > to read a file.
> > > 
> > > Thanks for your help.
> > > Isaac