diff mbox series

src/t_attr_corruption: use security.capability instead of security.evm

Message ID 20190225191052.15342-1-jeffm@suse.com (mailing list archive)
State New, archived
Headers show
Series src/t_attr_corruption: use security.capability instead of security.evm | expand

Commit Message

Jeff Mahoney Feb. 25, 2019, 7:10 p.m. UTC 
From: Jeff Mahoney <jeffm@suse.com>

src/t_attr_corruption uses the security.evm extended attribute because
it sorts before security.posix_acl_access.  The security.evm attribute
is a formatted structure and when passed an uninitialized buffer, it
will fail with EPERM.

We see test failures like:
    --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
    +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
    @@ -1,2 +1,2 @@
     QA output created by 529
    -list attr: Numerical result out of range
    +set evm: Operation not permitted

This patch uses security.capability which also sorts where it needs to
do for the test and also accepts an unformatted buffer.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 src/t_attr_corruption.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Darrick J. Wong Feb. 25, 2019, 10:47 p.m. UTC | #1 
On Mon, Feb 25, 2019 at 02:10:52PM -0500, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> src/t_attr_corruption uses the security.evm extended attribute because
> it sorts before security.posix_acl_access.  The security.evm attribute
> is a formatted structure and when passed an uninitialized buffer, it
> will fail with EPERM.
> 
> We see test failures like:
>     --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
>     +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
>     @@ -1,2 +1,2 @@
>      QA output created by 529
>     -list attr: Numerical result out of range
>     +set evm: Operation not permitted
> 
> This patch uses security.capability which also sorts where it needs to
> do for the test and also accepts an unformatted buffer.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  src/t_attr_corruption.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> index f26611f9..0c229dbc 100644
> --- a/src/t_attr_corruption.c
> +++ b/src/t_attr_corruption.c
> @@ -76,7 +76,7 @@ int main(int argc, char *argv[])
>  	if (ret)
>  		die("set posix acl");
>  
> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> +	ret = fsetxattr(fd, "security.capability", buf, 1, 1);

This fails for me both with and without EVM configured into my kernel:

fsetxattr(3, "security.capability", "\3", 1, XATTR_CREATE) = -1 EINVAL (Invalid argument)

Judging from fs/xattr.c it looks as though security.capability also has
a defined format that's parsed by security/commoncap.c...

--D

>  	if (ret)
>  		die("set evm");
>  
> -- 
> 2.16.4
>
Darrick J. Wong Feb. 25, 2019, 11:26 p.m. UTC | #2 
On Mon, Feb 25, 2019 at 02:47:44PM -0800, Darrick J. Wong wrote:
> On Mon, Feb 25, 2019 at 02:10:52PM -0500, jeffm@suse.com wrote:
> > From: Jeff Mahoney <jeffm@suse.com>
> > 
> > src/t_attr_corruption uses the security.evm extended attribute because
> > it sorts before security.posix_acl_access.  The security.evm attribute
> > is a formatted structure and when passed an uninitialized buffer, it
> > will fail with EPERM.
> > 
> > We see test failures like:
> >     --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
> >     +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
> >     @@ -1,2 +1,2 @@
> >      QA output created by 529
> >     -list attr: Numerical result out of range
> >     +set evm: Operation not permitted
> > 
> > This patch uses security.capability which also sorts where it needs to
> > do for the test and also accepts an unformatted buffer.
> > 
> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > ---
> >  src/t_attr_corruption.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> > index f26611f9..0c229dbc 100644
> > --- a/src/t_attr_corruption.c
> > +++ b/src/t_attr_corruption.c
> > @@ -76,7 +76,7 @@ int main(int argc, char *argv[])
> >  	if (ret)
> >  		die("set posix acl");
> >  
> > -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> > +	ret = fsetxattr(fd, "security.capability", buf, 1, 1);
> 
> This fails for me both with and without EVM configured into my kernel:
> 
> fsetxattr(3, "security.capability", "\3", 1, XATTR_CREATE) = -1 EINVAL (Invalid argument)
> 
> Judging from fs/xattr.c it looks as though security.capability also has
> a defined format that's parsed by security/commoncap.c...

And now that I've figured out how to reproduce the xfs bug without a
second attribute, NAK to this and I'll send a more complete fix shortly.

--D

> --D
> 
> >  	if (ret)
> >  		die("set evm");
> >  
> > -- 
> > 2.16.4
> >
Jeff Mahoney Feb. 26, 2019, 1:39 a.m. UTC | #3 
On 2/25/19 5:47 PM, Darrick J. Wong wrote:
> On Mon, Feb 25, 2019 at 02:10:52PM -0500, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> src/t_attr_corruption uses the security.evm extended attribute because
>> it sorts before security.posix_acl_access.  The security.evm attribute
>> is a formatted structure and when passed an uninitialized buffer, it
>> will fail with EPERM.
>>
>> We see test failures like:
>>     --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
>>     +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
>>     @@ -1,2 +1,2 @@
>>      QA output created by 529
>>     -list attr: Numerical result out of range
>>     +set evm: Operation not permitted
>>
>> This patch uses security.capability which also sorts where it needs to
>> do for the test and also accepts an unformatted buffer.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  src/t_attr_corruption.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
>> index f26611f9..0c229dbc 100644
>> --- a/src/t_attr_corruption.c
>> +++ b/src/t_attr_corruption.c
>> @@ -76,7 +76,7 @@ int main(int argc, char *argv[])
>>  	if (ret)
>>  		die("set posix acl");
>>  
>> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
>> +	ret = fsetxattr(fd, "security.capability", buf, 1, 1);
> 
> This fails for me both with and without EVM configured into my kernel:
> 
> fsetxattr(3, "security.capability", "\3", 1, XATTR_CREATE) = -1 EINVAL (Invalid argument)
> 
> Judging from fs/xattr.c it looks as though security.capability also has
> a defined format that's parsed by security/commoncap.c...

Huh.  Ok.  That worked on my SLE12 SP3 host (4.4.x) but now that I test
it on a 5.0-rc, I get EINVAL too.  It looks like prior to v3 caps
(v4.14), validation only happened when they were loaded as part of
prepare_binprm.

-Jeff
Xiao Yang Feb. 26, 2019, 2:07 a.m. UTC | #4 
On 2019/02/26 7:26, Darrick J. Wong wrote:
> On Mon, Feb 25, 2019 at 02:47:44PM -0800, Darrick J. Wong wrote:
>> On Mon, Feb 25, 2019 at 02:10:52PM -0500, jeffm@suse.com wrote:
>>> From: Jeff Mahoney<jeffm@suse.com>
>>>
>>> src/t_attr_corruption uses the security.evm extended attribute because
>>> it sorts before security.posix_acl_access.  The security.evm attribute
>>> is a formatted structure and when passed an uninitialized buffer, it
>>> will fail with EPERM.
>>>
>>> We see test failures like:
>>>      --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
>>>      +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
>>>      @@ -1,2 +1,2 @@
>>>       QA output created by 529
>>>      -list attr: Numerical result out of range
>>>      +set evm: Operation not permitted
>>>
>>> This patch uses security.capability which also sorts where it needs to
>>> do for the test and also accepts an unformatted buffer.
>>>
>>> Signed-off-by: Jeff Mahoney<jeffm@suse.com>
>>> ---
>>>   src/t_attr_corruption.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
>>> index f26611f9..0c229dbc 100644
>>> --- a/src/t_attr_corruption.c
>>> +++ b/src/t_attr_corruption.c
>>> @@ -76,7 +76,7 @@ int main(int argc, char *argv[])
>>>   	if (ret)
>>>   		die("set posix acl");
>>>
>>> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
>>> +	ret = fsetxattr(fd, "security.capability", buf, 1, 1);
>> This fails for me both with and without EVM configured into my kernel:
>>
>> fsetxattr(3, "security.capability", "\3", 1, XATTR_CREATE) = -1 EINVAL (Invalid argument)
>>
>> Judging from fs/xattr.c it looks as though security.capability also has
>> a defined format that's parsed by security/commoncap.c...
> And now that I've figured out how to reproduce the xfs bug without a
> second attribute, NAK to this and I'll send a more complete fix shortly.
>
> --D
Hi Darrick, Jeff

It seems that fsetxattr() with an uninitialized buffer fell into the 
following xattr_data->type
check at security/integrity/evm/evm_main.c in kernel:
----------------------------------------------------------------------------------------------------------------------------
int evm_inode_setxattr(struct dentry *dentry, const char *xattr_name,
                        const void *xattr_value, size_t xattr_value_len)
{
...
         if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
                 if (!xattr_value_len)
                         return -EINVAL;
                 if (xattr_data->type != EVM_IMA_XATTR_DIGSIG &&
                     xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG)
                         return -EPERM;
         }
...
}
----------------------------------------------------------------------------------------------------------------------------

Perhaps, we can skip this check by setting security.evm attribute to 
special value(e.g.
EVM_IMA_XATTR_DIGSIG or EVM_XATTR_PORTABLE_DIGSIG) .  For example:
---------------------------------------------------------------------------------------------------------------------
diff --git a/src/t_attr_corruption.c b/root/t_attr_corruption.c
index 1fa5e41f..05ce5b46 100644
--- a/src/t_attr_corruption.c
+++ b/root/t_attr_corruption.c
@@ -61,6 +61,7 @@ int main(int argc, char *argv[])
         ssize_t sz;
         int fd;
         int ret;
+       int eval = 0x03;

         if (argc > 1) {
                 ret = chdir(argv[1]);
@@ -76,7 +77,7 @@ int main(int argc, char *argv[])
         if (ret)
                 die("set posix acl");

-       ret = fsetxattr(fd, "security.evm", buf, 1, 1);
+       ret = fsetxattr(fd, "security.evm", &eval, sizeof(eval), 1);
---------------------------------------------------------------------------------------------------------------------

Best Regards,
Xiao Yang
>> --D
>>
>>>   	if (ret)
>>>   		die("set evm");
>>>
>>> -- 
>>> 2.16.4
>>>
>
>
Xiao Yang Feb. 26, 2019, 3:33 a.m. UTC | #5 
On 2019/02/26 7:26, Darrick J. Wong wrote:
> On Mon, Feb 25, 2019 at 02:47:44PM -0800, Darrick J. Wong wrote:
>> On Mon, Feb 25, 2019 at 02:10:52PM -0500, jeffm@suse.com wrote:
>>> From: Jeff Mahoney<jeffm@suse.com>
>>>
>>> src/t_attr_corruption uses the security.evm extended attribute because
>>> it sorts before security.posix_acl_access.  The security.evm attribute
>>> is a formatted structure and when passed an uninitialized buffer, it
>>> will fail with EPERM.
>>>
>>> We see test failures like:
>>>      --- tests/generic/529.out2019-02-21 13:22:47.583406922 -0500
>>>      +++ /opt/xfstests/results//generic/529.out.bad 2019-02-21 13:57:31.967406922 -0500
>>>      @@ -1,2 +1,2 @@
>>>       QA output created by 529
>>>      -list attr: Numerical result out of range
>>>      +set evm: Operation not permitted
>>>
>>> This patch uses security.capability which also sorts where it needs to
>>> do for the test and also accepts an unformatted buffer.
>>>
>>> Signed-off-by: Jeff Mahoney<jeffm@suse.com>
>>> ---
>>>   src/t_attr_corruption.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
>>> index f26611f9..0c229dbc 100644
>>> --- a/src/t_attr_corruption.c
>>> +++ b/src/t_attr_corruption.c
>>> @@ -76,7 +76,7 @@ int main(int argc, char *argv[])
>>>   	if (ret)
>>>   		die("set posix acl");
>>>
>>> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
>>> +	ret = fsetxattr(fd, "security.capability", buf, 1, 1);
>> This fails for me both with and without EVM configured into my kernel:
>>
>> fsetxattr(3, "security.capability", "\3", 1, XATTR_CREATE) = -1 EINVAL (Invalid argument)
>>
>> Judging from fs/xattr.c it looks as though security.capability also has
>> a defined format that's parsed by security/commoncap.c...
> And now that I've figured out how to reproduce the xfs bug without a
> second attribute, NAK to this and I'll send a more complete fix shortly.
>
> --D
Hi Darrick,

Sorry, i missed this comment.  Your v2 patch is better and simpler to me.

Best Regards,
Xiao Yang
>> --D
>>
>>>   	if (ret)
>>>   		die("set evm");
>>>
>>> -- 
>>> 2.16.4
>>>
>
>
diff mbox series

Patch

diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
index f26611f9..0c229dbc 100644
--- a/src/t_attr_corruption.c
+++ b/src/t_attr_corruption.c
@@ -76,7 +76,7 @@  int main(int argc, char *argv[])
 	if (ret)
 		die("set posix acl");
 
-	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
+	ret = fsetxattr(fd, "security.capability", buf, 1, 1);
 	if (ret)
 		die("set evm");