| Message ID | CAMe9rOroY25YVfHtz1WUwkE_yQYrdj=5NBX0MareEendtRBYyQ@mail.gmail.com |
|---|---|
| State | Committed |
| Headers |
Received: (qmail 120120 invoked by alias); 23 May 2018 16:44:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 120093 invoked by uid 89); 23 May 2018 16:44:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-11.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=8:he, 8:=c3=a4, 8:n., 8:ch?= X-HELO: mail-ot0-f196.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=vKk9LZ03GFggxS5o+PCzYws/+bI9kY03Fo3YvPS6bFQ=; b=HqIu5QWThLYgTcrVnNMZimSaIVHiJkSHSywMKcgSBC9zASCqYYzkkiy8fTfQGQDLPF EXgTejf9uICBLJ/69liDXmMV693ibMGTlINg6siPdjrgYn9jG7xGsX6BMotuvu/Ha2CV d1FpFv5kR3Xa9EOkzKE27SP/EaLKN2MBpmURKI/oPK5Df50Vo7IQS4yGkkq764sqGl8J MB3REC4MK4Ku2drIy7SbXH69HOptFoCGR7TG+nK+eyYLcc8s95AgnuFNsCbz7SvDsStP /T0gdAfb7Ccpc/WS0qu+Eaf4DVZByRt3RNFj/wynMWmFD09ViBjX1fDmWOiIbrbm2t+V 43Bw== X-Gm-Message-State: ALKqPwdhAXSNqs5eZo7GkbQ3JslB2c/t913ijwvCRlaJ1iVer2TLah+0 K9rwhM8DnrvjcLUde9q5PvIxH+g+P5R0pFg6y/Y= X-Google-Smtp-Source: AB8JxZpAn2asrCZDOsGVR1ZHuW3NxTJGClN+qTwmyLJfY4fNtnjRWG6DYOuNOJzqKu8sO7UNJgBW+yvTop2ghfzPVmk= X-Received: by 2002:a9d:441d:: with SMTP id u29-v6mr2504278ote.70.1527093880078; Wed, 23 May 2018 09:44:40 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180523132942.17F52402B59C6@oldenburg.str.redhat.com> References: <20180523132942.17F52402B59C6@oldenburg.str.redhat.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Wed, 23 May 2018 09:44:39 -0700 Message-ID: <CAMe9rOroY25YVfHtz1WUwkE_yQYrdj=5NBX0MareEendtRBYyQ@mail.gmail.com> Subject: Re: [PATCH COMMITTED] Switch IDNA implementation to libidn2 [BZ #19728] [BZ #19729] [BZ #22247] To: Florian Weimer <fweimer@redhat.com> Cc: GNU C Library <libc-alpha@sourceware.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable |
Commit Message
H.J. Lu
May 23, 2018, 4:44 p.m. UTC
On Wed, May 23, 2018 at 6:29 AM, Florian Weimer <fweimer@redhat.com> wrote: > This provides an implementation of the IDNA2008 standard and fixes > CVE-2016-6261, CVE-2016-6263, CVE-2017-14062. > > 2018-05-23 Florian Weimer <fweimer@redhat.com> > > [BZ #19728] > [BZ #19729] > [BZ #22247] > CVE-2016-6261 > CVE-2016-6263 > CVE-2017-14062 > Switch to extern IDNA implementation (libidn2). > * libidn: Remove subdirectory. > * LICENSES: Do not mention licensing conditions for the removed > libidn code. > * config.h.in (HAVE_LIBIDN): Remove. > * include/dlfcn.h (__libc_dlopen): Update comment. > * include/idna.h: Remove file. > * inet/Makefile (routines): Add idna. > (tests-static, tests-internal): Add tst-idna_name_classify. > (LOCALES): Generate locales for tests. > (tst-idna_name_classify.out): Depend on generated locales. > * inet/idna_name_classify.c: New file. > * inet/tst-idna_name_classify.c: Likewise. > * inet/net-internal.h (__idna_to_dns_encoding) > (__idna_from_dns_encoding): Declare. > * inet/net-internal.h (enum idna_name_classification): Define. > (__idna_name_classify): Declare. > * inet/Versions (GLIBC_PRIVATE): Add __idna_to_dns_encoding, > __idna_from_dns_encoding. > * inet/getnameinfo.c (DEPRECATED_NI_IDN): Define. > (gni_host_inet_name): Call __idna_from_dns_encoding. Use punycode > name as a fallback in case of encoding errors. > (getnameinfo): Use DEPRECATED_NI_IDN. > * inet/idna.c: New file. > * nscd/gai.c: Do not include <libidn/idn-stub.c>. > * resolv/Makefile (tests): Add tst-resolv-ai_idn, > tst-resolv-ai_idn-latin1, tst-resolv-ai_idn-nolibidn2. > (modules-names): Add tst-no-libidn2. > (extra-test-objs): Add tst-no-libidn2.os. > (LDFLAGS-tst-no-libidn2.so): Set soname. > (LOCALES): Set, and generate locales. > (tst-resolv-ai_idn): Link with -ldl -lresolv -lpthread. > (tst-resolv-ai_idn-latin1): Likewise. > (tst-resolv-ai_idn-nolibidn2): Likewise. > (tst-resolv-ai_idn.out): Depend on locales. > (tst-resolv-ai_idn-latin1.out): Depend on locales. > (tst-resolv-ai_idn-nolibidn2.out): Depend on locales and > tst-no-libidn2.so. > * resolv/netdb.h (AI_IDN_ALLOW_UNASSIGNED) > (AI_IDN_USE_STD3_ASCII_RULES, NI_IDN_ALLOW_UNASSIGNED) > (NI_IDN_USE_STD3_ASCII_RULES): Deprecate. > * resolv/tst-resolv-ai_idn.c: New file. > * resolv/tst-resolv-ai_idn-latin1.c: Likewise. > * resolv/tst-resolv-ai_idn-nolibidn2.c: Likewise. > * resolv/tst-no-libidn2.c: Likewise. > * support/support_format_addrinfo.c (format_ai_flags): Do not > handle AI_IDN_ALLOW_UNASSIGNED, AI_IDN_USE_STD3_ASCII_RULES. > * sysdeps/posix/getaddrinfo.c (DEPRECATED_AI_IDN): Define. > (gaih_inet): Call __idna_to_dns_encoding and > __idna_from_dns_encoding, and use the original (punycode) name if > __idna_from_dns_encoding fails due to an encoding error. > (getaddrinfo): Use DEPRECATED_AI_IDN. > * sysdeps/unix/inet/Subdirs (libidn): Remove. > * sysdeps/unix/inet/configure: Remove file. > * sysdeps/unix/inet/configure.ac: Likewise. > On Fedora 28, I got FAIL: resolv/tst-resolv-ai_idn FAIL: resolv/tst-resolv-ai_idn-latin1 [hjl@gnu-hsw-1 build-x86_64-linux]$ cat resolv/tst-resolv-ai_idn.out error: addrinfo comparison failure query: nämchen_zwo.example:80 AF_INET/0x40 error: 6 test failures [hjl@gnu-hsw-1 build-x86_64-linux]$
Comments
On 05/23/2018 06:44 PM, H.J. Lu wrote: > On Fedora 28, I got > > FAIL: resolv/tst-resolv-ai_idn > FAIL: resolv/tst-resolv-ai_idn-latin1 See the NEWS file. You need libidn 2.0.5 (and later) to avoid test failures. The mirror push for that is currently under way, and you should be able to install the new version tomorrow. Thanks, Florian
On 23/05/2018 13:55, Florian Weimer wrote: > On 05/23/2018 06:44 PM, H.J. Lu wrote: >> On Fedora 28, I got >> >> FAIL: resolv/tst-resolv-ai_idn >> FAIL: resolv/tst-resolv-ai_idn-latin1 > > See the NEWS file. You need libidn 2.0.5 (and later) to avoid test failures. The mirror push for that is currently under way, and you should be able to install the new version tomorrow. Shouldn't we in this case mark the tests as unsupported instead of failing?
On 05/23/2018 01:12 PM, Adhemerval Zanella wrote: > On 23/05/2018 13:55, Florian Weimer wrote: >> On 05/23/2018 06:44 PM, H.J. Lu wrote: >>> On Fedora 28, I got >>> >>> FAIL: resolv/tst-resolv-ai_idn FAIL: >>> resolv/tst-resolv-ai_idn-latin1 >> >> See the NEWS file. You need libidn 2.0.5 (and later) to avoid test >> failures. The mirror push for that is currently under way, and you >> should be able to install the new version tomorrow. > > Shouldn't we in this case mark the tests as unsupported instead of > failing? $0.02. No. It should fail to indicate you have bugs in your system libidn. Unsupported would be if you didn't have a libidn present at all (and the test looks for this too and returns FAIL_UNSUPPORTED).
On 23/05/2018 14:38, Carlos O'Donell wrote: > On 05/23/2018 01:12 PM, Adhemerval Zanella wrote: >> On 23/05/2018 13:55, Florian Weimer wrote: >>> On 05/23/2018 06:44 PM, H.J. Lu wrote: >>>> On Fedora 28, I got >>>> >>>> FAIL: resolv/tst-resolv-ai_idn FAIL: >>>> resolv/tst-resolv-ai_idn-latin1 >>> >>> See the NEWS file. You need libidn 2.0.5 (and later) to avoid test >>> failures. The mirror push for that is currently under way, and you >>> should be able to install the new version tomorrow. >> >> Shouldn't we in this case mark the tests as unsupported instead of >> failing? > > $0.02. No. It should fail to indicate you have bugs in your system libidn. > > Unsupported would be if you didn't have a libidn present at all (and the > test looks for this too and returns FAIL_UNSUPPORTED). > So in this case we should at least document this better, commit message does not have anything regarding it and NEWS update just states "[...] libidn2 version 2.0.5 or later is recommended. [...]" which does not really state why 2.0.5 is preferred. Also, reading the newer tests does give any indication why 2.0.5 is required to fully compliance nor they check libidn2 version. To one involved in glibc developments it should be straightforward to relate possible test-suite to related error, but I still think that indicating that system tests libidn is the culprit nor glibc itself is a better error output.
On 05/23/2018 01:51 PM, Adhemerval Zanella wrote: > > > On 23/05/2018 14:38, Carlos O'Donell wrote: >> On 05/23/2018 01:12 PM, Adhemerval Zanella wrote: >>> On 23/05/2018 13:55, Florian Weimer wrote: >>>> On 05/23/2018 06:44 PM, H.J. Lu wrote: >>>>> On Fedora 28, I got >>>>> >>>>> FAIL: resolv/tst-resolv-ai_idn FAIL: >>>>> resolv/tst-resolv-ai_idn-latin1 >>>> >>>> See the NEWS file. You need libidn 2.0.5 (and later) to avoid test >>>> failures. The mirror push for that is currently under way, and you >>>> should be able to install the new version tomorrow. >>> >>> Shouldn't we in this case mark the tests as unsupported instead of >>> failing? >> >> $0.02. No. It should fail to indicate you have bugs in your system libidn. >> >> Unsupported would be if you didn't have a libidn present at all (and the >> test looks for this too and returns FAIL_UNSUPPORTED). >> > > So in this case we should at least document this better, commit message does > not have anything regarding it and NEWS update just states "[...] libidn2 > version 2.0.5 or later is recommended. [...]" which does not really state > why 2.0.5 is preferred. > > Also, reading the newer tests does give any indication why 2.0.5 is required > to fully compliance nor they check libidn2 version. To one involved in glibc > developments it should be straightforward to relate possible test-suite to > related error, but I still think that indicating that system tests libidn is > the culprit nor glibc itself is a better error output. That sounds good to me. I'll let Florian respond here and add more NEWS information that people think should be present.
On 05/23/2018 07:51 PM, Adhemerval Zanella wrote: > So in this case we should at least document this better, commit message does > not have anything regarding it and NEWS update just states "[...] libidn2 > version 2.0.5 or later is recommended. [...]" which does not really state > why 2.0.5 is preferred. I thought that obvious: It has fewer bugs. This dragged on for so many months because we kept finding issues that needed addressing, and only with the 2.0.5 release, we have something that is close both to the WHATWG recommendations (which isn't what browsers implement) and what is needed on some enterprise networks. HJ tested with Fedora 28 libidn2 which has backports, so he reported only comparatively few test failures. > Also, reading the newer tests does give any indication why 2.0.5 is required > to fully compliance nor they check libidn2 version. To one involved in glibc > developments it should be straightforward to relate possible test-suite to > related error, but I still think that indicating that system tests libidn is > the culprit nor glibc itself is a better error output. There are comments in the test case which indicate what is being tested (non-transitional mode, Unicode TR 46 mode). These are areas where we pushed libidn2 to change defaults. Due to the backports from some distributions, mentioning specific libidn2 versions could be misleading. Immediately after the commit, I added a note <https://sourceware.org/glibc/wiki/Release/2.28#libidn2_is_a_recommended_dependency>. Isn't this sufficient? Thanks, Florian
On 23/05/2018 16:33, Florian Weimer wrote: > On 05/23/2018 07:51 PM, Adhemerval Zanella wrote: > >> So in this case we should at least document this better, commit message does >> not have anything regarding it and NEWS update just states "[...] libidn2 >> version 2.0.5 or later is recommended. [...]" which does not really state >> why 2.0.5 is preferred. > > I thought that obvious: It has fewer bugs. This dragged on for so many months because we kept finding issues that needed addressing, and only with the 2.0.5 release, we have something that is close both to the WHATWG recommendations (which isn't what browsers implement) and what is needed on some enterprise networks. > > HJ tested with Fedora 28 libidn2 which has backports, so he reported only comparatively few test failures. > >> Also, reading the newer tests does give any indication why 2.0.5 is required >> to fully compliance nor they check libidn2 version. To one involved in glibc >> developments it should be straightforward to relate possible test-suite to >> related error, but I still think that indicating that system tests libidn is >> the culprit nor glibc itself is a better error output. > > There are comments in the test case which indicate what is being tested (non-transitional mode, Unicode TR 46 mode). These are areas where we pushed libidn2 to change defaults. > > Due to the backports from some distributions, mentioning specific libidn2 versions could be misleading. > > Immediately after the commit, I added a note <https://sourceware.org/glibc/wiki/Release/2.28#libidn2_is_a_recommended_dependency>. Isn't this sufficient? > > Thanks, > Florian What was not immediate obvious to me (and H.J. Lu I would say) was which kind of issues to expect in a non conforming environment, neither that resolv/tst-resolv-ai_idn should fail in such scenarios. It it just for future references it would be good to have reference that it might be triggered by a faulty libidn2 installation (which this thread should be enough if someone cross this very issue).
On 23/05/2018 16:33, Florian Weimer wrote: > On 05/23/2018 07:51 PM, Adhemerval Zanella wrote: > >> So in this case we should at least document this better, commit message does >> not have anything regarding it and NEWS update just states "[...] libidn2 >> version 2.0.5 or later is recommended. [...]" which does not really state >> why 2.0.5 is preferred. > > I thought that obvious: It has fewer bugs. This dragged on for so many months because we kept finding issues that needed addressing, and only with the 2.0.5 release, we have something that is close both to the WHATWG recommendations (which isn't what browsers implement) and what is needed on some enterprise networks. > > HJ tested with Fedora 28 libidn2 which has backports, so he reported only comparatively few test failures. > >> Also, reading the newer tests does give any indication why 2.0.5 is required >> to fully compliance nor they check libidn2 version. To one involved in glibc >> developments it should be straightforward to relate possible test-suite to >> related error, but I still think that indicating that system tests libidn is >> the culprit nor glibc itself is a better error output. > > There are comments in the test case which indicate what is being tested (non-transitional mode, Unicode TR 46 mode). These are areas where we pushed libidn2 to change defaults. > > Due to the backports from some distributions, mentioning specific libidn2 versions could be misleading. > > Immediately after the commit, I added a note <https://sourceware.org/glibc/wiki/Release/2.28#libidn2_is_a_recommended_dependency>. Isn't this sufficient? > > Thanks, > Florian What was not immediate obvious to me (and H.J. Lu I would say) was which kind of issues to expect in a non conforming environment, neither that resolv/tst-resolv-ai_idn should fail in such scenarios. It it just for future references it would be good to have reference that it might be triggered by a faulty libidn2 installation (which this thread should be enough if someone cross this very issue).
--- expected +++ actual @@ -1,2 +1 @@ -flags: AI_IDN -address: STREAM/TCP 192.0.2.120 80 +error: Parameter string not correctly encoded error: addrinfo comparison failure query: nämchen_zwo.example:80 AF_INET/0x42 --- expected +++ actual @@ -1,3 +1 @@ -flags: AI_CANONNAME AI_IDN -canonname: xn--nmchen_zwo-q5a.example -address: STREAM/TCP 192.0.2.120 80 +error: Parameter string not correctly encoded error: addrinfo comparison failure query: nämchen_zwo.example:80 AF_INET/0xc2 --- expected +++ actual @@ -1,3 +1 @@ -flags: AI_CANONNAME AI_IDN AI_CANONIDN -canonname: nämchen_zwo.example -address: STREAM/TCP 192.0.2.120 80 +error: Parameter string not correctly encoded error: addrinfo comparison failure query: With.idn-cname.nämchen.example:80 AF_INET/0x40 --- expected +++ actual @@ -1,2 +1,2 @@ flags: AI_IDN -address: STREAM/TCP 192.0.2.119 80 +address: STREAM/TCP 192.0.2.87 80 error: addrinfo comparison failure query: With.idn-cname.nämchen.example:80 AF_INET/0x42 --- expected +++ actual @@ -1,3 +1,3 @@ flags: AI_CANONNAME AI_IDN canonname: xn--anderes-nmchen-eib.example -address: STREAM/TCP 192.0.2.119 80 +address: STREAM/TCP 192.0.2.87 80 error: addrinfo comparison failure query: With.idn-cname.nämchen.example:80 AF_INET/0xc2 --- expected +++ actual @@ -1,3 +1,3 @@ flags: AI_CANONNAME AI_IDN AI_CANONIDN canonname: anderes-nämchen.example -address: STREAM/TCP 192.0.2.119 80 +address: STREAM/TCP 192.0.2.87 80