← Back to branch summary

~ubuntu-branches/ubuntu/wily/apparmor/wily

« back to all changes in this revision

Viewing changes to kernel-patches/for-mainline/apparmor-dir-rename-fix.diff

  • Committer: Bazaar Package Importer
  • Author(s): Kees Cook
  • Date: 2011-04-27 10:38:07 UTC
  • mfrom: (5.1.118 natty)
  • Revision ID: james.westby@ubuntu.com-20110427103807-ym3rhwys6o84ith0
Tags: 2.6.1-2
debian/copyright: clarify for some full organization names.

Show diffs side-by-side

added added

removed removed

Lines of Context:
kernel-patches/for-mainline/apparmor-dir-rename-fix.diff
1
 
Directory rename bugfix
2
 
 
3
 
Up until now, the profile permission checks for directory rename only
4
 
worked in most cases, and mostly be coincidence: when we did permission
5
 
checks, we ignored requests for MAY_WRITE and MAY_EXEC on directories
6
 
because else we would have to include x access on each directory that
7
 
should be accessed, and w access on each directory that files can be
8
 
created in or removed from.
9
 
 
10
 
We also ignored MAY_WRITE and MAY_EXEC for directory renames, and it was
11
 
only by sheer luck that most directory renames failed -- the only reason
12
 
why most they failed was because in most cases, the target file will
13
 
either not exist, or be a non-directory. If the target file exists and
14
 
is a directory, then it is sufficient to have r profile access in order
15
 
to rename directories, though.
16
 
 
17
 
Test case: create two empty directories d1 and d2 somewhere. Give a
18
 
process r access to that directory. Do a rename("d1", "d2") syscall, and
19
 
d2 will disappear. This will also fail with ENOTEMPTY if d2 is
20
 
non-empty. The expected result of course would be EPERM.
21
 
 
22
 
This issue may have been noticed before. Anyway, a fix is attached;
23
 
there are now also comments in the code that explain what's going on.
24
 
 
25
 
Caveat: there still is no way to to tell a directory traversal from the
26
 
access(2) system call, and so access("d1", W_OK) will still return 0
27
 
even if the profile does not allow w access to d1. This is mildly
28
 
annoying, but using access(2) for permission checking is insecure,
29
 
anyway.
30
 
 
31
 
Index: b/security/apparmor/lsm.c
32
 
===================================================================
33
 
--- a/security/apparmor/lsm.c
34
 
+++ b/security/apparmor/lsm.c
35
 
@@ -278,7 +278,7 @@ out:
36
 
 }
37
 
 
38
 
 static int aa_permission(struct inode *inode, struct dentry *dentry,
39
 
-                        struct vfsmount *mnt, int mask)
40
 
+                        struct vfsmount *mnt, int mask, int leaf)
41
 
 {
42
 
        int error = 0;
43
 
 
44
 
@@ -286,7 +286,7 @@ static int aa_permission(struct inode *i
45
 
                struct aa_profile *active = get_active_aa_profile();
46
 
 
47
 
                if (active)
48
 
-                       error = aa_perm(active, dentry, mnt, mask);
49
 
+                       error = aa_perm(active, dentry, mnt, mask, leaf);
50
 
                put_aa_profile(active);
51
 
        }
52
 
        return error;
53
 
@@ -295,7 +295,7 @@ static int aa_permission(struct inode *i
54
 
 static int apparmor_inode_create(struct inode *dir, struct dentry *dentry,
55
 
                                 struct vfsmount *mnt, int mask)
56
 
 {
57
 
-       return aa_permission(dir, dentry, mnt, MAY_WRITE);
58
 
+       return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
59
 
 }
60
 
 
61
 
 static int apparmor_inode_link(struct dentry *old_dentry,
62
 
@@ -325,19 +325,19 @@ static int apparmor_inode_unlink(struct 
63
 
                                 struct dentry *dentry,
64
 
                                 struct vfsmount *mnt)
65
 
 {
66
 
-       return aa_permission(dir, dentry, mnt, MAY_WRITE);
67
 
+       return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
68
 
 }
69
 
 
70
 
 static int apparmor_inode_symlink(struct inode *dir, struct dentry *dentry,
71
 
                                  struct vfsmount *mnt, const char *old_name)
72
 
 {
73
 
-       return aa_permission(dir, dentry, mnt, MAY_WRITE);
74
 
+       return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
75
 
 }
76
 
 
77
 
 static int apparmor_inode_mknod(struct inode *dir, struct dentry *dentry,
78
 
                                struct vfsmount *mnt, int mode, dev_t dev)
79
 
 {
80
 
-       return aa_permission(dir, dentry, mnt, MAY_WRITE);
81
 
+       return aa_permission(dir, dentry, mnt, MAY_WRITE, 1);
82
 
 }
83
 
 
84
 
 static int apparmor_inode_rename(struct inode *old_dir,
85
 
@@ -358,11 +358,11 @@ static int apparmor_inode_rename(struct 
86
 
        if (active) {
87
 
                if (old_mnt)
88
 
                        error = aa_perm(active, old_dentry, old_mnt,
89
 
-                                       MAY_READ|MAY_WRITE);
90
 
+                                       MAY_READ | MAY_WRITE, 1);
91
 
 
92
 
                if (!error && new_mnt)
93
 
                        error = aa_perm(active, new_dentry, new_mnt,
94
 
-                                       MAY_WRITE);
95
 
+                                       MAY_WRITE, 1);
96
 
        }
97
 
 
98
 
        put_aa_profile(active);
99
 
@@ -376,8 +376,22 @@ static int apparmor_inode_permission(str
100
 
 {
101
 
        if (!nd)
102
 
                return 0;
103
 
+       /*
104
 
+        * Assume we are /not/ checking a leaf directory.  We do not
105
 
+        * require profile access to non-leaf directories in order to
106
 
+        * traverse them, create or remove files in them.  We do require
107
 
+        * MAY_WRITE profile access on the actual file or directory
108
 
+        * being created or removed, though, which makes the model safe
109
 
+        * again.
110
 
+        *
111
 
+        * For the access(2) system call this means that when used on a
112
 
+        * leaf directory, we may return 0 even though the requesting
113
 
+        * process doesn't actually have MAY_WRITE nor MAY_EXEC access
114
 
+        * in the profile.  This is nothing but an inconvenience; using
115
 
+        * access(2) for permission checking is invalid, anyways.
116
 
+        */
117
 
        return aa_permission(inode, nd->dentry, nd->mnt,
118
 
-                            mask & (MAY_READ | MAY_WRITE | MAY_EXEC));
119
 
+                            mask & (MAY_READ | MAY_WRITE | MAY_EXEC), 0);
120
 
 }
121
 
 
122
 
 static int apparmor_inode_setattr(struct dentry *dentry, struct vfsmount *mnt,
123
 
@@ -454,6 +468,7 @@ static int apparmor_inode_removexattr(st
124
 
 static inline int aa_mmap(struct file *file, unsigned long prot,
125
 
                          unsigned long flags)
126
 
 {
127
 
+       struct dentry *dentry;
128
 
        int mask = 0;
129
 
 
130
 
        if (!file)
131
 
@@ -468,8 +483,8 @@ static inline int aa_mmap(struct file *f
132
 
        if (prot & PROT_EXEC)
133
 
                mask |= AA_EXEC_MMAP;
134
 
 
135
 
-       return aa_permission(file->f_dentry->d_inode, file->f_dentry,
136
 
-                            file->f_vfsmnt, mask);
137
 
+       dentry = file->f_dentry;
138
 
+       return aa_permission(dentry->d_inode, dentry, file->f_vfsmnt, mask, 1);
139
 
 }
140
 
 
141
 
 static int apparmor_file_mmap(struct file *file, unsigned long reqprot,
142
 
Index: b/security/apparmor/main.c
143
 
===================================================================
144
 
--- a/security/apparmor/main.c
145
 
+++ b/security/apparmor/main.c
146
 
@@ -80,32 +80,6 @@ out:
147
 
        return perms;
148
 
 }
149
 
 
150
 
-/**
151
 
- * aa_filter_mask
152
 
- * @mask: requested mask
153
 
- * @inode: potential directory inode
154
 
- *
155
 
- * This fn performs pre-verification of the requested mask
156
 
- * We ignore append. Previously we required 'w' on a dir to add a file.
157
 
- * No longer. Now we require 'w' on just the file itself. Traversal 'x' is
158
 
- * also ignored for directories.
159
 
- *
160
 
- * Returned value of %0 indicates no need to perform a perm check.
161
 
- */
162
 
-static inline int aa_filter_mask(int mask, struct inode *inode)
163
 
-{
164
 
-       if (mask) {
165
 
-               int elim = MAY_APPEND;
166
 
-
167
 
-               if (inode && S_ISDIR(inode->i_mode))
168
 
-                       elim |= (MAY_EXEC | MAY_WRITE);
169
 
-
170
 
-               mask &= ~elim;
171
 
-       }
172
 
-
173
 
-       return mask;
174
 
-}
175
 
-
176
 
 static inline void aa_permerror2result(int perm_result, struct aa_audit *sa)
177
 
 {
178
 
        if (perm_result == 0) { /* success */
179
 
@@ -599,17 +573,30 @@ int aa_perm_xattr(struct aa_profile *act
180
 
  * @dentry: dentry
181
 
  * @mnt: mountpoint
182
 
  * @mask: access mode requested
183
 
+ * @leaf: are we checking a leaf node?
184
 
  *
185
 
  * Determine if access (mask) for dentry is authorized by active
186
 
  * profile.  Result, %0 (success), -ve (error)
187
 
  */
188
 
 int aa_perm(struct aa_profile *active, struct dentry *dentry,
189
 
-           struct vfsmount *mnt, int mask)
190
 
+           struct vfsmount *mnt, int mask, int leaf)
191
 
 {
192
 
-       int error = 0;
193
 
+       struct inode *inode = dentry->d_inode;
194
 
        struct aa_audit sa;
195
 
+       int error = 0;
196
 
 
197
 
-       if ((mask = aa_filter_mask(mask, dentry->d_inode)) == 0)
198
 
+       if (!leaf && inode && S_ISDIR(inode->i_mode)) {
199
 
+               /*
200
 
+                * If checking a non-leaf directory, allow traverse and
201
 
+                * write access: we do not require profile access to
202
 
+                * non-leaf directories in order to traverse them,
203
 
+                * create or remove files in them. We do require
204
 
+                * MAY_WRITE profile access on the actual file or
205
 
+                * directory being created or removed, though.
206
 
+                */
207
 
+               mask &= ~(MAY_EXEC | MAY_WRITE);
208
 
+       }
209
 
+       if (mask == 0)
210
 
                goto out;
211
 
 
212
 
        sa.type = AA_AUDITTYPE_FILE;
213
 
Index: b/security/apparmor/apparmor.h
214
 
===================================================================
215
 
--- a/security/apparmor/apparmor.h
216
 
+++ b/security/apparmor/apparmor.h
217
 
@@ -209,7 +209,7 @@ extern int aa_perm_xattr(struct aa_profi
218
 
                         const char *xattr_xattr, int mask);
219
 
 extern int aa_capability(struct aa_profile *active, int cap);
220
 
 extern int aa_perm(struct aa_profile *active, struct dentry *dentry,
221
 
-                  struct vfsmount *mnt, int mask);
222
 
+                  struct vfsmount *mnt, int mask, int leaf);
223
 
 extern int aa_perm_dir(struct aa_profile *active, struct dentry *dentry,
224
 
                       struct vfsmount *mnt, const char *operation, int mask);
225
 
 extern int aa_link(struct aa_profile *active,