From 88cf9dec060e8c3f1705a4e41a30e283bc3c8b1b Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>

Date: Tue, 10 Jul 2018 14:34:35 +0200

Subject: [PATCH] Improve error handling in idn_ace_to_locale()

While idn2_to_unicode_8zlz() takes a 'flags' argument, it is ignored and

thus cannot be used to perform IDN checks on the output string.

The bug in libidn2 versions before 2.0.5 was not that a call to

idn2_to_unicode_8zlz() with certain flags set did not cause IDN checks

to be performed.  The bug was that idn2_to_unicode_8zlz() did not check

whether a conversion can be performed between UTF-8 and the current

locale's character encoding.  In other words, with libidn2 version

2.0.5+, if the current locale's character encoding is ASCII, then

idn2_to_unicode_8zlz() will fail when it is passed any Punycode string

which decodes to a non-ASCII string, even if it is a valid IDNA2008

name.

Rework idn_ace_to_locale() so that invalid IDNA2008 names are properly

and consistently detected for all libidn2 versions and locales.

Update the "idna" system test accordingly.  Add checks for processing a

server response containing Punycode which decodes to an invalid IDNA2008

name.  Fix invalid subtest description.

(cherry picked from commit 7fe0f00a3bf31dc26ee3dc8777e3076546b83990)

(cherry picked from commit 4fdee34a0b0ad142182914327a1bc53144b90ed1)

---

 bin/dig/dighost.c              | 72 ++++++++++++++++++++++++-------

 bin/tests/system/idna/tests.sh | 77 +++++++++++-----------------------

 2 files changed, 82 insertions(+), 67 deletions(-)

diff --git a/bin/dig/dighost.c b/bin/dig/dighost.c

index d46379ddc2..26528af3cc 100644

--- a/bin/dig/dighost.c

+++ b/bin/dig/dighost.c

@@ -4812,26 +4812,70 @@ idn_locale_to_ace(const char *from, char *to, size_t tolen) {

 static isc_result_t

 idn_ace_to_locale(const char *from, char *to, size_t tolen) {

 	int res;

-	char *tmp_str = NULL;

+	char *utf8_src, *tmp_str = NULL;

-	res = idn2_to_unicode_8zlz(from, &tmp_str,

-				   IDN2_NONTRANSITIONAL|IDN2_NFC_INPUT);

+	/*

+	 * We need to:

+	 *

+	 *  1) check whether 'from' is a valid IDNA2008 name,

+	 *  2) if it is, output it in the current locale's character encoding.

+	 *

+	 * Unlike idn2_to_ascii_*(), idn2_to_unicode_*() functions are unable

+	 * to perform IDNA2008 validity checks.  Thus, we need to decode any

+	 * Punycode in 'from', check if the resulting name is a valid IDNA2008

+	 * name, and only once we ensure it is, output that name in the current

+	 * locale's character encoding.

+	 *

+	 * We could just use idn2_to_unicode_8zlz() + idn2_to_ascii_lz(), but

+	 * then we would not be able to universally tell invalid names and

+	 * character encoding errors apart (if the current locale uses ASCII

+	 * for character encoding, the former function would fail even for a

+	 * valid IDNA2008 name, as long as it contained any non-ASCII

+	 * character).  Thus, we need to take a longer route.

+	 *

+	 * First, convert 'from' to UTF-8, ignoring the current locale.

+	 */

+	res = idn2_to_unicode_8z8z(from, &utf8_src, 0);

+	if (res != IDN2_OK) {

+		fatal("Bad ACE string '%s' (%s), use +noidnout",

+		      from, idn2_strerror(res));

+	}

-	if (res == IDN2_OK) {

-		/* check the length */

-		if (strlen(tmp_str) >= tolen) {

-			debug("encoded ASC string is too long");

-			idn2_free(tmp_str);

-			return ISC_R_FAILURE;

-		}

+	/*

+	 * Then, check whether decoded 'from' is a valid IDNA2008 name.

+	 */

+	res = idn2_to_ascii_8z(utf8_src, NULL, IDN2_NONTRANSITIONAL);

+	if (res != IDN2_OK) {

+		fatal("'%s' is not a legal IDNA2008 name (%s), use +noidnout",

+		      from, idn2_strerror(res));

+	}

-		(void) strlcpy(to, tmp_str, tolen);

+	/*

+	 * Finally, try converting the decoded 'from' into the current locale's

+	 * character encoding.

+	 */

+	res = idn2_to_unicode_8zlz(utf8_src, &tmp_str, 0);

+	if (res != IDN2_OK) {

+		fatal("Cannot represent '%s' in the current locale (%s), "

+		      "use +noidnout or a different locale",

+		      from, idn2_strerror(res));

+	}

+

+	/*

+	 * Free the interim conversion result.

+	 */

+	idn2_free(utf8_src);

+

+	/* check the length */

+	if (strlen(tmp_str) >= tolen) {

+		debug("encoded ASC string is too long");

 		idn2_free(tmp_str);

-		return ISC_R_SUCCESS;

+		return (ISC_R_FAILURE);

 	}

-	fatal("'%s' is not a legal IDN name (%s), use +noidnout", from, idn2_strerror(res));

-	return ISC_R_FAILURE;

+	(void) strlcpy(to, tmp_str, tolen);

+	idn2_free(tmp_str);

+	return (ISC_R_SUCCESS);

 }

 #endif /* WITH_IDN_OUT_SUPPORT */

 #endif /* WITH_LIBIDN2 */

diff --git a/bin/tests/system/idna/tests.sh b/bin/tests/system/idna/tests.sh

index 3a9b91b442..6637bf6828 100644

--- a/bin/tests/system/idna/tests.sh

+++ b/bin/tests/system/idna/tests.sh

@@ -136,46 +136,6 @@ idna_fail() {

     status=`expr $status + $ret`

 }

-# Check if current version of libidn2 is >= a given version

-#

-# This requires that:

-# a) "pkg-config" exists on the system

-# b) The libidn2 installed has an associated ".pc" file

-# c) The system sort command supports "-V"

-#

-# $1 - Minimum version required

-#

-# Returns:

-# 0 - Version check is OK, libidn2 at required version or greater.

-# 1 - Version check was made, but libidn2 not at required version.

-# 2 - Could not carry out version check

-

-libidn_version_check() {

-    ret=2

-    if [ -n "`command -v pkg-config`" ]; then

-        version=`pkg-config --modversion --silence-errors libidn2`

-        if [ -n "$version" ]; then

-            # Does the sort command have a "-V" flag on this system?

-            sort -V 2>&1 > /dev/null << .

-.

-            if [ $? -eq 0 ]; then

-                # Sort -V exists.  Sort the IDN version and the minimum version

-                # required.  If the IDN version is greater than or equal to that

-                # version, it will appear last in the list.

-                last_version=`printf "%s\n" $version $1 | sort -V | tail -1`

-                if [ "$version" = "$last_version" ]; then

-                    ret=0

-                else

-                    ret=1

-                fi

-            fi

-        fi

-    fi

-

-    return $ret

-}

-

-

 # Function to check that case is preserved for an all-ASCII label.

 #

 # Without IDNA support, case-preservation is the expected behavior.

@@ -310,16 +270,7 @@ idna_enabled_test() {

     text="Checking fake A-label"

     idna_fail "$text" ""                   "xn--ahahah"

     idna_test "$text" "+noidnin +noidnout" "xn--ahahah" "xn--ahahah."

-

-    # Owing to issues with libdns, the next test will fail for versions of

-    # libidn earlier than 2.0.5.  For this reason, get the version (if

-    # available) and compare with 2.0.5.

-    libidn_version_check 2.0.5

-    if [ $? -ne 0 ]; then

-        echo_i "Skipping fake A-label +noidnin +idnout test (libidn2 version issues)"

-    else

-        idna_test "$text" "+noidnin   +idnout" "xn--ahahah" "xn--ahahah."

-    fi

+    idna_fail "$text" "+noidnin   +idnout" "xn--ahahah"

     idna_fail "$text" "+idnin   +noidnout" "xn--ahahah"

     idna_fail "$text" "+idnin     +idnout" "xn--ahahah"

@@ -327,7 +278,7 @@ idna_enabled_test() {

     # BIND rejects such labels: with +idnin

     label="xn--xflod18hstflod18hstflod18hstflod18hstflod18hstflod18-1iejjjj"

-    text="Checking punycode label shorter than minimum valid length"

+    text="Checking punycode label longer than maximum valid length"

     idna_fail "$text" ""                   "$label"

     idna_fail "$text" "+noidnin +noidnout" "$label"

     idna_fail "$text" "+noidnin   +idnout" "$label"

@@ -337,7 +288,7 @@ idna_enabled_test() {

-    # Tests of a valid unicode string but an invalid U-label

+    # Tests of a valid unicode string but an invalid U-label (input)

     #

     # Symbols are not valid IDNA names.

     #

@@ -347,12 +298,32 @@ idna_enabled_test() {

     #

     # The +[no]idnout options should not have any effect on the test.

-    text="Checking invalid U-label"

+    text="Checking invalid input U-label"

     idna_fail "$text" ""                   "🧦.com"

     idna_test "$text" "+noidnin +noidnout" "🧦.com" "\240\159\167\166.com."

     idna_test "$text" "+noidnin +idnout"   "🧦.com" "\240\159\167\166.com."

     idna_fail "$text" "+idnin   +noidnout" "🧦.com"

     idna_fail "$text" "+idnin   +idnout"   "🧦.com"

+

+    # Tests of a valid unicode string but an invalid U-label (output)

+    #

+    # Symbols are not valid IDNA names.

+    #

+    # Note that an invalid U-label is accepted even when +idnin is in effect

+    # because "xn--19g" is valid Punycode.

+    #

+    # +noidnout: "dig" should send the ACE string to the server and display the

+    #            returned qname.

+    # +idnout:   "dig" should generate an error.

+    #

+    # The +[no]idnin options should not have any effect on the test.

+

+    text="Checking invalid output U-label"

+    idna_fail "$text" ""                   "xn--19g"

+    idna_test "$text" "+noidnin +noidnout" "xn--19g" "xn--19g."

+    idna_fail "$text" "+noidnin +idnout"   "xn--19g"

+    idna_test "$text" "+idnin   +noidnout" "xn--19g" "xn--19g."

+    idna_fail "$text" "+idnin   +idnout"   "xn--19g"

 }

-- 

2.20.1