[PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Florian Weimer]

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

These changes are based on the fix for BZ #14134 in commit 
6e230d11837f3ae7b375ea69d7905f0d18eb79e5.  My analysis showed that the 
0xffff case was impossible with certain character sets, so I added 
asserts there.

Tested on master with no failures.

I will request CVE IDs (for this fix and the 2012 commit) on oss-security.

-- 
Florian Weimer / Red Hat Product Security

[-- Attachment #2: 0001-Fix-crashes-on-invalid-input-in-IBM-gconv-modules-BZ.patch --]
[-- Type: text/x-patch, Size: 7267 bytes --]

From f787724b8a7036804a789737cc0fea040f7b999c Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Fri, 29 Aug 2014 13:16:49 +0200
Subject: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

These changes are based on the fix for BZ #14134 in commit
6e230d11837f3ae7b375ea69d7905f0d18eb79e5.

2014-08-29  Florian Weimer  <fweimer@redhat.com>

	[BZ #17325]
	* iconvdata/ibm1364.c (BODY): Fix check for sentinel.
	* iconvdata/ibm932.c (BODY): Replace invalid sentinel check with
	assert.
	* iconvdata/ibm933.c (BODY): Fix check for sentinel.
	* iconvdata/ibm935.c (BODY): Likewise.
	* iconvdata/ibm937.c (BODY): Likewise.
	* iconvdata/ibm939.c (BODY): Likewise.
	* iconvdata/ibm943.c (BODY): Replace invalid sentinel check with
	assert.
	* iconvdata/Makefile (iconv-test.out): Pass module list to test
	script.
	* iconvdata/run-iconv-test.sh: New test loop for checking for
	decoder crashers.

diff --git a/NEWS b/NEWS
index 1af9e70..dd93f2f 100644
--- a/NEWS
+++ b/NEWS
@@ -23,7 +23,7 @@ Version 2.20
   16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
   17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
   17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
-  17187, 17213, 17259, 17261, 17262, 17263, 17319.
+  17187, 17213, 17259, 17261, 17262, 17263, 17319, 17325.
 
 * Reverted change of ABI data structures for s390 and s390x:
   On s390 and s390x the size of struct ucontext and jmp_buf was increased in
@@ -115,6 +115,11 @@ Version 2.20
   normal gconv conversion modules are still supported.  Transliteration
   with //TRANSLIT is still possible, and the //IGNORE specifier
   continues to be  supported. (CVE-2014-5119)
+
+* Decoding a crafted input sequence in the character sets IBM933, IBM935,
+  IBM937, IBM939, IBM1364 could result in an out-of-bounds array read,
+  resulting a denial-of-service security vulnerability in applications which
+  use functions related to iconv.
 \f
 Version 2.19
 
diff --git a/iconvdata/Makefile b/iconvdata/Makefile
index 0a410a1..b6327d6 100644
--- a/iconvdata/Makefile
+++ b/iconvdata/Makefile
@@ -297,6 +297,7 @@ $(objpfx)tst-iconv7.out: $(objpfx)gconv-modules \
 $(objpfx)iconv-test.out: run-iconv-test.sh $(objpfx)gconv-modules \
 			 $(addprefix $(objpfx),$(modules.so)) \
 			 $(common-objdir)/iconv/iconv_prog TESTS
+	iconv_modules="$(modules)" \
 	$(SHELL) $< $(common-objdir) '$(test-wrapper-env)' \
 		 '$(run-program-env)' > $@; \
 	$(evaluate-test)
diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
index 0b5484f..cf80993 100644
--- a/iconvdata/ibm1364.c
+++ b/iconvdata/ibm1364.c
@@ -221,7 +221,8 @@ enum
 	  ++rp2;							      \
 									      \
 	uint32_t res;							      \
-	if (__builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
+	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = DB_TO_UCS4[ch + rp2->idx],			      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
 	  {								      \
diff --git a/iconvdata/ibm932.c b/iconvdata/ibm932.c
index f5dca59..aa69d65 100644
--- a/iconvdata/ibm932.c
+++ b/iconvdata/ibm932.c
@@ -74,11 +74,12 @@
 	  }								      \
 									      \
 	ch = (ch * 0x100) + inptr[1];					      \
+	/* ch was less than 0xfd.  */					      \
+	assert (ch < 0xfd00);						      \
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
-	    || __builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm932db_to_ucs4[ch + rp2->idx],		      \
 	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
 	  {								      \
diff --git a/iconvdata/ibm933.c b/iconvdata/ibm933.c
index f46dfb5..461fb5e 100644
--- a/iconvdata/ibm933.c
+++ b/iconvdata/ibm933.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm933db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm935.c b/iconvdata/ibm935.c
index a8e4e6c..56babb8 100644
--- a/iconvdata/ibm935.c
+++ b/iconvdata/ibm935.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (ch == 0xffff, 0)				      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm935db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm937.c b/iconvdata/ibm937.c
index 239be61..1ba340f 100644
--- a/iconvdata/ibm937.c
+++ b/iconvdata/ibm937.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (ch == 0xffff, 0)				      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm937db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm939.c b/iconvdata/ibm939.c
index 5d0db36..16caa62 100644
--- a/iconvdata/ibm939.c
+++ b/iconvdata/ibm939.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (ch == 0xffff, 0)				      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm939db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm943.c b/iconvdata/ibm943.c
index be0c14f..c5d5742 100644
--- a/iconvdata/ibm943.c
+++ b/iconvdata/ibm943.c
@@ -75,11 +75,12 @@
 	  }								      \
 									      \
 	ch = (ch * 0x100) + inptr[1];					      \
+	/* ch was less than 0xfd.  */					      \
+	assert (ch < 0xfd00);						      \
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
-	    || __builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm943db_to_ucs4[ch + rp2->idx],		      \
 	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
 	  {								      \
diff --git a/iconvdata/run-iconv-test.sh b/iconvdata/run-iconv-test.sh
index c98c929..5dfb69f 100755
--- a/iconvdata/run-iconv-test.sh
+++ b/iconvdata/run-iconv-test.sh
@@ -184,6 +184,24 @@ while read utf8 from filename; do
 
 done < TESTS2
 
+# Check for crashes in decoders.
+printf '\016\377\377\377\377\377\377\377' > $temp1
+for from in $iconv_modules ; do
+    echo $ac_n "test decoder $from $ac_c"
+    PROG=`eval echo $ICONV`
+    if $PROG < $temp1 >/dev/null 2>&1 ; then
+	: # fall through
+    else
+	status=$?
+	if test $status -gt 1 ; then
+	    echo "/FAILED"
+	    failed=1
+	    continue
+	fi
+    fi
+    echo "OK"
+done
+
 exit $failed
 # Local Variables:
 #  mode:shell-script
-- 
1.9.3

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Adhemerval Zanella]

It is ok, thanks for fixing it.

On 29-08-2014 09:13, Florian Weimer wrote:
> These changes are based on the fix for BZ #14134 in commit 6e230d11837f3ae7b375ea69d7905f0d18eb79e5.  My analysis showed that the 0xffff case was impossible with certain character sets, so I added asserts there.
>
> Tested on master with no failures.
>
> I will request CVE IDs (for this fix and the 2012 commit) on oss-security.
>
> -- 
> Florian Weimer / Red Hat Product Security
>
> From f787724b8a7036804a789737cc0fea040f7b999c Mon Sep 17 00:00:00 2001
> From: Florian Weimer <fweimer@redhat.com>
> Date: Fri, 29 Aug 2014 13:16:49 +0200
> Subject: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]
>
> These changes are based on the fix for BZ #14134 in commit
> 6e230d11837f3ae7b375ea69d7905f0d18eb79e5.
>
> 2014-08-29  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #17325]
> 	* iconvdata/ibm1364.c (BODY): Fix check for sentinel.
> 	* iconvdata/ibm932.c (BODY): Replace invalid sentinel check with
> 	assert.
> 	* iconvdata/ibm933.c (BODY): Fix check for sentinel.
> 	* iconvdata/ibm935.c (BODY): Likewise.
> 	* iconvdata/ibm937.c (BODY): Likewise.
> 	* iconvdata/ibm939.c (BODY): Likewise.
> 	* iconvdata/ibm943.c (BODY): Replace invalid sentinel check with
> 	assert.
> 	* iconvdata/Makefile (iconv-test.out): Pass module list to test
> 	script.
> 	* iconvdata/run-iconv-test.sh: New test loop for checking for
> 	decoder crashers.
>
> diff --git a/NEWS b/NEWS
> index 1af9e70..dd93f2f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,7 +23,7 @@ Version 2.20
>    16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
>    17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
>    17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
> -  17187, 17213, 17259, 17261, 17262, 17263, 17319.
> +  17187, 17213, 17259, 17261, 17262, 17263, 17319, 17325.
>
>  * Reverted change of ABI data structures for s390 and s390x:
>    On s390 and s390x the size of struct ucontext and jmp_buf was increased in
> @@ -115,6 +115,11 @@ Version 2.20
>    normal gconv conversion modules are still supported.  Transliteration
>    with //TRANSLIT is still possible, and the //IGNORE specifier
>    continues to be  supported. (CVE-2014-5119)
> +
> +* Decoding a crafted input sequence in the character sets IBM933, IBM935,
> +  IBM937, IBM939, IBM1364 could result in an out-of-bounds array read,
> +  resulting a denial-of-service security vulnerability in applications which
> +  use functions related to iconv.
>  
>  Version 2.19
>
> diff --git a/iconvdata/Makefile b/iconvdata/Makefile
> index 0a410a1..b6327d6 100644
> --- a/iconvdata/Makefile
> +++ b/iconvdata/Makefile
> @@ -297,6 +297,7 @@ $(objpfx)tst-iconv7.out: $(objpfx)gconv-modules \
>  $(objpfx)iconv-test.out: run-iconv-test.sh $(objpfx)gconv-modules \
>  			 $(addprefix $(objpfx),$(modules.so)) \
>  			 $(common-objdir)/iconv/iconv_prog TESTS
> +	iconv_modules="$(modules)" \
>  	$(SHELL) $< $(common-objdir) '$(test-wrapper-env)' \
>  		 '$(run-program-env)' > $@; \
>  	$(evaluate-test)
> diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
> index 0b5484f..cf80993 100644
> --- a/iconvdata/ibm1364.c
> +++ b/iconvdata/ibm1364.c
> @@ -221,7 +221,8 @@ enum
>  	  ++rp2;							      \
>  									      \
>  	uint32_t res;							      \
> -	if (__builtin_expect (ch < rp2->start, 0)			      \
> +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
> +	    || __builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = DB_TO_UCS4[ch + rp2->idx],			      \
>  		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
>  	  {								      \
> diff --git a/iconvdata/ibm932.c b/iconvdata/ibm932.c
> index f5dca59..aa69d65 100644
> --- a/iconvdata/ibm932.c
> +++ b/iconvdata/ibm932.c
> @@ -74,11 +74,12 @@
>  	  }								      \
>  									      \
>  	ch = (ch * 0x100) + inptr[1];					      \
> +	/* ch was less than 0xfd.  */					      \
> +	assert (ch < 0xfd00);						      \
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> -	    || __builtin_expect (ch < rp2->start, 0)			      \
> +	if (__builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm932db_to_ucs4[ch + rp2->idx],		      \
>  	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
>  	  {								      \
> diff --git a/iconvdata/ibm933.c b/iconvdata/ibm933.c
> index f46dfb5..461fb5e 100644
> --- a/iconvdata/ibm933.c
> +++ b/iconvdata/ibm933.c
> @@ -162,7 +162,7 @@ enum
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
>  	    || __builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm933db_to_ucs4[ch + rp2->idx],		      \
>  		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
> diff --git a/iconvdata/ibm935.c b/iconvdata/ibm935.c
> index a8e4e6c..56babb8 100644
> --- a/iconvdata/ibm935.c
> +++ b/iconvdata/ibm935.c
> @@ -162,7 +162,7 @@ enum
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> +	if (__builtin_expect (ch == 0xffff, 0)				      \
>  	    || __builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm935db_to_ucs4[ch + rp2->idx],		      \
>  		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
> diff --git a/iconvdata/ibm937.c b/iconvdata/ibm937.c
> index 239be61..1ba340f 100644
> --- a/iconvdata/ibm937.c
> +++ b/iconvdata/ibm937.c
> @@ -162,7 +162,7 @@ enum
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> +	if (__builtin_expect (ch == 0xffff, 0)				      \
>  	    || __builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm937db_to_ucs4[ch + rp2->idx],		      \
>  		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
> diff --git a/iconvdata/ibm939.c b/iconvdata/ibm939.c
> index 5d0db36..16caa62 100644
> --- a/iconvdata/ibm939.c
> +++ b/iconvdata/ibm939.c
> @@ -162,7 +162,7 @@ enum
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> +	if (__builtin_expect (ch == 0xffff, 0)				      \
>  	    || __builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm939db_to_ucs4[ch + rp2->idx],		      \
>  		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
> diff --git a/iconvdata/ibm943.c b/iconvdata/ibm943.c
> index be0c14f..c5d5742 100644
> --- a/iconvdata/ibm943.c
> +++ b/iconvdata/ibm943.c
> @@ -75,11 +75,12 @@
>  	  }								      \
>  									      \
>  	ch = (ch * 0x100) + inptr[1];					      \
> +	/* ch was less than 0xfd.  */					      \
> +	assert (ch < 0xfd00);						      \
>  	while (ch > rp2->end)						      \
>  	  ++rp2;							      \
>  									      \
> -	if (__builtin_expect (rp2 == NULL, 0)				      \
> -	    || __builtin_expect (ch < rp2->start, 0)			      \
> +	if (__builtin_expect (ch < rp2->start, 0)			      \
>  	    || (res = __ibm943db_to_ucs4[ch + rp2->idx],		      \
>  	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
>  	  {								      \
> diff --git a/iconvdata/run-iconv-test.sh b/iconvdata/run-iconv-test.sh
> index c98c929..5dfb69f 100755
> --- a/iconvdata/run-iconv-test.sh
> +++ b/iconvdata/run-iconv-test.sh
> @@ -184,6 +184,24 @@ while read utf8 from filename; do
>
>  done < TESTS2
>
> +# Check for crashes in decoders.
> +printf '\016\377\377\377\377\377\377\377' > $temp1
> +for from in $iconv_modules ; do
> +    echo $ac_n "test decoder $from $ac_c"
> +    PROG=`eval echo $ICONV`
> +    if $PROG < $temp1 >/dev/null 2>&1 ; then
> +	: # fall through
> +    else
> +	status=$?
> +	if test $status -gt 1 ; then
> +	    echo "/FAILED"
> +	    failed=1
> +	    continue
> +	fi
> +    fi
> +    echo "OK"
> +done
> +
>  exit $failed
>  # Local Variables:
>  #  mode:shell-script
> -- 1.9.3

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \

Please use either this

> +	if (__builtin_expect (ch == 0xffff, 0)				      \

or this consistently.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Florian Weimer]

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

On 08/29/2014 03:33 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
>
> Please use either this
>
>> +	if (__builtin_expect (ch == 0xffff, 0)				      \
>
> or this consistently.

Oops, you are right.  I went with the first variant, as in the original fix.

Retested successfully.

Allan, is this okay for master at this stage?

-- 
Florian Weimer / Red Hat Product Security

[-- Attachment #2: 0001-Fix-crashes-on-invalid-input-in-IBM-gconv-modules-BZ.patch --]
[-- Type: text/x-patch, Size: 7288 bytes --]

From 1819abce68402c06b43621a4168828bb95c40bc1 Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Fri, 29 Aug 2014 13:16:49 +0200
Subject: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

These changes are based on the fix for BZ #14134 in commit
6e230d11837f3ae7b375ea69d7905f0d18eb79e5.

2014-08-29  Florian Weimer  <fweimer@redhat.com>

	[BZ #17325]
	* iconvdata/ibm1364.c (BODY): Fix check for sentinel.
	* iconvdata/ibm932.c (BODY): Replace invalid sentinel check with
	assert.
	* iconvdata/ibm933.c (BODY): Fix check for sentinel.
	* iconvdata/ibm935.c (BODY): Likewise.
	* iconvdata/ibm937.c (BODY): Likewise.
	* iconvdata/ibm939.c (BODY): Likewise.
	* iconvdata/ibm943.c (BODY): Replace invalid sentinel check with
	assert.
	* iconvdata/Makefile (iconv-test.out): Pass module list to test
	script.
	* iconvdata/run-iconv-test.sh: New test loop for checking for
	decoder crashers.

diff --git a/NEWS b/NEWS
index 1af9e70..dd93f2f 100644
--- a/NEWS
+++ b/NEWS
@@ -23,7 +23,7 @@ Version 2.20
   16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
   17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
   17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
-  17187, 17213, 17259, 17261, 17262, 17263, 17319.
+  17187, 17213, 17259, 17261, 17262, 17263, 17319, 17325.
 
 * Reverted change of ABI data structures for s390 and s390x:
   On s390 and s390x the size of struct ucontext and jmp_buf was increased in
@@ -115,6 +115,11 @@ Version 2.20
   normal gconv conversion modules are still supported.  Transliteration
   with //TRANSLIT is still possible, and the //IGNORE specifier
   continues to be  supported. (CVE-2014-5119)
+
+* Decoding a crafted input sequence in the character sets IBM933, IBM935,
+  IBM937, IBM939, IBM1364 could result in an out-of-bounds array read,
+  resulting a denial-of-service security vulnerability in applications which
+  use functions related to iconv.
 \f
 Version 2.19
 
diff --git a/iconvdata/Makefile b/iconvdata/Makefile
index 0a410a1..b6327d6 100644
--- a/iconvdata/Makefile
+++ b/iconvdata/Makefile
@@ -297,6 +297,7 @@ $(objpfx)tst-iconv7.out: $(objpfx)gconv-modules \
 $(objpfx)iconv-test.out: run-iconv-test.sh $(objpfx)gconv-modules \
 			 $(addprefix $(objpfx),$(modules.so)) \
 			 $(common-objdir)/iconv/iconv_prog TESTS
+	iconv_modules="$(modules)" \
 	$(SHELL) $< $(common-objdir) '$(test-wrapper-env)' \
 		 '$(run-program-env)' > $@; \
 	$(evaluate-test)
diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
index 0b5484f..cf80993 100644
--- a/iconvdata/ibm1364.c
+++ b/iconvdata/ibm1364.c
@@ -221,7 +221,8 @@ enum
 	  ++rp2;							      \
 									      \
 	uint32_t res;							      \
-	if (__builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
+	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = DB_TO_UCS4[ch + rp2->idx],			      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
 	  {								      \
diff --git a/iconvdata/ibm932.c b/iconvdata/ibm932.c
index f5dca59..aa69d65 100644
--- a/iconvdata/ibm932.c
+++ b/iconvdata/ibm932.c
@@ -74,11 +74,12 @@
 	  }								      \
 									      \
 	ch = (ch * 0x100) + inptr[1];					      \
+	/* ch was less than 0xfd.  */					      \
+	assert (ch < 0xfd00);						      \
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
-	    || __builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm932db_to_ucs4[ch + rp2->idx],		      \
 	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
 	  {								      \
diff --git a/iconvdata/ibm933.c b/iconvdata/ibm933.c
index f46dfb5..461fb5e 100644
--- a/iconvdata/ibm933.c
+++ b/iconvdata/ibm933.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm933db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm935.c b/iconvdata/ibm935.c
index a8e4e6c..132d816 100644
--- a/iconvdata/ibm935.c
+++ b/iconvdata/ibm935.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm935db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm937.c b/iconvdata/ibm937.c
index 239be61..69b154d 100644
--- a/iconvdata/ibm937.c
+++ b/iconvdata/ibm937.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm937db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm939.c b/iconvdata/ibm939.c
index 5d0db36..9936e2c 100644
--- a/iconvdata/ibm939.c
+++ b/iconvdata/ibm939.c
@@ -162,7 +162,7 @@ enum
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
 	    || __builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm939db_to_ucs4[ch + rp2->idx],		      \
 		__builtin_expect (res, L'\1') == L'\0' && ch != '\0'))	      \
diff --git a/iconvdata/ibm943.c b/iconvdata/ibm943.c
index be0c14f..c5d5742 100644
--- a/iconvdata/ibm943.c
+++ b/iconvdata/ibm943.c
@@ -75,11 +75,12 @@
 	  }								      \
 									      \
 	ch = (ch * 0x100) + inptr[1];					      \
+	/* ch was less than 0xfd.  */					      \
+	assert (ch < 0xfd00);						      \
 	while (ch > rp2->end)						      \
 	  ++rp2;							      \
 									      \
-	if (__builtin_expect (rp2 == NULL, 0)				      \
-	    || __builtin_expect (ch < rp2->start, 0)			      \
+	if (__builtin_expect (ch < rp2->start, 0)			      \
 	    || (res = __ibm943db_to_ucs4[ch + rp2->idx],		      \
 	    __builtin_expect (res, '\1') == 0 && ch !=0))		      \
 	  {								      \
diff --git a/iconvdata/run-iconv-test.sh b/iconvdata/run-iconv-test.sh
index c98c929..5dfb69f 100755
--- a/iconvdata/run-iconv-test.sh
+++ b/iconvdata/run-iconv-test.sh
@@ -184,6 +184,24 @@ while read utf8 from filename; do
 
 done < TESTS2
 
+# Check for crashes in decoders.
+printf '\016\377\377\377\377\377\377\377' > $temp1
+for from in $iconv_modules ; do
+    echo $ac_n "test decoder $from $ac_c"
+    PROG=`eval echo $ICONV`
+    if $PROG < $temp1 >/dev/null 2>&1 ; then
+	: # fall through
+    else
+	status=$?
+	if test $status -gt 1 ; then
+	    echo "/FAILED"
+	    failed=1
+	    continue
+	fi
+    fi
+    echo "OK"
+done
+
 exit $failed
 # Local Variables:
 #  mode:shell-script
-- 
1.9.3

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Roland McGrath]

> Florian Weimer <fweimer@redhat.com> writes:
> 
> > +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
> 
> Please use either this
> 
> > +	if (__builtin_expect (ch == 0xffff, 0)				      \
> 
> or this consistently.

Use neither.  Use __glibc_{un,}likely consistently.

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Florian Weimer]

On 08/29/2014 11:54 PM, Roland McGrath wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> +	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
>>
>> Please use either this
>>
>>> +	if (__builtin_expect (ch == 0xffff, 0)				      \
>>
>> or this consistently.
>
> Use neither.  Use __glibc_{un,}likely consistently.

I would like to do this in a future cleanup across all gconv modules, 
after the 2.20 release.  For this patch, I went with the existing style 
in the changed files.  This also simplifies backporting.

-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Ondřej Bílka]

On Fri, Aug 29, 2014 at 04:32:24PM +0200, Florian Weimer wrote:
> On 08/29/2014 03:33 PM, Andreas Schwab wrote:
> >Florian Weimer <fweimer@redhat.com> writes:
> >
> >>+	if (__builtin_expect (rp2->start == 0xffff, 0)			      \
> >
> >Please use either this
> >
> >>+	if (__builtin_expect (ch == 0xffff, 0)				      \
> >
> >or this consistently.
> 
> Oops, you are right.  I went with the first variant, as in the original fix.
> 
> Retested successfully.
> 
> Allan, is this okay for master at this stage?
> 
Anyway when we go for consistency wouldn't be better just use
libc_likely/unlikely?

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Allan McRae]

On 30/08/14 00:32, Florian Weimer wrote:
> On 08/29/2014 03:33 PM, Andreas Schwab wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>
>>> +    if (__builtin_expect (rp2->start == 0xffff, 0)                  \
>>
>> Please use either this
>>
>>> +    if (__builtin_expect (ch == 0xffff, 0)                      \
>>
>> or this consistently.
> 
> Oops, you are right.  I went with the first variant, as in the original
> fix.
> 
> Retested successfully.
> 
> Allan, is this okay for master at this stage?
> 

I am OK with this, once there is an ack to use __builtin_expect instead
of __glibc_likely.

I agree with you that the cleanup of all the  __builtin_expect should
happen in a following patch so the change is minimal at this stage and
easy to backport.

Allan

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Carlos O'Donell]

On 08/30/2014 06:26 AM, Florian Weimer wrote:
> On 08/29/2014 11:54 PM, Roland McGrath wrote:
>>> Florian Weimer <fweimer@redhat.com> writes:
>>>
>>>> +    if (__builtin_expect (rp2->start == 0xffff, 0)                  \
>>>
>>> Please use either this
>>>
>>>> +    if (__builtin_expect (ch == 0xffff, 0)                      \
>>>
>>> or this consistently.
>>
>> Use neither.  Use __glibc_{un,}likely consistently.
> 
> I would like to do this in a future cleanup across all gconv modules,
> after the 2.20 release. For this patch, I went with the existing
> style in the changed files. This also simplifies backporting.

At this late in the 2.20 freeze the CVE fix should be the minimal
change possible that fixes the bug for 2.20.

You get an ACK from me to use __builtin_expect for now, since it
also simplified backports of this security bug fix by minimally
touching code.

I am however holding you responsible to cleanup the uses after
2.20 branches >:-)

Cheers,
Carlos.

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Florian Weimer]

On 09/03/2014 06:37 PM, Carlos O'Donell wrote:
> You get an ACK from me to use __builtin_expect for now, since it
> also simplified backports of this security bug fix by minimally
> touching code.

Thanks, I've committed the change to master.

> I am however holding you responsible to cleanup the uses after
> 2.20 branches >:-)

Sure, let's get 2.20 out soon, so that I don't forget about it. :-)


-- 
Florian Weimer / Red Hat Product Security

Re: [PATCH] Fix crashes on invalid input in IBM gconv modules [BZ #17325]

[Roland McGrath]

> I would like to do this in a future cleanup across all gconv modules, 
> after the 2.20 release.  For this patch, I went with the existing style 
> in the changed files.  This also simplifies backporting.

That's certainly fine.  If you noticed a thing like that when doing the
change, it's a good idea to mention explicitly in the posting what future
cleanups you intend to do.  Without that, people like me are liable to
simply review each line you touched generically for how it should look in
the end.