e2fsprogs: avoid segfault when s_nr_users is too high

PATCH Applied Lukas Czerner 2 participants 2 messages Sep 26, 2018 – Oct 3, 2018 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lukas Czerner

author

2018-09-26 14:51 UTC #1 lore raw

Quoted 11 lines · Lukas Czerner

Currently in e2fsprogs tools it's possible to access out of bounds memory when reading list of ids sharing a journal log (journal_superblock_t->s_users[]) in case where s_nr_users is too high.
This is because we never check whether the s_nr_users fits into the restriction of JFS_USERS_MAX. Fix it by checking that nr_users is not bigger than JFS_USERS_MAX and error out when possiblem.
Also add test for dumpe2fs. The rest would require involving external journal which is not possible to test with e2fsprogs test suite at the moment.

Seems this went unnoticed for a while. Can I get some eyes on it ?

Thanks!
-Lukas

Quoted 1,007 lines

Signed-off-by Lukas Czerner <[email protected]>

---
 lib/e2p/ljs.c                             |   4 +-
 lib/ext2fs/mkjournal.c                    |   2 +
 misc/tune2fs.c                            |  11 +++
 tests/d_corrupt_journal_nr_users/expect   |  99 ++++++++++++++++++++++
 tests/d_corrupt_journal_nr_users/image.gz | Bin 0 -> 8788 bytes
 tests/d_corrupt_journal_nr_users/name     |   1 +
 tests/d_corrupt_journal_nr_users/script   |  25 ++++++
 tests/f_bad_local_jnl/image               | Bin 0 -> 8388608 bytes
 8 files changed, 140 insertions(+), 2 deletions(-)
 create mode 100644 tests/d_corrupt_journal_nr_users/expect
 create mode 100644 tests/d_corrupt_journal_nr_users/image.gz
 create mode 100644 tests/d_corrupt_journal_nr_users/name
 create mode 100644 tests/d_corrupt_journal_nr_users/script
 create mode 100644 tests/f_bad_local_jnl/image

lib/e2p/ljs.c +2 -2

index 0b1beadb..c99126b6 100644
@@ -101,10 +101,10 @@ void e2p_list_journal_super(FILE *f, char *journal_sb_buf,
 			e2p_be32(jsb->s_checksum));
 	if ((nr_users > 1) ||
 	    !e2p_is_null_uuid(&jsb->s_users[0])) {
-		for (i=0; i < nr_users; i++) {
+		for (i=0; i < nr_users && i < JFS_USERS_MAX; i++) {
 			printf(i ? "                          %s\n"
 			       : "Journal users:            %s\n",
-			       e2p_uuid2str(&jsb->s_users[i*16]));
+			       e2p_uuid2str(&jsb->s_users[i * UUID_SIZE]));
 		}
 	}
 	if (jsb->s_errno != 0)

lib/ext2fs/mkjournal.c +2 -0

index 7f78291d..a90e80e0 100644
@@ -401,6 +401,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 
 	/* Check and see if this filesystem has already been added */
 	nr_users = ntohl(jsb->s_nr_users);
+	if (nr_users > JFS_USERS_MAX)
+		return EXT2_ET_CORRUPT_JOURNAL_SB;
 	for (i=0; i < nr_users; i++) {
 		if (memcmp(fs->super->s_uuid,
 			   &jsb->s_users[i*16], 16) == 0)

misc/tune2fs.c +11 -0

index 4b673bc2..d5d542fb 100644
@@ -291,6 +291,12 @@ static int remove_journal_device(ext2_filsys fs)
 	jsb = (journal_superblock_t *) buf;
 	/* Find the filesystem UUID */
 	nr_users = ntohl(jsb->s_nr_users);
+	if (nr_users > JFS_USERS_MAX) {
+		fprintf(stderr, _("Journal superblock is corrupted, nr_users\n"
+				 "is too high (%d).\n"), nr_users);
+		commit_remove_journal = 1;
+		goto no_valid_journal;
+	}
 
 	if (!journal_user(fs->super->s_uuid, jsb->s_users, nr_users)) {
 		fputs(_("Filesystem's UUID not found on journal device.\n"),

@@ -2854,6 +2860,11 @@ fs_update_journal_user(struct ext2_super_block *sb, __u8 old_uuid[UUID_SIZE])
 	jsb = (journal_superblock_t *) buf;
 	/* Find the filesystem UUID */
 	nr_users = ntohl(jsb->s_nr_users);
+	if (nr_users > JFS_USERS_MAX) {
+		fprintf(stderr, _("Journal superblock is corrupted, nr_users\n"
+				 "is too high (%d).\n"), nr_users);
+		return EXT2_ET_CORRUPT_JOURNAL_SB;
+	}
 
 	j_uuid = journal_user(old_uuid, jsb->s_users, nr_users);
 	if (j_uuid == NULL) {

tests/d_corrupt_journal_nr_users/expect new file +99 -0

new file mode 100644
index 00000000..cdfb49a0
@@ -0,0 +1,99 @@
+Filesystem volume name:   <none>
+Last mounted on:          <not available>
+Filesystem magic number:  0xEF53
+Filesystem revision #:    1 (dynamic)
+Filesystem features:      has_journal ext_attr resize_inode dir_index filetype extent 64bit flex_bg sparse_super large_file huge_file dir_nlink extra_isize metadata_csum
+Default mount options:    user_xattr acl
+Filesystem state:         clean
+Errors behavior:          Continue
+Filesystem OS type:       Linux
+Inode count:              512
+Block count:              2048
+Reserved block count:     102
+Free blocks:              982
+Free inodes:              501
+First block:              0
+Block size:               4096
+Fragment size:            4096
+Group descriptor size:    64
+Blocks per group:         32768
+Fragments per group:      32768
+Inodes per group:         512
+Inode blocks per group:   32
+Flex block group size:    16
+Mount count:              0
+Check interval:           0 (<none>)
+Reserved blocks uid:      0
+Reserved blocks gid:      0
+First inode:              11
+Inode size:	          256
+Required extra isize:     32
+Desired extra isize:      32
+Journal inode:            8
+Default directory hash:   half_md4
+Journal backup:           inode blocks
+Checksum type:            crc32c
+Journal features:         (none)
+Journal size:             4096k
+Journal length:           1024
+Journal sequence:         0x00000001
+Journal start:            0
+Journal number of users:  9999
+Journal users:            <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+                          <none>
+
+
+Group 0: (Blocks 0-2047)
+  Primary superblock at 0, Group descriptors at 1-1
+  Block bitmap at 2 (+2)
+  Inode bitmap at 18 (+18)
+  Inode table at 34-65 (+34)
+  982 free blocks, 501 free inodes, 2 directories, 501 unused inodes
+  Free blocks: 1066-2047
+  Free inodes: 12-512

tests/d_corrupt_journal_nr_users/image.gz new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..1fc32eddd679fbea791aeba59faf0fcd93cbccd3
GIT binary patch

tests/d_corrupt_journal_nr_users/name new file +1 -0

new file mode 100644
index 00000000..8b33a273
@@ -0,0 +1 @@
+Journal superblock corrupted, nr_users too high

tests/d_corrupt_journal_nr_users/script new file +25 -0

new file mode 100644
index 00000000..683cd487
@@ -0,0 +1,25 @@
+if ! test -x $DEBUGFS_EXE; then
+	echo "$test_name: $test_description: skipped (no debugfs)"
+	return 0
+fi
+
+IMAGE=$test_dir/image.gz
+EXP=$test_dir/expect
+OUT=$test_name.log
+gunzip < $IMAGE > $TMPFILE
+
+$DUMPE2FS $TMPFILE >> $OUT.new 2>&1
+sed -f $cmd_dir/filter.sed $OUT.new > $OUT
+rm -f $TMPFILE $OUT.new
+
+cmp -s $OUT $EXP
+status=$?
+
+if [ "$status" = 0 ] ; then
+	echo "$test_name: $test_description: ok"
+	touch $test_name.ok
+else
+	echo "$test_name: $test_description: failed"
+	diff $DIFF_OPTS $EXP $OUT > $test_name.failed
+	rm -f $test_name.tmp
+fi

tests/f_bad_local_jnl/image new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..6f2b550a3811cdffb3cdf8f3eb0eaae4e9a9421a
GIT binary patch

Signature · 2 lines

-- 
2.17.1

Theodore Y. Ts'o

Applied

2018-10-03 08:40 UTC #2 lore raw

In reply to a message not in the archive

Quoted 8 lines · Lukas Czerner

Currently in e2fsprogs tools it's possible to access out of bounds memory when reading list of ids sharing a journal log (journal_superblock_t->s_users[]) in case where s_nr_users is too high.
This is because we never check whether the s_nr_users fits into the restriction of JFS_USERS_MAX. Fix it by checking that nr_users is not bigger than JFS_USERS_MAX and error out when possiblem.

Quoted 5 lines

Also add test for dumpe2fs. The rest would require involving external journal which is not possible to test with e2fsprogs test suite at the moment.
Signed-off-by Lukas Czerner <[email protected]>

Thanks, applied.

- Ted