[PATCH 0/2] Handle invalid os_type in e2p.

[PATCH 0/2] Handle invalid os_type in e2p.

[Adam Buchbinder]

Hi! While fuzzing with american fuzzy lop, I discovered an issue in
the e2p library. The fix there seemed straightforward, but I'm not
sure I understand the testing infrastructure; running dumpe2fs looks
like the easiest way to trigger the problem. Any feedback here would
be welcome. Thanks!

Adam Buchbinder
Google, Inc.

[PATCH 1/2] tests: dumpe2fs against an image with bad os_type.

[Adam Buchbinder]

The os_type here is large enough to be negative when interpreted as a
signed integer.

This test case was generated by american fuzzy lop, starting from a
base filesystem image from files.fuzzing-project.org.

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 tests/d_bad_ostype/expect   |   1 +
 tests/d_bad_ostype/image.gz | Bin 0 -> 272 bytes
 tests/d_bad_ostype/name     |   1 +
 tests/d_bad_ostype/script   |  19 +++++++++++++++++++
 4 files changed, 21 insertions(+)
 create mode 100644 tests/d_bad_ostype/expect
 create mode 100644 tests/d_bad_ostype/image.gz
 create mode 100644 tests/d_bad_ostype/name
 create mode 100644 tests/d_bad_ostype/script

diff --git a/tests/d_bad_ostype/expect b/tests/d_bad_ostype/expect
new file mode 100644
index 00000000..b00318c4
--- /dev/null
+++ b/tests/d_bad_ostype/expect
@@ -0,0 +1 @@
+Filesystem OS type:       (unknown os)
diff --git a/tests/d_bad_ostype/image.gz b/tests/d_bad_ostype/image.gz
new file mode 100644
index 0000000000000000000000000000000000000000..06805e1093c1ce9d2dc172aa89fb57eaedfa5505
GIT binary patch
literal 272
zcmV+r0q_1FiwFpQOm0~K18HqxXJr6mU>F6XU=)m0AOOS;K+Fup8bHhh#Ei%a6d)L+
z?&+16q1uYMkqiv~{|CQk1gnGa!Jq-Aj~m1RVhgBwAhY*~Zq>5lb9}siN7?zgx^kZA
z?$FQ8E731WEiR$5|B%5FZu@YTFo#cl{h8X~OW!;Lxt9kjw+`qxTG+{mLtK~^mW_f@
zKq3I;O%@<#WdP-WCLo_>fL$~SM!_hc7x@3b9z-?#9|fae6pTDzKhO>s1*2dT3>1L1
z13>-wb@2W@{lHsC25<-DIneI+Pz;e_0goMk0ElL%r=v!}hye|aK*E591(fz-7$U<6
WPk-bB5lS6L2LJ#sc=EFV7ytl<z;*Tj

literal 0
HcmV?d00001

diff --git a/tests/d_bad_ostype/name b/tests/d_bad_ostype/name
new file mode 100644
index 00000000..3da887a8
--- /dev/null
+++ b/tests/d_bad_ostype/name
@@ -0,0 +1 @@
+handle bad (negative) os_type
diff --git a/tests/d_bad_ostype/script b/tests/d_bad_ostype/script
new file mode 100644
index 00000000..94f356a6
--- /dev/null
+++ b/tests/d_bad_ostype/script
@@ -0,0 +1,19 @@
+gzip -dc < $test_dir/image.gz > $TMPFILE
+
+OUT=$test_name.log
+EXP=$test_dir/expect
+$DUMPE2FS $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed | grep 'Filesystem OS type:' > $OUT
+
+rm -f $TMPFILE
+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
+unset OUT EXP
-- 
2.14.0.rc0.284.gd933b75aa4-goog

Re: [PATCH 1/2] tests: dumpe2fs against an image with bad os_type.

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2997 bytes --]

On Jul 18, 2017, at 1:29 PM, Adam Buchbinder <abuchbinder@google.com> wrote:
> 
> The os_type here is large enough to be negative when interpreted as a
> signed integer.
> 
> This test case was generated by american fuzzy lop, starting from a
> base filesystem image from files.fuzzing-project.org.

It is preferable to create the image filesystem via script as part of the
test (e.g. using "debugfs -w ssv ostype 0xf00000000" or similar), rather
than using a binary image.  That allows us to see what is being changed,
rather than getting a binary blob.

Cheers, Andreas

> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
> ---
> tests/d_bad_ostype/expect   |   1 +
> tests/d_bad_ostype/image.gz | Bin 0 -> 272 bytes
> tests/d_bad_ostype/name     |   1 +
> tests/d_bad_ostype/script   |  19 +++++++++++++++++++
> 4 files changed, 21 insertions(+)
> create mode 100644 tests/d_bad_ostype/expect
> create mode 100644 tests/d_bad_ostype/image.gz
> create mode 100644 tests/d_bad_ostype/name
> create mode 100644 tests/d_bad_ostype/script
> 
> diff --git a/tests/d_bad_ostype/expect b/tests/d_bad_ostype/expect
> new file mode 100644
> index 00000000..b00318c4
> --- /dev/null
> +++ b/tests/d_bad_ostype/expect
> @@ -0,0 +1 @@
> +Filesystem OS type:       (unknown os)
> diff --git a/tests/d_bad_ostype/image.gz b/tests/d_bad_ostype/image.gz
> new file mode 100644
> index 0000000000000000000000000000000000000000..06805e1093c1ce9d2dc172aa89fb57eaedfa5505
> GIT binary patch
> literal 272
> zcmV+r0q_1FiwFpQOm0~K18HqxXJr6mU>F6XU=)m0AOOS;K+Fup8bHhh#Ei%a6d)L+
> z?&+16q1uYMkqiv~{|CQk1gnGa!Jq-Aj~m1RVhgBwAhY*~Zq>5lb9}siN7?zgx^kZA
> z?$FQ8E731WEiR$5|B%5FZu@YTFo#cl{h8X~OW!;Lxt9kjw+`qxTG+{mLtK~^mW_f@
> zKq3I;O%@<#WdP-WCLo_>fL$~SM!_hc7x@3b9z-?#9|fae6pTDzKhO>s1*2dT3>1L1
> z13>-wb@2W@{lHsC25<-DIneI+Pz;e_0goMk0ElL%r=v!}hye|aK*E591(fz-7$U<6
> WPk-bB5lS6L2LJ#sc=EFV7ytl<z;*Tj
> 
> literal 0
> HcmV?d00001
> 
> diff --git a/tests/d_bad_ostype/name b/tests/d_bad_ostype/name
> new file mode 100644
> index 00000000..3da887a8
> --- /dev/null
> +++ b/tests/d_bad_ostype/name
> @@ -0,0 +1 @@
> +handle bad (negative) os_type
> diff --git a/tests/d_bad_ostype/script b/tests/d_bad_ostype/script
> new file mode 100644
> index 00000000..94f356a6
> --- /dev/null
> +++ b/tests/d_bad_ostype/script
> @@ -0,0 +1,19 @@
> +gzip -dc < $test_dir/image.gz > $TMPFILE
> +
> +OUT=$test_name.log
> +EXP=$test_dir/expect
> +$DUMPE2FS $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed | grep 'Filesystem OS type:' > $OUT
> +
> +rm -f $TMPFILE
> +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
> +unset OUT EXP
> --
> 2.14.0.rc0.284.gd933b75aa4-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH 2/2] e2p: Fix segfault in e2p_os2string.

[Adam Buchbinder]

Passing in a negative integer would lead to a segfault, and
a crafted filesystem image could trigger that.

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 lib/e2p/ostype.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/e2p/ostype.c b/lib/e2p/ostype.c
index 7f088446..c5fd8ab3 100644
--- a/lib/e2p/ostype.c
+++ b/lib/e2p/ostype.c
@@ -30,7 +30,7 @@ char *e2p_os2string(int os_type)
         const char	*os;
 	char 		*ret;
 
-	if (os_type <= EXT2_OS_LITES)
+	if (os_type >= 0 && os_type <= EXT2_OS_LITES)
 		os = os_tab[os_type];
 	else
 		os = "(unknown os)";
-- 
2.14.0.rc0.284.gd933b75aa4-goog

Re: [PATCH 2/2] e2p: Fix segfault in e2p_os2string.

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Jul 18, 2017, at 1:29 PM, Adam Buchbinder <abuchbinder@google.com> wrote:
> 
> Passing in a negative integer would lead to a segfault, and
> a crafted filesystem image could trigger that.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Note that it would be better to land this patch as 1/2, and the test which
causes the segfault as 2/2, otherwise it could break git bisect.

Cheers, Andreas
> ---
> lib/e2p/ostype.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/e2p/ostype.c b/lib/e2p/ostype.c
> index 7f088446..c5fd8ab3 100644
> --- a/lib/e2p/ostype.c
> +++ b/lib/e2p/ostype.c
> @@ -30,7 +30,7 @@ char *e2p_os2string(int os_type)
>         const char	*os;
> 	char 		*ret;
> 
> -	if (os_type <= EXT2_OS_LITES)
> +	if (os_type >= 0 && os_type <= EXT2_OS_LITES)
> 		os = os_tab[os_type];
> 	else
> 		os = "(unknown os)";




Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH v2 0/2] Handle invalid os_type in e2p.

[Adam Buchbinder]

Hi! Here's a new version with the updates suggested by Andreas Dilger,
specifically to switch the order (to not break git bisect) and for the
test to avoid including a binary blob.

Adam Buchbinder
Google, Inc.

[PATCH v2 1/2] e2p: Fix segfault in e2p_os2string.

[Adam Buchbinder]

Passing in a negative integer would lead to a segfault, and
a crafted filesystem image could trigger that.

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 lib/e2p/ostype.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/e2p/ostype.c b/lib/e2p/ostype.c
index 7f088446..c5fd8ab3 100644
--- a/lib/e2p/ostype.c
+++ b/lib/e2p/ostype.c
@@ -30,7 +30,7 @@ char *e2p_os2string(int os_type)
         const char	*os;
 	char 		*ret;
 
-	if (os_type <= EXT2_OS_LITES)
+	if (os_type >= 0 && os_type <= EXT2_OS_LITES)
 		os = os_tab[os_type];
 	else
 		os = "(unknown os)";
-- 
2.14.0.rc0.284.gd933b75aa4-goog

Re: [PATCH v2 1/2] e2p: Fix segfault in e2p_os2string.

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]


> On Jul 18, 2017, at 3:48 PM, Adam Buchbinder <abuchbinder@google.com> wrote:
> 
> Passing in a negative integer would lead to a segfault, and
> a crafted filesystem image could trigger that.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> lib/e2p/ostype.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/e2p/ostype.c b/lib/e2p/ostype.c
> index 7f088446..c5fd8ab3 100644
> --- a/lib/e2p/ostype.c
> +++ b/lib/e2p/ostype.c
> @@ -30,7 +30,7 @@ char *e2p_os2string(int os_type)
>         const char	*os;
> 	char 		*ret;
> 
> -	if (os_type <= EXT2_OS_LITES)
> +	if (os_type >= 0 && os_type <= EXT2_OS_LITES)
> 		os = os_tab[os_type];
> 	else
> 		os = "(unknown os)";
> --
> 2.14.0.rc0.284.gd933b75aa4-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 1/2] e2p: Fix segfault in e2p_os2string.

[Theodore Ts'o]

On Tue, Jul 18, 2017 at 02:48:22PM -0700, Adam Buchbinder wrote:
> Passing in a negative integer would lead to a segfault, and
> a crafted filesystem image could trigger that.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

Thanks, applied.

					- Ted

[PATCH v2 2/2] tests: dumpe2fs against an image with bad os_type.

[Adam Buchbinder]

The os_type here is large enough to be negative when interpreted as a
signed integer.

This test case was generated by american fuzzy lop, starting from a
base filesystem image from files.fuzzing-project.org.

Signed-off-by: Adam Buchbinder <abuchbinder@google.com>
---
 tests/d_bad_ostype/expect |  1 +
 tests/d_bad_ostype/name   |  1 +
 tests/d_bad_ostype/script | 21 +++++++++++++++++++++
 3 files changed, 23 insertions(+)
 create mode 100644 tests/d_bad_ostype/expect
 create mode 100644 tests/d_bad_ostype/name
 create mode 100644 tests/d_bad_ostype/script

diff --git a/tests/d_bad_ostype/expect b/tests/d_bad_ostype/expect
new file mode 100644
index 00000000..b00318c4
--- /dev/null
+++ b/tests/d_bad_ostype/expect
@@ -0,0 +1 @@
+Filesystem OS type:       (unknown os)
diff --git a/tests/d_bad_ostype/name b/tests/d_bad_ostype/name
new file mode 100644
index 00000000..3da887a8
--- /dev/null
+++ b/tests/d_bad_ostype/name
@@ -0,0 +1 @@
+handle bad (negative) os_type
diff --git a/tests/d_bad_ostype/script b/tests/d_bad_ostype/script
new file mode 100644
index 00000000..992a3030
--- /dev/null
+++ b/tests/d_bad_ostype/script
@@ -0,0 +1,21 @@
+dd if=/dev/zero of=$TMPFILE bs=1k count=64 > /dev/null 2>&1
+$MKE2FS -q -b 1024 $TMPFILE
+$DEBUGFS -w -R 'set_super_value creator_os 0xf0000000' $TMPFILE
+
+OUT=$test_name.log
+EXP=$test_dir/expect
+$DUMPE2FS $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed | grep 'Filesystem OS type:' > $OUT
+
+rm -f $TMPFILE
+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
+unset OUT EXP
-- 
2.14.0.rc0.284.gd933b75aa4-goog

Re: [PATCH v2 2/2] tests: dumpe2fs against an image with bad os_type.

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Jul 18, 2017, at 3:48 PM, Adam Buchbinder <abuchbinder@google.com> wrote:
> 
> The os_type here is large enough to be negative when interpreted as a
> signed integer.
> 
> This test case was generated by american fuzzy lop, starting from a
> base filesystem image from files.fuzzing-project.org.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> tests/d_bad_ostype/expect |  1 +
> tests/d_bad_ostype/name   |  1 +
> tests/d_bad_ostype/script | 21 +++++++++++++++++++++
> 3 files changed, 23 insertions(+)
> create mode 100644 tests/d_bad_ostype/expect
> create mode 100644 tests/d_bad_ostype/name
> create mode 100644 tests/d_bad_ostype/script
> 
> diff --git a/tests/d_bad_ostype/expect b/tests/d_bad_ostype/expect
> new file mode 100644
> index 00000000..b00318c4
> --- /dev/null
> +++ b/tests/d_bad_ostype/expect
> @@ -0,0 +1 @@
> +Filesystem OS type:       (unknown os)
> diff --git a/tests/d_bad_ostype/name b/tests/d_bad_ostype/name
> new file mode 100644
> index 00000000..3da887a8
> --- /dev/null
> +++ b/tests/d_bad_ostype/name
> @@ -0,0 +1 @@
> +handle bad (negative) os_type
> diff --git a/tests/d_bad_ostype/script b/tests/d_bad_ostype/script
> new file mode 100644
> index 00000000..992a3030
> --- /dev/null
> +++ b/tests/d_bad_ostype/script
> @@ -0,0 +1,21 @@
> +dd if=/dev/zero of=$TMPFILE bs=1k count=64 > /dev/null 2>&1
> +$MKE2FS -q -b 1024 $TMPFILE
> +$DEBUGFS -w -R 'set_super_value creator_os 0xf0000000' $TMPFILE
> +
> +OUT=$test_name.log
> +EXP=$test_dir/expect
> +$DUMPE2FS $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed | grep 'Filesystem OS type:' > $OUT
> +
> +rm -f $TMPFILE
> +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
> +unset OUT EXP
> --
> 2.14.0.rc0.284.gd933b75aa4-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v2 2/2] tests: dumpe2fs against an image with bad os_type.

[Theodore Ts'o]

On Tue, Jul 18, 2017 at 02:48:23PM -0700, Adam Buchbinder wrote:
> The os_type here is large enough to be negative when interpreted as a
> signed integer.
> 
> This test case was generated by american fuzzy lop, starting from a
> base filesystem image from files.fuzzing-project.org.
> 
> Signed-off-by: Adam Buchbinder <abuchbinder@google.com>

Thanks, applied.

					- Ted