* [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
@ 2018-05-27 22:55 Colin King
2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:30 ` [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Mimi Zohar
0 siblings, 2 replies; 6+ messages in thread
From: Colin King @ 2018-05-27 22:55 UTC (permalink / raw)
To: linux-security-module
From: Colin Ian King <colin.king@canonical.com>
In the case where the allocation of xattr fails and xattr is NULL, the
error exit return path via label 'out' will dereference xattr when
kfree'ing xattr-name. Fix this by only kfree'ing xattr->name and xattr
when xattr is non-null.
Detected by CoverityScan, CID#1469366 ("Dereference after null check")
Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
security/integrity/evm/evm_secfs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index fb8bc950aceb..cf5cd303d7c0 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -253,8 +253,10 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
out:
audit_log_format(ab, " res=%d", err);
audit_log_end(ab);
- kfree(xattr->name);
- kfree(xattr);
+ if (xattr) {
+ kfree(xattr->name);
+ kfree(xattr);
+ }
return err;
}
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
@ 2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:31 ` Mimi Zohar
2018-05-29 12:30 ` [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Mimi Zohar
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2018-05-29 9:05 UTC (permalink / raw)
To: linux-security-module
Not really related to this patch except I was looking at the function:
security/integrity/evm/evm_secfs.c
191 ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
192 if (IS_ERR(ab))
193 return PTR_ERR(ab);
194
195 xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
196 if (!xattr) {
197 err = -ENOMEM;
198 goto out;
199 }
200
201 xattr->name = memdup_user_nul(buf, count);
202 if (IS_ERR(xattr->name)) {
203 err = PTR_ERR(xattr->name);
204 xattr->name = NULL;
205 goto out;
206 }
207
208 /* Remove any trailing newline */
209 len = strlen(xattr->name);
210 if (xattr->name[len-1] == '\n')
strlen() could be zero, leading to a read underflow here.
211 xattr->name[len-1] = '\0';
212
213 if (strcmp(xattr->name, ".") == 0) {
214 evm_xattrs_locked = 1;
215 newattrs.ia_mode = S_IFREG | 0440;
216 newattrs.ia_valid = ATTR_MODE;
217 inode = evm_xattrs->d_inode;
218 inode_lock(inode);
219 err = simple_setattr(evm_xattrs, &newattrs);
220 inode_unlock(inode);
221 audit_log_format(ab, "locked");
222 if (!err)
223 err = count;
224 goto out;
225 }
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-29 9:05 ` Dan Carpenter
@ 2018-05-29 12:31 ` Mimi Zohar
2018-05-29 13:11 ` [PATCH] EVM: prevent array underflow in evm_write_xattrs() Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Mimi Zohar @ 2018-05-29 12:31 UTC (permalink / raw)
To: linux-security-module
Hi Dan,
On Tue, 2018-05-29 at 12:05 +0300, Dan Carpenter wrote:
> Not really related to this patch except I was looking at the function:
>
> security/integrity/evm/evm_secfs.c
> 191 ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> 192 if (IS_ERR(ab))
> 193 return PTR_ERR(ab);
> 194
> 195 xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
> 196 if (!xattr) {
> 197 err = -ENOMEM;
> 198 goto out;
> 199 }
> 200
> 201 xattr->name = memdup_user_nul(buf, count);
> 202 if (IS_ERR(xattr->name)) {
> 203 err = PTR_ERR(xattr->name);
> 204 xattr->name = NULL;
> 205 goto out;
> 206 }
> 207
> 208 /* Remove any trailing newline */
> 209 len = strlen(xattr->name);
> 210 if (xattr->name[len-1] == '\n')
>
> strlen() could be zero, leading to a read underflow here.
Thanks! ?Could you modify the maximum xattr size check (before this
code snippet) to check for underflow?
Mimi
>
> 211 xattr->name[len-1] = '\0';
> 212
> 213 if (strcmp(xattr->name, ".") == 0) {
> 214 evm_xattrs_locked = 1;
> 215 newattrs.ia_mode = S_IFREG | 0440;
> 216 newattrs.ia_valid = ATTR_MODE;
> 217 inode = evm_xattrs->d_inode;
> 218 inode_lock(inode);
> 219 err = simple_setattr(evm_xattrs, &newattrs);
> 220 inode_unlock(inode);
> 221 audit_log_format(ab, "locked");
> 222 if (!err)
> 223 err = count;
> 224 goto out;
> 225 }
>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] EVM: prevent array underflow in evm_write_xattrs()
2018-05-29 12:31 ` Mimi Zohar
@ 2018-05-29 13:11 ` Dan Carpenter
2018-05-31 15:00 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2018-05-29 13:11 UTC (permalink / raw)
To: linux-security-module
If the user sets xattr->name[0] to NUL then we would read one character
before the start of the array. This bug seems harmless as far as I can
see but perhaps it would trigger a warning in KASAN.
Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
The user can pass a zeroed buffer to memdup_user_nul() so we can't rely
on "count" to test this.
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index a7a0a1acae99..94c739180a0b 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -207,7 +207,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
/* Remove any trailing newline */
len = strlen(xattr->name);
- if (xattr->name[len-1] == '\n')
+ if (len && xattr->name[len-1] == '\n')
xattr->name[len-1] = '\0';
if (strcmp(xattr->name, ".") == 0) {
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] EVM: prevent array underflow in evm_write_xattrs()
2018-05-29 13:11 ` [PATCH] EVM: prevent array underflow in evm_write_xattrs() Dan Carpenter
@ 2018-05-31 15:00 ` Mimi Zohar
0 siblings, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2018-05-31 15:00 UTC (permalink / raw)
To: linux-security-module
On Tue, 2018-05-29 at 16:11 +0300, Dan Carpenter wrote:
> If the user sets xattr->name[0] to NUL then we would read one character
> before the start of the array. This bug seems harmless as far as I can
> see but perhaps it would trigger a warning in KASAN.
>
> Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks, this patch is now queued in the next-integrity branch.
Mimi
> ---
> The user can pass a zeroed buffer to memdup_user_nul() so we can't rely
> on "count" to test this.
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index a7a0a1acae99..94c739180a0b 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -207,7 +207,7 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
>
> /* Remove any trailing newline */
> len = strlen(xattr->name);
> - if (xattr->name[len-1] == '\n')
> + if (len && xattr->name[len-1] == '\n')
> xattr->name[len-1] = '\0';
>
> if (strcmp(xattr->name, ".") == 0) {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
2018-05-29 9:05 ` Dan Carpenter
@ 2018-05-29 12:30 ` Mimi Zohar
1 sibling, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2018-05-29 12:30 UTC (permalink / raw)
To: linux-security-module
Hi Colin,
On Sun, 2018-05-27 at 23:55 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the case where the allocation of xattr fails and xattr is NULL, the
> error exit return path via label 'out' will dereference xattr when
> kfree'ing xattr-name. Fix this by only kfree'ing xattr->name and xattr
> when xattr is non-null.
>
> Detected by CoverityScan, CID#1469366 ("Dereference after null check")
>
> Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> security/integrity/evm/evm_secfs.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index fb8bc950aceb..cf5cd303d7c0 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -253,8 +253,10 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> out:
> audit_log_format(ab, " res=%d", err);
> audit_log_end(ab);
> - kfree(xattr->name);
> - kfree(xattr);
> + if (xattr) {
> + kfree(xattr->name);
> + kfree(xattr);
> + }
> return err;
> }
>
Thanks! ?To fix this problem, I think more is needed.
Without the xattr, there is nothing to audit except the attempt to
extend the xattr list. ?Failure to allocate the xattr or xattr->name
should either result in a different audit message or return
immediately without any audit message.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-31 15:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:31 ` Mimi Zohar
2018-05-29 13:11 ` [PATCH] EVM: prevent array underflow in evm_write_xattrs() Dan Carpenter
2018-05-31 15:00 ` Mimi Zohar
2018-05-29 12:30 ` [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Mimi Zohar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).