[RFC PATCH] integrity: Use a new type for asymmetric signature

[RFC PATCH] integrity: Use a new type for asymmetric signature

[Vivek Goyal]

Hi Dmitry/Mimi,

Here is an RFC patch. I am playing with exporting some functions from
ima/integrity and make reuse of IMA signature format and reuse of some
of IMA verification code.

One of the things required is that caller wants trusts only certain
type of signatures. For example, it might not trust DIGEST or HMAC
but might trust only digital signatures. So caller needs to know what
kind of signature are stored in IMA security attribute (if any) and
decide what to do.

Currently there seem to be two types of digital signatures. Old one and
that is RSA and new one which is being called asymmetric. Right now they
both fall in the categorty of EVM_IMA_XATTR_DIGSIG and one differentiates
between two using signature version. Version 1 is old type and version 2
is new type.

How about asymmetric signature using a new type say
EVM_IMA_XATTR_DIGSIG_ASYMMETRIC. And version numbering can be used for
structure variation with-in signature type.

This can allow caller to differentiate between two kinds of digital
signatures as understood by IMA.  And calling subsystems will call into
ima/integrity for verification only if digital signatures are of certain 
type.

asymmetric support has gone in just now. Before it becomes an ABI, it
might be worth to discuss it.

Yet-to-by-signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/integrity/digsig.c           |   11 +++++++----
 security/integrity/evm/evm_main.c     |    4 +++-
 security/integrity/ima/ima_appraise.c |    7 +++++--
 security/integrity/integrity.h        |    9 ++++++---
 4 files changed, 21 insertions(+), 10 deletions(-)

Index: linux-2.6/security/integrity/integrity.h
===================================================================
--- linux-2.6.orig/security/integrity/integrity.h	2013-03-14 13:44:30.000000000 -0400
+++ linux-2.6/security/integrity/integrity.h	2013-03-14 14:02:43.404474646 -0400
@@ -63,6 +63,7 @@ enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
 	EVM_XATTR_HMAC,
 	EVM_IMA_XATTR_DIGSIG,
+	EVM_IMA_XATTR_DIGSIG_ASYMMETRIC,
 };
 
 struct evm_ima_xattr_data {
@@ -98,12 +99,14 @@ struct integrity_iint_cache *integrity_i
 
 #ifdef CONFIG_INTEGRITY_SIGNATURE
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-					const char *digest, int digestlen);
+int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+	const unsigned int id, const char *sig, int siglen,
+	const char *digest, int digestlen);
 
 #else
 
-static inline int integrity_digsig_verify(const unsigned int id,
+static inline int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+					  const unsigned int id,
 					  const char *sig, int siglen,
 					  const char *digest, int digestlen)
 {
Index: linux-2.6/security/integrity/ima/ima_appraise.c
===================================================================
--- linux-2.6.orig/security/integrity/ima/ima_appraise.c	2013-03-14 13:44:30.000000000 -0400
+++ linux-2.6/security/integrity/ima/ima_appraise.c	2013-03-14 14:00:06.027469811 -0400
@@ -188,8 +188,10 @@ int ima_appraise_measurement(int func, s
 		status = INTEGRITY_PASS;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		iint->flags |= IMA_DIGSIG;
-		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+		rc = integrity_digsig_verify(xattr_value->type,
+					     INTEGRITY_KEYRING_IMA,
 					     xattr_value->digest, rc - 1,
 					     iint->ima_xattr.digest,
 					     IMA_DIGEST_SIZE);
@@ -210,7 +212,8 @@ out:
 	if (status != INTEGRITY_PASS) {
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
-		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+		     (xattr_value->type != EVM_IMA_XATTR_DIGSIG &&
+		      xattr_value->type != EVM_IMA_XATTR_DIGSIG_ASYMMETRIC))) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
 		}
Index: linux-2.6/security/integrity/evm/evm_main.c
===================================================================
--- linux-2.6.orig/security/integrity/evm/evm_main.c	2013-03-14 09:51:46.000000000 -0400
+++ linux-2.6/security/integrity/evm/evm_main.c	2013-03-14 14:00:52.265471232 -0400
@@ -134,11 +134,13 @@ static enum integrity_status evm_verify_
 			rc = -EINVAL;
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
 				xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
+		rc = integrity_digsig_verify(xattr_data->type,
+					INTEGRITY_KEYRING_EVM,
 					xattr_data->digest, xattr_len,
 					calc.digest, sizeof(calc.digest));
 		if (!rc) {
Index: linux-2.6/security/integrity/digsig.c
===================================================================
--- linux-2.6.orig/security/integrity/digsig.c	2013-03-12 15:06:54.000000000 -0400
+++ linux-2.6/security/integrity/digsig.c	2013-03-14 14:06:53.721482335 -0400
@@ -27,7 +27,8 @@ static const char *keyring_name[INTEGRIT
 	"_ima",
 };
 
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
+			const unsigned int id, const char *sig, int siglen,
 					const char *digest, int digestlen)
 {
 	if (id >= INTEGRITY_KEYRING_MAX)
@@ -44,13 +45,15 @@ int integrity_digsig_verify(const unsign
 		}
 	}
 
-	switch (sig[0]) {
-	case 1:
+	switch (sig_type) {
+	case EVM_IMA_XATTR_DIGSIG:
 		return digsig_verify(keyring[id], sig, siglen,
 				     digest, digestlen);
-	case 2:
+	case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
 		return asymmetric_verify(keyring[id], sig, siglen,
 					 digest, digestlen);
+	default:
+		break;
 	}
 
 	return -EOPNOTSUPP;

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Kasatkin, Dmitry]

On Thu, Mar 14, 2013 at 8:28 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> Hi Dmitry/Mimi,
>
> Here is an RFC patch. I am playing with exporting some functions from
> ima/integrity and make reuse of IMA signature format and reuse of some
> of IMA verification code.
>
> One of the things required is that caller wants trusts only certain
> type of signatures. For example, it might not trust DIGEST or HMAC
> but might trust only digital signatures. So caller needs to know what
> kind of signature are stored in IMA security attribute (if any) and
> decide what to do.
>
> Currently there seem to be two types of digital signatures. Old one and
> that is RSA and new one which is being called asymmetric. Right now they
> both fall in the categorty of EVM_IMA_XATTR_DIGSIG and one differentiates
> between two using signature version. Version 1 is old type and version 2
> is new type.
>
> How about asymmetric signature using a new type say
> EVM_IMA_XATTR_DIGSIG_ASYMMETRIC. And version numbering can be used for
> structure variation with-in signature type.
>

Hello Vivek,

This was exactly the way how I initially implemented asymmetric key support.
But then thought that we might have new versions of signature formats
in the future and there is
not point to create new xattr type for every signature format.

What prevents just using of signature version?

Thanks,

Dmitry

> This can allow caller to differentiate between two kinds of digital
> signatures as understood by IMA.  And calling subsystems will call into
> ima/integrity for verification only if digital signatures are of certain
> type.
>
> asymmetric support has gone in just now. Before it becomes an ABI, it
> might be worth to discuss it.
>
> Yet-to-by-signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  security/integrity/digsig.c           |   11 +++++++----
>  security/integrity/evm/evm_main.c     |    4 +++-
>  security/integrity/ima/ima_appraise.c |    7 +++++--
>  security/integrity/integrity.h        |    9 ++++++---
>  4 files changed, 21 insertions(+), 10 deletions(-)
>
> Index: linux-2.6/security/integrity/integrity.h
> ===================================================================
> --- linux-2.6.orig/security/integrity/integrity.h       2013-03-14 13:44:30.000000000 -0400
> +++ linux-2.6/security/integrity/integrity.h    2013-03-14 14:02:43.404474646 -0400
> @@ -63,6 +63,7 @@ enum evm_ima_xattr_type {
>         IMA_XATTR_DIGEST = 0x01,
>         EVM_XATTR_HMAC,
>         EVM_IMA_XATTR_DIGSIG,
> +       EVM_IMA_XATTR_DIGSIG_ASYMMETRIC,
>  };
>
>  struct evm_ima_xattr_data {
> @@ -98,12 +99,14 @@ struct integrity_iint_cache *integrity_i
>
>  #ifdef CONFIG_INTEGRITY_SIGNATURE
>
> -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> -                                       const char *digest, int digestlen);
> +int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
> +       const unsigned int id, const char *sig, int siglen,
> +       const char *digest, int digestlen);
>
>  #else
>
> -static inline int integrity_digsig_verify(const unsigned int id,
> +static inline int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
> +                                         const unsigned int id,
>                                           const char *sig, int siglen,
>                                           const char *digest, int digestlen)
>  {
> Index: linux-2.6/security/integrity/ima/ima_appraise.c
> ===================================================================
> --- linux-2.6.orig/security/integrity/ima/ima_appraise.c        2013-03-14 13:44:30.000000000 -0400
> +++ linux-2.6/security/integrity/ima/ima_appraise.c     2013-03-14 14:00:06.027469811 -0400
> @@ -188,8 +188,10 @@ int ima_appraise_measurement(int func, s
>                 status = INTEGRITY_PASS;
>                 break;
>         case EVM_IMA_XATTR_DIGSIG:
> +       case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
>                 iint->flags |= IMA_DIGSIG;
> -               rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> +               rc = integrity_digsig_verify(xattr_value->type,
> +                                            INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
> @@ -210,7 +212,8 @@ out:
>         if (status != INTEGRITY_PASS) {
>                 if ((ima_appraise & IMA_APPRAISE_FIX) &&
>                     (!xattr_value ||
> -                    xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> +                    (xattr_value->type != EVM_IMA_XATTR_DIGSIG &&
> +                     xattr_value->type != EVM_IMA_XATTR_DIGSIG_ASYMMETRIC))) {
>                         if (!ima_fix_xattr(dentry, iint))
>                                 status = INTEGRITY_PASS;
>                 }
> Index: linux-2.6/security/integrity/evm/evm_main.c
> ===================================================================
> --- linux-2.6.orig/security/integrity/evm/evm_main.c    2013-03-14 09:51:46.000000000 -0400
> +++ linux-2.6/security/integrity/evm/evm_main.c 2013-03-14 14:00:52.265471232 -0400
> @@ -134,11 +134,13 @@ static enum integrity_status evm_verify_
>                         rc = -EINVAL;
>                 break;
>         case EVM_IMA_XATTR_DIGSIG:
> +       case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
>                 rc = evm_calc_hash(dentry, xattr_name, xattr_value,
>                                 xattr_value_len, calc.digest);
>                 if (rc)
>                         break;
> -               rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
> +               rc = integrity_digsig_verify(xattr_data->type,
> +                                       INTEGRITY_KEYRING_EVM,
>                                         xattr_data->digest, xattr_len,
>                                         calc.digest, sizeof(calc.digest));
>                 if (!rc) {
> Index: linux-2.6/security/integrity/digsig.c
> ===================================================================
> --- linux-2.6.orig/security/integrity/digsig.c  2013-03-12 15:06:54.000000000 -0400
> +++ linux-2.6/security/integrity/digsig.c       2013-03-14 14:06:53.721482335 -0400
> @@ -27,7 +27,8 @@ static const char *keyring_name[INTEGRIT
>         "_ima",
>  };
>
> -int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> +int integrity_digsig_verify(enum evm_ima_xattr_type sig_type,
> +                       const unsigned int id, const char *sig, int siglen,
>                                         const char *digest, int digestlen)
>  {
>         if (id >= INTEGRITY_KEYRING_MAX)
> @@ -44,13 +45,15 @@ int integrity_digsig_verify(const unsign
>                 }
>         }
>
> -       switch (sig[0]) {
> -       case 1:
> +       switch (sig_type) {
> +       case EVM_IMA_XATTR_DIGSIG:
>                 return digsig_verify(keyring[id], sig, siglen,
>                                      digest, digestlen);
> -       case 2:
> +       case EVM_IMA_XATTR_DIGSIG_ASYMMETRIC:
>                 return asymmetric_verify(keyring[id], sig, siglen,
>                                          digest, digestlen);
> +       default:
> +               break;
>         }
>
>         return -EOPNOTSUPP;

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Vivek Goyal]

On Thu, Mar 14, 2013 at 10:05:59PM +0200, Kasatkin, Dmitry wrote:
> On Thu, Mar 14, 2013 at 8:28 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > Hi Dmitry/Mimi,
> >
> > Here is an RFC patch. I am playing with exporting some functions from
> > ima/integrity and make reuse of IMA signature format and reuse of some
> > of IMA verification code.
> >
> > One of the things required is that caller wants trusts only certain
> > type of signatures. For example, it might not trust DIGEST or HMAC
> > but might trust only digital signatures. So caller needs to know what
> > kind of signature are stored in IMA security attribute (if any) and
> > decide what to do.
> >
> > Currently there seem to be two types of digital signatures. Old one and
> > that is RSA and new one which is being called asymmetric. Right now they
> > both fall in the categorty of EVM_IMA_XATTR_DIGSIG and one differentiates
> > between two using signature version. Version 1 is old type and version 2
> > is new type.
> >
> > How about asymmetric signature using a new type say
> > EVM_IMA_XATTR_DIGSIG_ASYMMETRIC. And version numbering can be used for
> > structure variation with-in signature type.
> >
> 
> Hello Vivek,
> 
> This was exactly the way how I initially implemented asymmetric key support.
> But then thought that we might have new versions of signature formats
> in the future and there is
> not point to create new xattr type for every signature format.
> 
> What prevents just using of signature version?

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. Exporting that to caller sounds
odd to me.

If I export signature version separately, then it becomes two calls. First
call for signature type and second call for signature version. As not all
formats have versions, caller also needs to be aware of that. This is like
exposing too much internal detail to caller.

Hence I felt exposing seprate signature format as separate type becomes
easy for the caller of the API. Even in user space we don't ask user the
version of signature to be generated. What is more intutive is what kind
of signature one wants to generate. Versioning info sounds more like an
internal detail to the subsystem.

Having said that, I am not very sure what's the fundamental difference
between two digital signature formats. (Exising RSA one and new one 
which is being called ASYMMETRIC). And how many possible formats are
there. What's wrong with keep on increasing the enum value as new
signature formats show up. 

Thanks
Vivek

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Vivek Goyal]

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.

Thanks
Vivek

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Kasatkin, Dmitry]

On Thu, Mar 14, 2013 at 10:37 PM, Vivek Goyal <vgoyal@redhat.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.
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.

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.
Top patch also locks the memory if executable is verified and it has
asymmetric signature type.
This unconditional locking is just for development and we might use
mlock bit from signature header.
Notice, that patch reset appraisal status when we get BPRM check and
there is a signature.

BTW, do you have a kernel repository somewhere.
It is often easier to understand and discuss when seeing "complete"
set of commits.
If patch is taken out of context, it might be a waste of time to
discuss unclear things.

- Dmitry


> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Vivek Goyal]

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@redhat.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

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Kasatkin, Dmitry]

On Fri, Mar 15, 2013 at 5:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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@redhat.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.
>

Hi Vivek,

I was/am a bit busy with responding.
I will do it asap.

Thanks,
Dmitry

>> 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

Re: [RFC PATCH] integrity: Use a new type for asymmetric signature

[Vivek Goyal]

On Wed, Mar 20, 2013 at 09:57:57AM +0200, Kasatkin, Dmitry wrote:
> On Fri, Mar 15, 2013 at 5:41 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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@redhat.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.
> >
> 
> Hi Vivek,
> 
> I was/am a bit busy with responding.
> I will do it asap.

Hi Dmitry,

Sure. Only thing to keep in mind is that if you want to change it, we
better do it in 3.9. Otherwise changing it in 3.10 will be messy.

Thanks
Vivek