[v2] iconv: Accept redundant shift sequences in IBM1364 [BZ #26224]

  From: Arjun Shankar <arjun@redhat.com>

The IBM1364, IBM1371, IBM1388, IBM1390 and IBM1399 character sets
share converter logic (iconvdata/ibm1364.c) which would reject
redundant shift sequences when processing input in these character
sets.  This led to a hang in the iconv program (CVE-2020-27618).

This commit adjusts the converter to ignore redundant shift sequences
and adds test cases for iconv_prog hangs that would be triggered upon
their rejection.  This brings the implementation in line with other
converters that also ignore redundant shift sequences (e.g. IBM930
etc., fixed in commit 692de4b3960d).
---
v2: Added a NEWS entry.

 NEWS                    |  4 +++-
 iconv/tst-iconv_prog.sh | 16 ++++++++++------
 iconvdata/ibm1364.c     | 14 ++------------
 3 files changed, 15 insertions(+), 19 deletions(-)
  

On 11/3/20 5:21 PM, Arjun Shankar via Libc-alpha wrote:
> From: Arjun Shankar <arjun@redhat.com>

I was looking at some of this code recently while looking at
C.UTF-8 so I'll comment on this patch, otherwise I should be
working down my existing review list ;-)
 
> The IBM1364, IBM1371, IBM1388, IBM1390 and IBM1399 character sets
> share converter logic (iconvdata/ibm1364.c) which would reject
> redundant shift sequences when processing input in these character
> sets.  This led to a hang in the iconv program (CVE-2020-27618).
> 
> This commit adjusts the converter to ignore redundant shift sequences
> and adds test cases for iconv_prog hangs that would be triggered upon
> their rejection.  This brings the implementation in line with other
> converters that also ignore redundant shift sequences (e.g. IBM930
> etc., fixed in commit 692de4b3960d).
> ---
> v2: Added a NEWS entry.
> 
>  NEWS                    |  4 +++-
>  iconv/tst-iconv_prog.sh | 16 ++++++++++------
>  iconvdata/ibm1364.c     | 14 ++------------
>  3 files changed, 15 insertions(+), 19 deletions(-)

OK for master because it *does* fix the issue.

However, like in the original case, I'm curious why setting result
to __GCONV_ILLEGAL_INPUT and break-ing doesn't exit the converter
loop and return the error?

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> diff --git a/NEWS b/NEWS
> index 4307c4b1b0..0335fb98e5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -58,7 +58,9 @@ Changes to build and runtime requirements:
>  
>  Security related changes:
>  
> -  [Add security related changes here]
> +  CVE-2020-27618: An infinite loop has been fixed in the iconv program when
> +  invoked with input containing redundant shift sequences in the IBM1364,
> +  IBM1371, IBM1388, IBM1390, or IBM1399 character sets.

OK.

>  
>  The following bugs are resolved with this release:
>  
> diff --git a/iconv/tst-iconv_prog.sh b/iconv/tst-iconv_prog.sh
> index 8298136b7f..d8db7b335c 100644
> --- a/iconv/tst-iconv_prog.sh
> +++ b/iconv/tst-iconv_prog.sh
> @@ -102,12 +102,16 @@ hangarray=(
>  "\x00\x80;-c;IBM1161;UTF-8//TRANSLIT//IGNORE"
>  "\x00\xdb;-c;IBM1162;UTF-8//TRANSLIT//IGNORE"
>  "\x00\x70;-c;IBM12712;UTF-8//TRANSLIT//IGNORE"
> -# These are known hangs that are yet to be fixed:
> -# "\x00\x0f;-c;IBM1364;UTF-8"
> -# "\x00\x0f;-c;IBM1371;UTF-8"
> -# "\x00\x0f;-c;IBM1388;UTF-8"
> -# "\x00\x0f;-c;IBM1390;UTF-8"
> -# "\x00\x0f;-c;IBM1399;UTF-8"
> +"\x00\x0f;-c;IBM1364;UTF-8"
> +"\x0e\x0e;-c;IBM1364;UTF-8"
> +"\x00\x0f;-c;IBM1371;UTF-8"
> +"\x0e\x0e;-c;IBM1371;UTF-8"
> +"\x00\x0f;-c;IBM1388;UTF-8"
> +"\x0e\x0e;-c;IBM1388;UTF-8"
> +"\x00\x0f;-c;IBM1390;UTF-8"
> +"\x0e\x0e;-c;IBM1390;UTF-8"
> +"\x00\x0f;-c;IBM1399;UTF-8"
> +"\x0e\x0e;-c;IBM1399;UTF-8"

OK. Redundant SO+SO. Redundant SI when in sb mode.

>  "\x00\x53;-c;IBM16804;UTF-8//TRANSLIT//IGNORE"
>  "\x00\x41;-c;IBM274;UTF-8//TRANSLIT//IGNORE"
>  "\x00\x41;-c;IBM275;UTF-8//TRANSLIT//IGNORE"
> diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
> index 49e7267ab4..521f0825b7 100644
> --- a/iconvdata/ibm1364.c
> +++ b/iconvdata/ibm1364.c
> @@ -158,24 +158,14 @@ enum
>  									      \
>      if (__builtin_expect (ch, 0) == SO)					      \
>        {									      \
> -	/* Shift OUT, change to DBCS converter.  */			      \
> -	if (curcs == db)						      \
> -	  {								      \
> -	    result = __GCONV_ILLEGAL_INPUT;				      \
> -	    break;							      \
> -	  }								      \
> +	/* Shift OUT, change to DBCS converter (redundant escape okay).  */   \
>  	curcs = db;							      \
>  	++inptr;							      \
>  	continue;							      \
>        }									      \
>      if (__builtin_expect (ch, 0) == SI)					      \
>        {									      \
> -	/* Shift IN, change to SBCS converter.  */			      \
> -	if (curcs == sb)						      \
> -	  {								      \
> -	    result = __GCONV_ILLEGAL_INPUT;				      \
> -	    break;							      \
> -	  }								      \
> +	/* Shift IN, change to SBCS converter (redundant escape okay).  */    \
>  	curcs = sb;							      \
>  	++inptr;							      \
>  	continue;							      \
> 

OK. Verified only 2 instances of the problem, one for shift-in the other
for shift-out redundant sequences.