Skip to content

Commit ed5fa55

Browse files
torvaldstorvalds
committed
Merge tag 'audit-pr-20200226' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit
Pull audit fixes from Paul Moore: "Two fixes for problems found by syzbot: - Moving audit filter structure fields into a union caused some problems in the code which populates that filter structure. We keep the union (that idea is a good one), but we are fixing the code so that it doesn't needlessly set fields in the union and mess up the error handling. - The audit_receive_msg() function wasn't validating user input as well as it should in all cases, we add the necessary checks" * tag 'audit-pr-20200226' of git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit: audit: always check the netlink payload length in audit_receive_msg() audit: fix error handling in audit_data_to_entry()
2 parents bfdc6d9 + 7561252 commit ed5fa55

2 files changed

Lines changed: 60 additions & 51 deletions

File tree

‎kernel/audit.c‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1101,13 +1101,11 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
11011101
audit_log_end(ab);
11021102
}
11031103

1104-
static int audit_set_feature(struct sk_buff *skb)
1104+
static int audit_set_feature(struct audit_features *uaf)
11051105
{
1106-
struct audit_features *uaf;
11071106
int i;
11081107

11091108
BUILD_BUG_ON(AUDIT_LAST_FEATURE + 1 > ARRAY_SIZE(audit_feature_names));
1110-
uaf = nlmsg_data(nlmsg_hdr(skb));
11111109

11121110
/* if there is ever a version 2 we should handle that here */
11131111

@@ -1175,6 +1173,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
11751173
{
11761174
u32 seq;
11771175
void *data;
1176+
int data_len;
11781177
int err;
11791178
struct audit_buffer *ab;
11801179
u16 msg_type = nlh->nlmsg_type;
@@ -1188,6 +1187,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
11881187

11891188
seq = nlh->nlmsg_seq;
11901189
data = nlmsg_data(nlh);
1190+
data_len = nlmsg_len(nlh);
11911191

11921192
switch (msg_type) {
11931193
case AUDIT_GET: {
@@ -1211,7 +1211,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
12111211
struct audit_status s;
12121212
memset(&s, 0, sizeof(s));
12131213
/* guard against past and future API changes */
1214-
memcpy(&s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));
1214+
memcpy(&s, data, min_t(size_t, sizeof(s), data_len));
12151215
if (s.mask & AUDIT_STATUS_ENABLED) {
12161216
err = audit_set_enabled(s.enabled);
12171217
if (err < 0)
@@ -1315,7 +1315,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
13151315
return err;
13161316
break;
13171317
case AUDIT_SET_FEATURE:
1318-
err = audit_set_feature(skb);
1318+
if (data_len < sizeof(struct audit_features))
1319+
return -EINVAL;
1320+
err = audit_set_feature(data);
13191321
if (err)
13201322
return err;
13211323
break;
@@ -1327,33 +1329,33 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
13271329

13281330
err = audit_filter(msg_type, AUDIT_FILTER_USER);
13291331
if (err == 1) { /* match or error */
1332+
char *str = data;
1333+
13301334
err = 0;
13311335
if (msg_type == AUDIT_USER_TTY) {
13321336
err = tty_audit_push();
13331337
if (err)
13341338
break;
13351339
}
13361340
audit_log_user_recv_msg(&ab, msg_type);
1337-
if (msg_type != AUDIT_USER_TTY)
1341+
if (msg_type != AUDIT_USER_TTY) {
1342+
/* ensure NULL termination */
1343+
str[data_len - 1] = '\0';
13381344
audit_log_format(ab, " msg='%.*s'",
13391345
AUDIT_MESSAGE_TEXT_MAX,
1340-
(char *)data);
1341-
else {
1342-
int size;
1343-
1346+
str);
1347+
} else {
13441348
audit_log_format(ab, " data=");
1345-
size = nlmsg_len(nlh);
1346-
if (size > 0 &&
1347-
((unsigned char *)data)[size - 1] == '\0')
1348-
size--;
1349-
audit_log_n_untrustedstring(ab, data, size);
1349+
if (data_len > 0 && str[data_len - 1] == '\0')
1350+
data_len--;
1351+
audit_log_n_untrustedstring(ab, str, data_len);
13501352
}
13511353
audit_log_end(ab);
13521354
}
13531355
break;
13541356
case AUDIT_ADD_RULE:
13551357
case AUDIT_DEL_RULE:
1356-
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
1358+
if (data_len < sizeof(struct audit_rule_data))
13571359
return -EINVAL;
13581360
if (audit_enabled == AUDIT_LOCKED) {
13591361
audit_log_common_recv_msg(audit_context(), &ab,
@@ -1365,7 +1367,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
13651367
audit_log_end(ab);
13661368
return -EPERM;
13671369
}
1368-
err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh));
1370+
err = audit_rule_change(msg_type, seq, data, data_len);
13691371
break;
13701372
case AUDIT_LIST_RULES:
13711373
err = audit_list_rules_send(skb, seq);
@@ -1380,7 +1382,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
13801382
case AUDIT_MAKE_EQUIV: {
13811383
void *bufp = data;
13821384
u32 sizes[2];
1383-
size_t msglen = nlmsg_len(nlh);
1385+
size_t msglen = data_len;
13841386
char *old, *new;
13851387

13861388
err = -EINVAL;
@@ -1456,7 +1458,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
14561458

14571459
memset(&s, 0, sizeof(s));
14581460
/* guard against past and future API changes */
1459-
memcpy(&s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));
1461+
memcpy(&s, data, min_t(size_t, sizeof(s), data_len));
14601462
/* check if new data is valid */
14611463
if ((s.enabled != 0 && s.enabled != 1) ||
14621464
(s.log_passwd != 0 && s.log_passwd != 1))

‎kernel/auditfilter.c‎

Lines changed: 39 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
456456
bufp = data->buf;
457457
for (i = 0; i < data->field_count; i++) {
458458
struct audit_field *f = &entry->rule.fields[i];
459+
u32 f_val;
459460

460461
err = -EINVAL;
461462

@@ -464,12 +465,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
464465
goto exit_free;
465466

466467
f->type = data->fields[i];
467-
f->val = data->values[i];
468+
f_val = data->values[i];
468469

469470
/* Support legacy tests for a valid loginuid */
470-
if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET)) {
471+
if ((f->type == AUDIT_LOGINUID) && (f_val == AUDIT_UID_UNSET)) {
471472
f->type = AUDIT_LOGINUID_SET;
472-
f->val = 0;
473+
f_val = 0;
473474
entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;
474475
}
475476

@@ -485,7 +486,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
485486
case AUDIT_SUID:
486487
case AUDIT_FSUID:
487488
case AUDIT_OBJ_UID:
488-
f->uid = make_kuid(current_user_ns(), f->val);
489+
f->uid = make_kuid(current_user_ns(), f_val);
489490
if (!uid_valid(f->uid))
490491
goto exit_free;
491492
break;
@@ -494,11 +495,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
494495
case AUDIT_SGID:
495496
case AUDIT_FSGID:
496497
case AUDIT_OBJ_GID:
497-
f->gid = make_kgid(current_user_ns(), f->val);
498+
f->gid = make_kgid(current_user_ns(), f_val);
498499
if (!gid_valid(f->gid))
499500
goto exit_free;
500501
break;
501502
case AUDIT_ARCH:
503+
f->val = f_val;
502504
entry->rule.arch_f = f;
503505
break;
504506
case AUDIT_SUBJ_USER:
@@ -511,11 +513,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
511513
case AUDIT_OBJ_TYPE:
512514
case AUDIT_OBJ_LEV_LOW:
513515
case AUDIT_OBJ_LEV_HIGH:
514-
str = audit_unpack_string(&bufp, &remain, f->val);
515-
if (IS_ERR(str))
516+
str = audit_unpack_string(&bufp, &remain, f_val);
517+
if (IS_ERR(str)) {
518+
err = PTR_ERR(str);
516519
goto exit_free;
517-
entry->rule.buflen += f->val;
518-
520+
}
521+
entry->rule.buflen += f_val;
522+
f->lsm_str = str;
519523
err = security_audit_rule_init(f->type, f->op, str,
520524
(void **)&f->lsm_rule);
521525
/* Keep currently invalid fields around in case they
@@ -524,68 +528,71 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
524528
pr_warn("audit rule for LSM \'%s\' is invalid\n",
525529
str);
526530
err = 0;
527-
}
528-
if (err) {
529-
kfree(str);
531+
} else if (err)
530532
goto exit_free;
531-
} else
532-
f->lsm_str = str;
533533
break;
534534
case AUDIT_WATCH:
535-
str = audit_unpack_string(&bufp, &remain, f->val);
536-
if (IS_ERR(str))
535+
str = audit_unpack_string(&bufp, &remain, f_val);
536+
if (IS_ERR(str)) {
537+
err = PTR_ERR(str);
537538
goto exit_free;
538-
entry->rule.buflen += f->val;
539-
540-
err = audit_to_watch(&entry->rule, str, f->val, f->op);
539+
}
540+
err = audit_to_watch(&entry->rule, str, f_val, f->op);
541541
if (err) {
542542
kfree(str);
543543
goto exit_free;
544544
}
545+
entry->rule.buflen += f_val;
545546
break;
546547
case AUDIT_DIR:
547-
str = audit_unpack_string(&bufp, &remain, f->val);
548-
if (IS_ERR(str))
548+
str = audit_unpack_string(&bufp, &remain, f_val);
549+
if (IS_ERR(str)) {
550+
err = PTR_ERR(str);
549551
goto exit_free;
550-
entry->rule.buflen += f->val;
551-
552+
}
552553
err = audit_make_tree(&entry->rule, str, f->op);
553554
kfree(str);
554555
if (err)
555556
goto exit_free;
557+
entry->rule.buflen += f_val;
556558
break;
557559
case AUDIT_INODE:
560+
f->val = f_val;
558561
err = audit_to_inode(&entry->rule, f);
559562
if (err)
560563
goto exit_free;
561564
break;
562565
case AUDIT_FILTERKEY:
563-
if (entry->rule.filterkey || f->val > AUDIT_MAX_KEY_LEN)
566+
if (entry->rule.filterkey || f_val > AUDIT_MAX_KEY_LEN)
564567
goto exit_free;
565-
str = audit_unpack_string(&bufp, &remain, f->val);
566-
if (IS_ERR(str))
568+
str = audit_unpack_string(&bufp, &remain, f_val);
569+
if (IS_ERR(str)) {
570+
err = PTR_ERR(str);
567571
goto exit_free;
568-
entry->rule.buflen += f->val;
572+
}
573+
entry->rule.buflen += f_val;
569574
entry->rule.filterkey = str;
570575
break;
571576
case AUDIT_EXE:
572-
if (entry->rule.exe || f->val > PATH_MAX)
577+
if (entry->rule.exe || f_val > PATH_MAX)
573578
goto exit_free;
574-
str = audit_unpack_string(&bufp, &remain, f->val);
579+
str = audit_unpack_string(&bufp, &remain, f_val);
575580
if (IS_ERR(str)) {
576581
err = PTR_ERR(str);
577582
goto exit_free;
578583
}
579-
entry->rule.buflen += f->val;
580-
581-
audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
584+
audit_mark = audit_alloc_mark(&entry->rule, str, f_val);
582585
if (IS_ERR(audit_mark)) {
583586
kfree(str);
584587
err = PTR_ERR(audit_mark);
585588
goto exit_free;
586589
}
590+
entry->rule.buflen += f_val;
587591
entry->rule.exe = audit_mark;
588592
break;
593+
default:
594+
f->val = f_val;
595+
break;
589596
}
590597
}
591598

0 commit comments

Comments
 (0)