Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify IDNA usage and switch to libidn2 when needed #97

Merged
merged 6 commits into from Oct 30, 2017

Conversation

nmav
Copy link
Contributor

@nmav nmav commented Aug 14, 2017

That is, use libc's AI_IDN consistently. In the case IDNA conversions are needed that cannot be delegated to libc, use libidn2 which implements the IDNA2008 standard (libidn implements the currently obsolete IDNA2003).

@nmav
Copy link
Contributor Author

nmav commented Aug 14, 2017

Note that WIP flag in the ping6 change. I'm not sure what is intention of this function so my change may not be entirely correct. I'd appreciate some review here.

Note also that my goal was to switch to IDNA2008 completely, however, since most programs used libc's IDN functions, I changed the rest to do the same (except ping6).

@okias
Copy link
Member

okias commented Aug 14, 2017

I'm on the phone, but I recall some security issues with libidn2, is everything OK?

Do you have some "tests" to run to verify correct behaviour?

Also, these days we're already able yo build iputils with meson.build, so please also include changes to build with meson :)

@nmav
Copy link
Contributor Author

nmav commented Aug 15, 2017

There were security issues with both libidn and libidn2, but at this point libidn2 is much better tested and fuzzed than any library I've worked with:
https://gitlab.com/libidn/libidn2

I could introduce some tests with internationalized names. Is there some testsuite I could add some checks at?

@okias
Copy link
Member

okias commented Aug 15, 2017

$ make
...
ping6_common.c: In function ‘niquery_option_subject_name_handler’:
ping6_common.c:460:7: warning: implicit declaration of function ‘idna_to_ascii_lz’ [-Wimplicit-function-declaration]
  rc = idna_to_ascii_lz(name, &idn, 0);
       ^
ping6_common.c:463:4: warning: implicit declaration of function ‘idna_strerror’ [-Wimplicit-function-declaration]
    idna_strerror(rc));
    ^
ping6_common.c:462:19: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=]
   fprintf(stderr, "ping6: IDN encoding error: %s\n",
                   ^
cc   -O3 -g -fno-strict-aliasing -Wstrict-prototypes -Wall ping.o ping_common.o ping6_common.o -lcap -lidn2 -lnettle -lresolv -lm   -o ping
ping6_common.o: In function `niquery_option_subject_name_handler':
/home/okias/projects/iputils/ping6_common.c:460: undefined reference to `idna_to_ascii_lz'
/home/okias/projects/iputils/ping6_common.c:462: undefined reference to `idna_strerror'

Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
The function was converting from locale to UTF-8, performing some
check and then converting to IDNA form. Convert instead directly
to IDNA from locale format and perform the check afterwards.

Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
@nmav nmav force-pushed the tmp-idn2 branch 3 times, most recently from 85f6718 to 0eaa8f8 Compare August 24, 2017 19:44
@nmav nmav changed the title Simplify IDNA usage and switch to libidn2 Simplify IDNA usage and switch to libidn2 when needed Aug 24, 2017
@nmav nmav force-pushed the tmp-idn2 branch 2 times, most recently from 4286718 to 1990d61 Compare August 24, 2017 20:12
@nmav
Copy link
Contributor Author

nmav commented Aug 24, 2017

@okias It seems that by using libidn2 compatibility functions we can compile it even in ubuntu trusty. The current version of the patch does that.

That is, to provide IDNA2008 support instead of IDNA2003.
See https://fedoraproject.org/wiki/Changes/IDNA2008
for more rationale.

That uses libidn2 idn2_lookup_ul() which is identical to
idn2_to_ascii_lz() but is available on all versions of
libidn2.

Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
This enables compilation with the functionality intended to
be tested.

Signed-off-by: Nikos Mavrogiannopoulos <nmav@gnutls.org>
@okias
Copy link
Member

okias commented Oct 8, 2017

Just checking - final version? Build seems to be passing, should I check it and merge?

@nmav
Copy link
Contributor Author

nmav commented Oct 9, 2017

Yes, that's my final version.

@okias okias merged commit 29fac00 into iputils:master Oct 30, 2017
@okias
Copy link
Member

okias commented Oct 30, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants