appraisal reset safety

appraisal reset safety

[Janne Karhunen]

Hi,

I've setup an android based mobile device with pretty complete ima/evm
setup that covers just about all the standard use cases (imasig based
filesystems, ota support, factory reset support etc). All that is fine
and ima runs like a clock.

Since this is a mobile device, running out of battery or getting shot
in the head by something is always a realistic option. The random
resets seem to be leading into random appraisal failures as android
seems to be keeping surprisingly many files constantly open for
writing. So many actually, that I feel somewhat uneasy starting to
whitelist these files from the ima policy. That sounds like a viable
route only when it comes to the log files as those files primarily
move data only one way.

Now, is there any prior art on this how to make this work right? The
improvements that I can instantly think of are,
1) whitelist everything that can be,
2) reduce the vfs flush delays,
3) make it detect the reset condition and fix the known files when
that happened. Unsafe and requires a patch (but that seems easy).

Anything else?


--
Janne

Re: appraisal reset safety

[Janne Karhunen]

Hi,

Any thoughts on this? I would guess every system with active databases
would need to address this somehow?


--
Janne

On Fri, Apr 5, 2019 at 3:46 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> I've setup an android based mobile device with pretty complete ima/evm
> setup that covers just about all the standard use cases (imasig based
> filesystems, ota support, factory reset support etc). All that is fine
> and ima runs like a clock.
>
> Since this is a mobile device, running out of battery or getting shot
> in the head by something is always a realistic option. The random
> resets seem to be leading into random appraisal failures as android
> seems to be keeping surprisingly many files constantly open for
> writing. So many actually, that I feel somewhat uneasy starting to
> whitelist these files from the ima policy. That sounds like a viable
> route only when it comes to the log files as those files primarily
> move data only one way.
>
> Now, is there any prior art on this how to make this work right? The
> improvements that I can instantly think of are,
> 1) whitelist everything that can be,
> 2) reduce the vfs flush delays,
> 3) make it detect the reset condition and fix the known files when
> that happened. Unsafe and requires a patch (but that seems easy).
>
> Anything else?
>
>
> --
> Janne

Re: appraisal reset safety

[Janne Karhunen]

Hi,

Hmm, looks to me ima_update_xattr seems to be kicking in only from the
appraisal failure when in fix mode or via fput() delayed work item.
So, no sync() or anything like that will ever help and there is
nothing listening on the i_version updates. Moreover, there is no
integrity hook for write() or sync() to put such update in. Uh. I was
under impression it would somehow see the interim file updates, but I
guess no. Fundamental misunderstanding from my point of view how this
thing works, duh.


--
Janne

On Sat, Apr 6, 2019 at 9:16 AM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> Hi,
>
> Any thoughts on this? I would guess every system with active databases
> would need to address this somehow?
>
>
> --
> Janne
>
> On Fri, Apr 5, 2019 at 3:46 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >
> > Hi,
> >
> > I've setup an android based mobile device with pretty complete ima/evm
> > setup that covers just about all the standard use cases (imasig based
> > filesystems, ota support, factory reset support etc). All that is fine
> > and ima runs like a clock.
> >
> > Since this is a mobile device, running out of battery or getting shot
> > in the head by something is always a realistic option. The random
> > resets seem to be leading into random appraisal failures as android
> > seems to be keeping surprisingly many files constantly open for
> > writing. So many actually, that I feel somewhat uneasy starting to
> > whitelist these files from the ima policy. That sounds like a viable
> > route only when it comes to the log files as those files primarily
> > move data only one way.
> >
> > Now, is there any prior art on this how to make this work right? The
> > improvements that I can instantly think of are,
> > 1) whitelist everything that can be,
> > 2) reduce the vfs flush delays,
> > 3) make it detect the reset condition and fix the known files when
> > that happened. Unsafe and requires a patch (but that seems easy).
> >
> > Anything else?
> >
> >
> > --
> > Janne

Re: appraisal reset safety

[Mimi Zohar]

On Mon, 2019-04-08 at 12:22 +0300, Janne Karhunen wrote:
> Hi,
> 
> Hmm, looks to me ima_update_xattr seems to be kicking in only from the
> appraisal failure when in fix mode or via fput() delayed work item.
> So, no sync() or anything like that will ever help and there is
> nothing listening on the i_version updates. Moreover, there is no
> integrity hook for write() or sync() to put such update in. Uh. I was
> under impression it would somehow see the interim file updates, but I
> guess no. Fundamental misunderstanding from my point of view how this
> thing works, duh.

The question of how much/how little to measure/appraise/audit is based
on policy and affects the integrity of the system and its performance.
 Detecting and updating the file hash each time the file changes would
have major performance repercussions.  Even that wouldn't solve the
problem, as the file change is in cache.  Writing the file hash as an
xattr and making the file change persistent needs to be coordinated,
probably at the filesystem level.

Mimi

Re: appraisal reset safety

[Janne Karhunen]

On Mon, Apr 8, 2019 at 4:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> > Hmm, looks to me ima_update_xattr seems to be kicking in only from the
> > appraisal failure when in fix mode or via fput() delayed work item.
> > So, no sync() or anything like that will ever help and there is
> > nothing listening on the i_version updates. Moreover, there is no
> > integrity hook for write() or sync() to put such update in. Uh. I was
> > under impression it would somehow see the interim file updates, but I
> > guess no. Fundamental misunderstanding from my point of view how this
> > thing works, duh.
>
> The question of how much/how little to measure/appraise/audit is based
> on policy and affects the integrity of the system and its performance.
>  Detecting and updating the file hash each time the file changes would
> have major performance repercussions.  Even that wouldn't solve the
> problem, as the file change is in cache.  Writing the file hash as an
> xattr and making the file change persistent needs to be coordinated,
> probably at the filesystem level.

Thanks. I was somehow under the impression that i_version walks the
inode revisions as they occur on the filesystem and that ima is
somehow magically in sync with those revisions. I guess that was naive
thing to expect..

Since I can't leave the device in non-functioning state, I added
following logic to my stack:
- detect the crash,
- add 'appraise_type=fixcrash' to ima,
- load few extra policy rules when a crash is detected to allow subset
of the hashes to be fixed.

If this is of any interest for you, I can send the patch for review.
Most certainly this is not a perfectly secure thing to do, but it is
better than bricking the device just because the battery died or the
kernel crashed :-/

Thanks again,


--
Janne

Re: appraisal reset safety

[Janne Karhunen]

On Mon, Apr 8, 2019 at 4:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> The question of how much/how little to measure/appraise/audit is based
> on policy and affects the integrity of the system and its performance.
>  Detecting and updating the file hash each time the file changes would
> have major performance repercussions.  Even that wouldn't solve the
> problem, as the file change is in cache.  Writing the file hash as an
> xattr and making the file change persistent needs to be coordinated,
> probably at the filesystem level.

As an experiment, I will add 'ima_file_update' function and call it
from few strategic spots (such as vfs write) and see how far that can
go removing the crash-recovery band-aid.

If the hash is in sync with the latest write, there is at least some
hope of recovery since the emergency sync on crash should flush this
data along the rest of it (I think). If this works, at least it will
give an option to use ima relatively safely given that you are aware
of it.


--
Janne

Re: appraisal reset safety

[Janne Karhunen]

On Tue, Apr 9, 2019 at 10:58 AM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> On Mon, Apr 8, 2019 at 4:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> > The question of how much/how little to measure/appraise/audit is based
> > on policy and affects the integrity of the system and its performance.
> >  Detecting and updating the file hash each time the file changes would
> > have major performance repercussions.  Even that wouldn't solve the
> > problem, as the file change is in cache.  Writing the file hash as an
> > xattr and making the file change persistent needs to be coordinated,
> > probably at the filesystem level.
>
> As an experiment, I will add 'ima_file_update' function and call it
> from few strategic spots (such as vfs write) and see how far that can
> go removing the crash-recovery band-aid.

Adding ima_file_update in there seems to work fine and things stay
nicely up to date. It is certainly heavy, but maybe this is only
needed when sync() is intentionally being called for the fd?

void ima_file_update(struct file *file)
{
        struct inode *inode = file_inode(file);
        struct integrity_iint_cache *iint;

        if (!ima_policy_flag || !S_ISREG(inode->i_mode))
                return;

        iint = integrity_iint_find(inode);
        if (!iint)
                return;

        iint->flags &= ~IMA_COLLECTED;
        ima_update_xattr(iint, file);
}

It would take an additional integrity hook, of course.


--
Janne

Re: appraisal reset safety

[Mimi Zohar]

On Tue, 2019-04-09 at 14:46 +0300, Janne Karhunen wrote:
> On Tue, Apr 9, 2019 at 10:58 AM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> >
> > On Mon, Apr 8, 2019 at 4:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > > The question of how much/how little to measure/appraise/audit is based
> > > on policy and affects the integrity of the system and its performance.
> > >  Detecting and updating the file hash each time the file changes would
> > > have major performance repercussions.  Even that wouldn't solve the
> > > problem, as the file change is in cache.  Writing the file hash as an
> > > xattr and making the file change persistent needs to be coordinated,
> > > probably at the filesystem level.
> >
> > As an experiment, I will add 'ima_file_update' function and call it
> > from few strategic spots (such as vfs write) and see how far that can
> > go removing the crash-recovery band-aid.

Remember somehow EVM will need to be updated as well for it to work.

> Adding ima_file_update in there seems to work fine and things stay
> nicely up to date. It is certainly heavy, but maybe this is only
> needed when sync() is intentionally being called for the fd?

I was thinking the same thing.
> 
> void ima_file_update(struct file *file)
> {
>         struct inode *inode = file_inode(file);
>         struct integrity_iint_cache *iint;
> 
>         if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>                 return;
> 
>         iint = integrity_iint_find(inode);
>         if (!iint)
>                 return;
> 
>         iint->flags &= ~IMA_COLLECTED;
>         ima_update_xattr(iint, file);
> }
> 

I would think there needs to be some locking here.  

> It would take an additional integrity hook, of course.

That's fine.

Mimi

Re: appraisal reset safety

[Janne Karhunen]

On Tue, Apr 9, 2019 at 3:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:

> > > As an experiment, I will add 'ima_file_update' function and call it
> > > from few strategic spots (such as vfs write) and see how far that can
> > > go removing the crash-recovery band-aid.
>
> Remember somehow EVM will need to be updated as well for it to work.

I will test some more, seemed to work fine under my quick trials.


> > Adding ima_file_update in there seems to work fine and things stay
> > nicely up to date. It is certainly heavy, but maybe this is only
> > needed when sync() is intentionally being called for the fd?
>
> I was thinking the same thing.

Right, so now if the application is designed correctly the guaranteed
appraisal failure caused by the kernel crash goes to 99.9% (or higher)
reliability. Big improvement, even if not perfect, IMHO..


> > void ima_file_update(struct file *file)
> > {
> >         struct inode *inode = file_inode(file);
> >         struct integrity_iint_cache *iint;
> >
> >         if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> >                 return;
> >
> >         iint = integrity_iint_find(inode);
> >         if (!iint)
> >                 return;
> >
> >         iint->flags &= ~IMA_COLLECTED;
> >         ima_update_xattr(iint, file);
> > }
> >
>
> I would think there needs to be some locking here.
>
> > It would take an additional integrity hook, of course.
>
> That's fine.

Great, I will work up a proper patch and check the locking.


--
Janne

Re: appraisal reset safety

[Mimi Zohar]

On Tue, 2019-04-09 at 15:25 +0300, Janne Karhunen wrote:
> On Tue, Apr 9, 2019 at 3:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> > > It would take an additional integrity hook, of course.
> >
> > That's fine.
> 
> Great, I will work up a proper patch and check the locking.

Thanks!

Mimi

Re: appraisal reset safety

[Janne Karhunen]

On Tue, Apr 9, 2019 at 3:33 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-04-09 at 15:25 +0300, Janne Karhunen wrote:
> > On Tue, Apr 9, 2019 at 3:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > > > It would take an additional integrity hook, of course.
> > >
> > > That's fine.
> >
> > Great, I will work up a proper patch and check the locking.
>
> Thanks!

Sent for review. I have tested it with arm8l and x86_64 qemu/kvm setup
with both appraisal & fix modes. Hashes seem to update correctly as
the syncs are done and I can't see any obvious smoke coming out from
anywhere..

As for crash testing, I will try to make many runs today. Based on my
understanding it should be pretty good, certainly battery outage or
just plain crash should have quite low probability to manage to tear a
single inode update halfway through. Besides, if that happens the fact
that IMA detects a file corruption is only a good thing. Broken files
are not allowed to be read, intact files are fine..


--
Janne