[SRU,xenial] UBUNTU: SAUCE: apparmor: flock mediation is not being, enforced on cache check
diff mbox series

Message ID b07a592d-bfff-528b-78a4-142dab4b1668@canonical.com
State New
Headers show
Series
  • [SRU,xenial] UBUNTU: SAUCE: apparmor: flock mediation is not being, enforced on cache check
Related show

Commit Message

John Johansen Aug. 5, 2019, 11:26 p.m. UTC
When an open file with cached permissions is checked for the flock
permission. The cache check fails and falls through to no error instead
of auditing, and returning an error.

For the fall through to do a permission check, so it will audit the
failed flock permission check.

BugLink: http://bugs.launchpad.net/bugs/1838090
BugLink: http://bugs.launchpad.net/bugs/1658219
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/file.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Tyler Hicks Aug. 9, 2019, 4:42 p.m. UTC | #1
On 2019-08-05 16:26:38, John Johansen wrote:
> When an open file with cached permissions is checked for the flock
> permission. The cache check fails and falls through to no error instead
> of auditing, and returning an error.
> 
> For the fall through to do a permission check, so it will audit the
> failed flock permission check.
> 
> BugLink: http://bugs.launchpad.net/bugs/1838090
> BugLink: http://bugs.launchpad.net/bugs/1658219
> Signed-off-by: John Johansen <john.johansen@canonical.com>

I tested this change with the upstream AppArmor regression tests with
positive results.

 Acked-by: Tyler Hicks <tyhicks@canonical.com>

Thanks!

Tyler

> ---
>  security/apparmor/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 69eed1c3e0d0..646d3effd0f9 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>  	error = fn_for_each_not_in_set(flabel, label, profile,
>  			profile_path_perm(op, profile, &file->f_path, buffer,
>  					  request, &cond, flags, &perms));
> -	if (denied) {
> +	if (denied && !error) {
>  		/* check every profile in file label that was not tested
>  		 * in the initial check above.
>  		 */
>  		/* TODO: cache full perms so this only happens because of
>  		 * conditionals */
>  		/* TODO: don't audit here */
> -		last_error(error,
> -			fn_for_each_not_in_set(label, flabel, profile,
> +		if (label == flabel)
> +			error = fn_for_each(label, profile,
> +				profile_path_perm(op, profile, &file->f_path,
> +						  buffer, request, &cond, flags,
> +						  &perms));
> +		else
> +			error = fn_for_each_not_in_set(label, flabel, profile,
>  				profile_path_perm(op, profile, &file->f_path,
>  						  buffer, request, &cond, flags,
> -						  &perms)));
> +						  &perms));
>  	}
>  	if (!error)
>  		update_file_ctx(file_ctx(file), label, request);
> -- 
> 2.17.1
>
Stefan Bader Aug. 12, 2019, 2:54 p.m. UTC | #2
On 06.08.19 01:26, John Johansen wrote:
> When an open file with cached permissions is checked for the flock
> permission. The cache check fails and falls through to no error instead
> of auditing, and returning an error.
> 
> For the fall through to do a permission check, so it will audit the
> failed flock permission check.
> 
> BugLink: http://bugs.launchpad.net/bugs/1838090
> BugLink: http://bugs.launchpad.net/bugs/1658219
> Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  security/apparmor/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 69eed1c3e0d0..646d3effd0f9 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>  	error = fn_for_each_not_in_set(flabel, label, profile,
>  			profile_path_perm(op, profile, &file->f_path, buffer,
>  					  request, &cond, flags, &perms));
> -	if (denied) {
> +	if (denied && !error) {
>  		/* check every profile in file label that was not tested
>  		 * in the initial check above.
>  		 */
>  		/* TODO: cache full perms so this only happens because of
>  		 * conditionals */
>  		/* TODO: don't audit here */
> -		last_error(error,
> -			fn_for_each_not_in_set(label, flabel, profile,
> +		if (label == flabel)
> +			error = fn_for_each(label, profile,
> +				profile_path_perm(op, profile, &file->f_path,
> +						  buffer, request, &cond, flags,
> +						  &perms));
> +		else
> +			error = fn_for_each_not_in_set(label, flabel, profile,
>  				profile_path_perm(op, profile, &file->f_path,
>  						  buffer, request, &cond, flags,
> -						  &perms)));
> +						  &perms));
>  	}
>  	if (!error)
>  		update_file_ctx(file_ctx(file), label, request);
>
Stefan Bader Aug. 12, 2019, 2:56 p.m. UTC | #3
On 06.08.19 01:26, John Johansen wrote:
> When an open file with cached permissions is checked for the flock
> permission. The cache check fails and falls through to no error instead
> of auditing, and returning an error.
> 
> For the fall through to do a permission check, so it will audit the
> failed flock permission check.
> 
> BugLink: http://bugs.launchpad.net/bugs/1838090
  ^ that one leads nowhere
> BugLink: https://bugs.launchpad.net/bugs/1658219
  ^change to https when committing
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 69eed1c3e0d0..646d3effd0f9 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>  	error = fn_for_each_not_in_set(flabel, label, profile,
>  			profile_path_perm(op, profile, &file->f_path, buffer,
>  					  request, &cond, flags, &perms));
> -	if (denied) {
> +	if (denied && !error) {
>  		/* check every profile in file label that was not tested
>  		 * in the initial check above.
>  		 */
>  		/* TODO: cache full perms so this only happens because of
>  		 * conditionals */
>  		/* TODO: don't audit here */
> -		last_error(error,
> -			fn_for_each_not_in_set(label, flabel, profile,
> +		if (label == flabel)
> +			error = fn_for_each(label, profile,
> +				profile_path_perm(op, profile, &file->f_path,
> +						  buffer, request, &cond, flags,
> +						  &perms));
> +		else
> +			error = fn_for_each_not_in_set(label, flabel, profile,
>  				profile_path_perm(op, profile, &file->f_path,
>  						  buffer, request, &cond, flags,
> -						  &perms)));
> +						  &perms));
>  	}
>  	if (!error)
>  		update_file_ctx(file_ctx(file), label, request);
>
Kleber Sacilotto de Souza Aug. 12, 2019, 2:56 p.m. UTC | #4
On 8/6/19 1:26 AM, John Johansen wrote:
> When an open file with cached permissions is checked for the flock
> permission. The cache check fails and falls through to no error instead
> of auditing, and returning an error.
> 
> For the fall through to do a permission check, so it will audit the
> failed flock permission check.
> 
> BugLink: http://bugs.launchpad.net/bugs/1838090

The above BugLink doesn't work. Is this bug report flagged as private?


Kleber

> BugLink: http://bugs.launchpad.net/bugs/1658219
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 69eed1c3e0d0..646d3effd0f9 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>  	error = fn_for_each_not_in_set(flabel, label, profile,
>  			profile_path_perm(op, profile, &file->f_path, buffer,
>  					  request, &cond, flags, &perms));
> -	if (denied) {
> +	if (denied && !error) {
>  		/* check every profile in file label that was not tested
>  		 * in the initial check above.
>  		 */
>  		/* TODO: cache full perms so this only happens because of
>  		 * conditionals */
>  		/* TODO: don't audit here */
> -		last_error(error,
> -			fn_for_each_not_in_set(label, flabel, profile,
> +		if (label == flabel)
> +			error = fn_for_each(label, profile,
> +				profile_path_perm(op, profile, &file->f_path,
> +						  buffer, request, &cond, flags,
> +						  &perms));
> +		else
> +			error = fn_for_each_not_in_set(label, flabel, profile,
>  				profile_path_perm(op, profile, &file->f_path,
>  						  buffer, request, &cond, flags,
> -						  &perms)));
> +						  &perms));
>  	}
>  	if (!error)
>  		update_file_ctx(file_ctx(file), label, request);
>
John Johansen Aug. 12, 2019, 9:59 p.m. UTC | #5
On 8/12/19 7:56 AM, Kleber Souza wrote:
> On 8/6/19 1:26 AM, John Johansen wrote:
>> When an open file with cached permissions is checked for the flock
>> permission. The cache check fails and falls through to no error instead
>> of auditing, and returning an error.
>>
>> For the fall through to do a permission check, so it will audit the
>> failed flock permission check.
>>
>> BugLink: http://bugs.launchpad.net/bugs/1838090
> 
> The above BugLink doesn't work. Is this bug report flagged as private?
> 
> 
yes it was marked private security

it is also marked now as a duplicate of 
https://bugs.launchpad.net/apparmor/+bug/1658219/

which is public so I have switched it to public as well


> Kleber
> 
>> BugLink: http://bugs.launchpad.net/bugs/1658219
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>> ---
>>  security/apparmor/file.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
>> index 69eed1c3e0d0..646d3effd0f9 100644
>> --- a/security/apparmor/file.c
>> +++ b/security/apparmor/file.c
>> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>>  	error = fn_for_each_not_in_set(flabel, label, profile,
>>  			profile_path_perm(op, profile, &file->f_path, buffer,
>>  					  request, &cond, flags, &perms));
>> -	if (denied) {
>> +	if (denied && !error) {
>>  		/* check every profile in file label that was not tested
>>  		 * in the initial check above.
>>  		 */
>>  		/* TODO: cache full perms so this only happens because of
>>  		 * conditionals */
>>  		/* TODO: don't audit here */
>> -		last_error(error,
>> -			fn_for_each_not_in_set(label, flabel, profile,
>> +		if (label == flabel)
>> +			error = fn_for_each(label, profile,
>> +				profile_path_perm(op, profile, &file->f_path,
>> +						  buffer, request, &cond, flags,
>> +						  &perms));
>> +		else
>> +			error = fn_for_each_not_in_set(label, flabel, profile,
>>  				profile_path_perm(op, profile, &file->f_path,
>>  						  buffer, request, &cond, flags,
>> -						  &perms)));
>> +						  &perms));
>>  	}
>>  	if (!error)
>>  		update_file_ctx(file_ctx(file), label, request);
>>
>
Khaled Elmously Aug. 13, 2019, 4:07 a.m. UTC | #6
On 2019-08-05 16:26:38 , John Johansen wrote:
> When an open file with cached permissions is checked for the flock
> permission. The cache check fails and falls through to no error instead
> of auditing, and returning an error.
> 
> For the fall through to do a permission check, so it will audit the
> failed flock permission check.
> 
> BugLink: http://bugs.launchpad.net/bugs/1838090
> BugLink: http://bugs.launchpad.net/bugs/1658219
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/file.c b/security/apparmor/file.c
> index 69eed1c3e0d0..646d3effd0f9 100644
> --- a/security/apparmor/file.c
> +++ b/security/apparmor/file.c
> @@ -536,18 +536,23 @@ static int __file_path_perm(const char *op, struct aa_label *label,
>  	error = fn_for_each_not_in_set(flabel, label, profile,
>  			profile_path_perm(op, profile, &file->f_path, buffer,
>  					  request, &cond, flags, &perms));
> -	if (denied) {
> +	if (denied && !error) {
>  		/* check every profile in file label that was not tested
>  		 * in the initial check above.
>  		 */
>  		/* TODO: cache full perms so this only happens because of
>  		 * conditionals */
>  		/* TODO: don't audit here */
> -		last_error(error,
> -			fn_for_each_not_in_set(label, flabel, profile,
> +		if (label == flabel)
> +			error = fn_for_each(label, profile,
> +				profile_path_perm(op, profile, &file->f_path,
> +						  buffer, request, &cond, flags,
> +						  &perms));
> +		else
> +			error = fn_for_each_not_in_set(label, flabel, profile,
>  				profile_path_perm(op, profile, &file->f_path,
>  						  buffer, request, &cond, flags,
> -						  &perms)));
> +						  &perms));
>  	}
>  	if (!error)
>  		update_file_ctx(file_ctx(file), label, request);
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Patch
diff mbox series

diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index 69eed1c3e0d0..646d3effd0f9 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -536,18 +536,23 @@  static int __file_path_perm(const char *op, struct aa_label *label,
 	error = fn_for_each_not_in_set(flabel, label, profile,
 			profile_path_perm(op, profile, &file->f_path, buffer,
 					  request, &cond, flags, &perms));
-	if (denied) {
+	if (denied && !error) {
 		/* check every profile in file label that was not tested
 		 * in the initial check above.
 		 */
 		/* TODO: cache full perms so this only happens because of
 		 * conditionals */
 		/* TODO: don't audit here */
-		last_error(error,
-			fn_for_each_not_in_set(label, flabel, profile,
+		if (label == flabel)
+			error = fn_for_each(label, profile,
+				profile_path_perm(op, profile, &file->f_path,
+						  buffer, request, &cond, flags,
+						  &perms));
+		else
+			error = fn_for_each_not_in_set(label, flabel, profile,
 				profile_path_perm(op, profile, &file->f_path,
 						  buffer, request, &cond, flags,
-						  &perms)));
+						  &perms));
 	}
 	if (!error)
 		update_file_ctx(file_ctx(file), label, request);