Date:	Fri, 15 Mar 2013 11:41:51 -0400
From:	Vivek Goyal <vgoyal@...hat.com>
To:	"Kasatkin, Dmitry" <dmitry.kasatkin@...el.com>
Cc:	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	linux kernel mailing list <linux-kernel@...r.kernel.org>,
	linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

On Thu, Mar 14, 2013 at 11:08:45PM +0200, Kasatkin, Dmitry wrote:
> On Thu, Mar 14, 2013 at 10:37 PM, Vivek Goyal <vgoyal@...hat.com> wrote:
> > On Thu, Mar 14, 2013 at 04:30:28PM -0400, Vivek Goyal wrote:
> >
> > [..]
> >> I thought explicitly using signature format is more intutive. Exporting
> >> signature version is not. I personally associate versioning with minor
> >> changes like addition of some fields etc.
> >
> > For instance, you might want to add some fields to  signature_v2_hdr down
> > the line. I think even after that change, it still remains "asymmetric
> > signature" just that structure size changes and there is additional
> > info. If there is versioning info assciated with signature type
> > ASYMMETRIC, we could simple bump it to 1.1 or whatever and keep the
> > version detail internal to ima/integrity subsystem.
> >
> 
> Yes, it will make some things cleaner.

So do you agree that every new signature format should have a new entry
and we can introduce new signature type for asymmetric signature.

> We recently discussed with Mimi how to extend IMA with memory locking and
> one of the ideas was to use a flag in the signature header to indicate
> that we need
> require memory locking. There we need new subversion of the asymmetric
> signature type.

I have few concerns here.

- Whether a file should be mem locked or not is not file property. It
  should not be stored in the file signature when file is being signed.
  Whether file should be locked or not depends on the caller and that
  in turn depends on the system environment.

  For example, one can create a system where whole of the user space is
  signed. (Extending secureboot fully to the user space). I think in that
  system one might not need to lock executables/files in memory.

- IMA should not be mmaping files in process address space. IMA does not
  know what should be mapped where. For example, executable files.
  Respective binary loader knows what sections of the file should be
  mapped at what address.

  IIUC, do_mmap_pgoff() will map file at first suitable available address.

  Secondly will IMA return with file mapped and locked or will unmap file
  before returning. If it does not unmap then it can conflict with binary
  loader who wants to map a section of file on a particular address which
  is already used. And if IMA unmaps the file before returning to caller,
  then mapping and mlocking has no benefit.

- Special hardcoded handling of BPRM_CHECK hook sounds like a kludge.

+	if (function == BPRM_CHECK && (iint->flags & IMA_DIGSIG))
+		iint->flags &= ~IMA_DONE_MASK;

  IMA in general has the issue of direct writes to disk. If we want to solve
  the caching issues, these should be solved in general and not be made hook
  specific.

  Even with ima_appraise_tcb policy in effect, there are vulnerabilities.
  ima_appraise_tcb will appraise only root files and a member of group
  "disk" can directly write to disk without being appraised and bypass all
  the caching logic.

- Memory locking will not solve the problem of knowing what did successful
  appraisal mean. User needs to know whether its rule got executed or not
  and based on that it can make a decision.

I really think that it is caller who should decide whether a file should
be locked or not. If there is a need, caller should first map full or part
file at appropriate address range and lock mapped pages in memory and
then call into IMA for signature verification.

> 
> Please have a look to 3 top patches.
> 
> http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/log/?h=working
> 
> It is a prototype to verify signature with arbitrary hash algorithm.

Thanks. I had a quick look. Hash related improvements look fine to me.

> Top patch also locks the memory if executable is verified and it has
> asymmetric signature type.

I have concerned with ima memory locking as mentioned above. We need to
have some discussion here first w.r.t what problem we are solving by
locking files on bprm_check hook and whether it is appropriate to do
it in IMA or not.

> This unconditional locking is just for development and we might use
> mlock bit from signature header.

As mentioned above, I have concerns here. Whether a file should be
memlocked or not is not a property of file or file signature.

> Notice, that patch reset appraisal status when we get BPRM check and
> there is a signature.

This is also shouds like a kludge. Why BPRM_CHECK only is special. Same
problem will exist with any other measurement hook, isn't it.

> 
> BTW, do you have a kernel repository somewhere.
> It is often easier to understand and discuss when seeing "complete"
> set of commits.

No, I don't have yet. The moment I have something (even in raw form), I 
will post patches as RFC. If things still don't work, I will setup a 
repository somewhere.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists