Message-ID: <555bf01bee4b4ea7a9bee658366d535a@huawei.com>
Date:   Fri, 30 Jul 2021 13:16:42 +0000
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mchehab+huawei@...nel.org" <mchehab+huawei@...nel.org>
CC:     "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC][PATCH v2 06/12] diglim: Interfaces - digest_list_add,
 digest_list_del

> From: Mimi Zohar [mailto:zohar@...ux.ibm.com]
> Sent: Friday, July 30, 2021 2:40 PM
> Hi Roberto,
> 
> On Fri, 2021-07-30 at 07:16 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@...ux.ibm.com]
> > > Sent: Thursday, July 29, 2021 11:21 PM
> > >
> > > On Mon, 2021-07-26 at 18:36 +0200, Roberto Sassu wrote:
> > > > /*
> > > > + * digest_list_read: read and parse the digest list from the path
> > > > + */
> > > > +static ssize_t digest_list_read(char *path, enum ops op)
> > > > +{
> > > > +       void *data = NULL;
> > > > +       char *datap;
> > > > +       size_t size;
> > > > +       u8 actions = 0;
> > > > +       struct file *file;
> > > > +       char event_name[NAME_MAX + 9 + 1];
> > > > +       u8 digest[IMA_MAX_DIGEST_SIZE] = { 0 };
> > > > +       enum hash_algo algo;
> > > > +       int rc, pathlen = strlen(path);
> > > > +
> > > > +       /* Remove \n. */
> > > > +       datap = path;
> > > > +       strsep(&datap, "\n");
> > > > +
> > > > +       file = filp_open(path, O_RDONLY, 0);
> > > > +       if (IS_ERR(file)) {
> > > > +               pr_err("unable to open file: %s (%ld)", path, PTR_ERR(file));
> > > > +               return PTR_ERR(file);
> > > > +       }
> > > > +
> > > > +       rc = kernel_read_file(file, 0, &data, INT_MAX, NULL,
> > > > +                             READING_DIGEST_LIST);
> > > > +       if (rc < 0) {
> > > > +               pr_err("unable to read file: %s (%d)", path, rc);
> > > > +               goto out;
> > > > +       }
> > > > +
> > > > +       size = rc;
> > > > +
> > > > +       snprintf(event_name, sizeof(event_name), "%s_file_%s",
> > > > +                op == DIGEST_LIST_ADD ? "add" : "del",
> > > > +                file_dentry(file)->d_name.name);
> > > > +
> > > > +       rc = ima_measure_critical_data("diglim", event_name, data, size,
> false,
> > > > +                                      digest, sizeof(digest));
> > > > +       if (rc < 0 && rc != -EEXIST)
> > > > +               goto out_vfree;
> > >
> > > The digest lists could easily be measured while reading the digest list
> > > file above in kernel_read_file().  What makes it "critical-data"?  In
> > > the SELinux case, the in memory SELinux policy is being measured and
> > > re-measured to make sure it hasn't been modified.  Is the digest list
> > > file data being measured more than once?
> >
> > Hi Mimi
> >
> > yes, the digest lists can be measured with kernel_read_file().
> > I didn't send the change yet, but I added a DIGEST_LIST_CHECK
> > hook mapped to READING_DIGEST_LIST, so that digest lists
> > can be easily measured or appraised.
> >
> > The point was that the digest of the digest list must be always
> > calculated, as it is added to the hash table. Instead of duplicating
> > the code, I preferred to use ima_measure_critical_data().
> >
> > The advantage is also that, if the use case is to just measure
> > digest lists, ima_measure_critical_data() could do both at the
> > same time.
> >
> > Digest lists can be seen as "critical data" in the sense that
> > they can affect the security decision on whether to grant
> > access to a file or not, assuming that an appropriate rule is
> > added in the IMA policy.
> 
> Of course the integrity of files containing the digest lists is
> important, but that doesn't make them "critical data".  If the
> integrity of these files is important, then the digest lists not only
> need to be measured, but they need to be signed and the resulting
> signature verified.  Without signature verification, there is no basis
> on which to trust the digest lists data.

The reason of storing the actions performed by IMA on the
digest lists helps to determine for which purpose they can be
used. If digest lists are used only for measurement purpose,
it should be sufficient that digest lists are measured. The
same applies for appraisal.

> Adding the kernel_read_file() "READING_DIGEST_LIST" support in IMA does
> not seem to be optional.  IMA would then be calculating the digest list
> file hash twice, once in kernel_read_file() and then, again, in
> ima_measure_critical_data().

I didn't include also this part: I retrieve the integrity_iint_cache for
the opened file descriptor and I get the flags from there. If the
IMA_MEASURED flag is set, it is not necessary to call also
ima_measure_critical_data().

> > > I understand that with your changes to ima_measure_critical_data(),
> > > which are now in next-integrity-testing branch, allow IMA to calculate
> > > the file data hash.
> >
> > Yes, correct. But actually there is another useful use case.
> > If digest lists are not in the format supported by the kernel,
> > the user space parser has to convert them before uploading
> > them to the kernel.
> >
> > ima_measure_critical_data() would in this case measure
> > the converted digest list (it is written directly, without
> > sending the file path). It is easier to attest the result,
> > instead of determining whether the user space parser
> > produced the expected result (by checking the files it
> > read).
> 
> The application to properly convert the digest list file data into the
> appropriate format would need to be trusted.  I'm concerned that not
> requiring the converted data to be signed and the signature verified is
> introducing a new integrity gap.  Perhaps between an LSM policy,
> limiting which files may be read by the application, and an IMA policy,
> requiring all files read by this application to be measured and the
> signature verified, this integrity gap could be averted.

It is the weakest point in the chain, yes. Relying on existing LSMs
didn't seem to me a good idea, as:
- a new policy must be installed
- we must be sure that the policy is really enforced
- we need to support different LSMs (SELinux for Fedora,
  Apparmor for SUSE)
- there might be no LSM we can rely on

For these reasons, I developed a new LSM. Its purpose is to
identify the user space parser and for each file it opens, ensure
that the file has been measured or appraised by IMA. If one of
these actions are missing, it will not be set in the digest list the
user space parser uploads to the kernel (which means that IMA
will ignore the digest list for that specific action).

> "critical data", in this context, should probably be used for verifying
> the in memory file digests and other state information haven't been
> compromised.

Actually, this is what we are doing currently. To keep the
implementation simple, once the file or the buffer are uploaded
to the kernel, they will not be modified, just accessed through
the indexes.

I could send the second part of the patch set, so that it becomes
more clear how digest lists are used by IMA and how the
integrity gap is filled.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> thanks,
> 
> Mimi

Powered by blists - more mailing lists