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

[Carlos O'Donell]

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.

-- 
Cheers,
Carlos.