diff mbox

[Utopic,Vivid] UBUNTU: SAUCE: (no-up): apparmor: fix mediation of fs unix sockets

Message ID 54F822FE.6080005@canonical.com
State New
Headers show

Commit Message

John Johansen March 5, 2015, 9:33 a.m. UTC 
Fix for out of tree AppArmor 3 patches.

BugLink: http://bugs.launchpad.net/bugs/1408833

Fix 2 issues around the mediation of file base unix domain sockets.
* Add auditing of deleted/shutdown file based unix domains sockets so
  that the denials can be correctly evalated.
* fix the permission request mask so that it is correct for the
  deleted/shutdown socket case.

Signed-off-by: John Johansen <john.johansen@canonical.com>

Comments

Andy Whitcroft March 10, 2015, 1:03 p.m. UTC | #1 
On Thu, Mar 05, 2015 at 01:33:50AM -0800, John Johansen wrote:
> Fix for out of tree AppArmor 3 patches.
> 
> BugLink: http://bugs.launchpad.net/bugs/1408833
> 
> Fix 2 issues around the mediation of file base unix domain sockets.
> * Add auditing of deleted/shutdown file based unix domains sockets so
>   that the denials can be correctly evalated.
> * fix the permission request mask so that it is correct for the
>   deleted/shutdown socket case.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
> index 73bd94d..62e7fd1 100644
> --- a/security/apparmor/af_unix.c
> +++ b/security/apparmor/af_unix.c
> @@ -31,32 +31,31 @@ static inline int unix_fs_perm(int op, u32 mask, struct aa_label *label,
>  	if (unconfined(label) || !LABEL_MEDIATES(label, AA_CLASS_FILE))
>  		return 0;
>  
> +	mask &= NET_FS_PERMS;
>  	if (!u->path.dentry) {
>  		struct path_cond cond = { };
>  		struct file_perms perms = { };
>  		struct aa_profile *profile;
>  
> -		/* socket path has been cleared because it is being shutdown */
> -		/* TODO: fix flags */
> -		if (!(flags & PATH_MEDIATE_DELETED))
> -			return -EACCES;
> -		/* Mediate at original socket location */
> -		/* TODO: ns disconnected paths */
> -		/* TODO: after switch to newer audit provide deleted/shutdown
> -		 *       message as part of audit info
> +		/* socket path has been cleared because it is being shutdown
> +		 * can only fall back to original sun_path request
>  		 */
>  		return fn_for_each_confined(label, profile,
> +			((flags | profile->path_flags) & PATH_MEDIATE_DELETED) ?
>  				__aa_path_perm(op, profile,
> -					       u->addr->name->sun_path,
> -					       mask, &cond, flags, &perms));
> +					       u->addr->name->sun_path, mask,
> +					       &cond, flags, &perms) :
> +				aa_audit_file(profile, &nullperms, op, mask,
> +					      u->addr->name->sun_path, NULL,
> +					      cond.uid, "Failed name lookup - "
> +					      "deleted entry", -EACCES));
>  	} else {
>  		/* the sunpath may not be valid for this ns so use the path */
>  		struct path_cond cond = { u->path.dentry->d_inode->i_uid,
>  					  u->path.dentry->d_inode->i_mode
>  		};
>  
> -		return aa_path_perm(op, label, &u->path, flags, mask & NET_FS_PERMS,
> -				    &cond);
> +		return aa_path_perm(op, label, &u->path, flags, mask, &cond);
>  	}
>  
>  	return 0;
> 
Looks to do what is claimed.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Stefan Bader March 10, 2015, 1:12 p.m. UTC | #2 
Appears to do what it claims, but more importantly has some successful testing.
Leann Ogasawara March 10, 2015, 1:18 p.m. UTC | #3 
Applied to Vivid.  Ack'ing for Utopic:

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

On Thu, 2015-03-05 at 01:33 -0800, John Johansen wrote:
> Fix for out of tree AppArmor 3 patches.
> 
> BugLink: http://bugs.launchpad.net/bugs/1408833
> 
> Fix 2 issues around the mediation of file base unix domain sockets.
> * Add auditing of deleted/shutdown file based unix domains sockets so
>   that the denials can be correctly evalated.
> * fix the permission request mask so that it is correct for the
>   deleted/shutdown socket case.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
> index 73bd94d..62e7fd1 100644
> --- a/security/apparmor/af_unix.c
> +++ b/security/apparmor/af_unix.c
> @@ -31,32 +31,31 @@ static inline int unix_fs_perm(int op, u32 mask, struct aa_label *label,
>  	if (unconfined(label) || !LABEL_MEDIATES(label, AA_CLASS_FILE))
>  		return 0;
>  
> +	mask &= NET_FS_PERMS;
>  	if (!u->path.dentry) {
>  		struct path_cond cond = { };
>  		struct file_perms perms = { };
>  		struct aa_profile *profile;
>  
> -		/* socket path has been cleared because it is being shutdown */
> -		/* TODO: fix flags */
> -		if (!(flags & PATH_MEDIATE_DELETED))
> -			return -EACCES;
> -		/* Mediate at original socket location */
> -		/* TODO: ns disconnected paths */
> -		/* TODO: after switch to newer audit provide deleted/shutdown
> -		 *       message as part of audit info
> +		/* socket path has been cleared because it is being shutdown
> +		 * can only fall back to original sun_path request
>  		 */
>  		return fn_for_each_confined(label, profile,
> +			((flags | profile->path_flags) & PATH_MEDIATE_DELETED) ?
>  				__aa_path_perm(op, profile,
> -					       u->addr->name->sun_path,
> -					       mask, &cond, flags, &perms));
> +					       u->addr->name->sun_path, mask,
> +					       &cond, flags, &perms) :
> +				aa_audit_file(profile, &nullperms, op, mask,
> +					      u->addr->name->sun_path, NULL,
> +					      cond.uid, "Failed name lookup - "
> +					      "deleted entry", -EACCES));
>  	} else {
>  		/* the sunpath may not be valid for this ns so use the path */
>  		struct path_cond cond = { u->path.dentry->d_inode->i_uid,
>  					  u->path.dentry->d_inode->i_mode
>  		};
>  
> -		return aa_path_perm(op, label, &u->path, flags, mask & NET_FS_PERMS,
> -				    &cond);
> +		return aa_path_perm(op, label, &u->path, flags, mask, &cond);
>  	}
>  
>  	return 0;
>
Andy Whitcroft March 10, 2015, 1:38 p.m. UTC | #4 
Applied to utopic.

-apw
Andy Whitcroft March 10, 2015, 1:44 p.m. UTC | #5 
Applied to utopic.

-apw
diff mbox

Patch

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index 73bd94d..62e7fd1 100644
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -31,32 +31,31 @@  static inline int unix_fs_perm(int op, u32 mask, struct aa_label *label,
 	if (unconfined(label) || !LABEL_MEDIATES(label, AA_CLASS_FILE))
 		return 0;
 
+	mask &= NET_FS_PERMS;
 	if (!u->path.dentry) {
 		struct path_cond cond = { };
 		struct file_perms perms = { };
 		struct aa_profile *profile;
 
-		/* socket path has been cleared because it is being shutdown */
-		/* TODO: fix flags */
-		if (!(flags & PATH_MEDIATE_DELETED))
-			return -EACCES;
-		/* Mediate at original socket location */
-		/* TODO: ns disconnected paths */
-		/* TODO: after switch to newer audit provide deleted/shutdown
-		 *       message as part of audit info
+		/* socket path has been cleared because it is being shutdown
+		 * can only fall back to original sun_path request
 		 */
 		return fn_for_each_confined(label, profile,
+			((flags | profile->path_flags) & PATH_MEDIATE_DELETED) ?
 				__aa_path_perm(op, profile,
-					       u->addr->name->sun_path,
-					       mask, &cond, flags, &perms));
+					       u->addr->name->sun_path, mask,
+					       &cond, flags, &perms) :
+				aa_audit_file(profile, &nullperms, op, mask,
+					      u->addr->name->sun_path, NULL,
+					      cond.uid, "Failed name lookup - "
+					      "deleted entry", -EACCES));
 	} else {
 		/* the sunpath may not be valid for this ns so use the path */
 		struct path_cond cond = { u->path.dentry->d_inode->i_uid,
 					  u->path.dentry->d_inode->i_mode
 		};
 
-		return aa_path_perm(op, label, &u->path, flags, mask & NET_FS_PERMS,
-				    &cond);
+		return aa_path_perm(op, label, &u->path, flags, mask, &cond);
 	}
 
 	return 0;