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

Fix CIDR notation queries with libidn2 #53

Closed
wants to merge 2 commits into from
Closed

Fix CIDR notation queries with libidn2 #53

wants to merge 2 commits into from

Conversation

andreasstieger
Copy link
Contributor

Fixes #50
Fixes https://bugzilla.opensuse.org/show_bug.cgi?id=1026831

Tested with without IDN, libidn and libidn2. Does the right thing on the wire. Please check.

@thiagomacieira
Copy link

thiagomacieira commented Apr 4, 2017

How about IPv6? Please test "whois 2001:67c:2178:8::13"

See https://bugzilla.opensuse.org/show_bug.cgi?id=1026831#c5

Sorry, I disagree it's a libidn2 bug. It's a whois bug: it should not send IP addresses (v4 or v6, in CIDR notation or not) through libidn2. IDN is for hostnames, which IP addresses aren't.

@andreasstieger
Copy link
Contributor Author

The last push fixes IPv6 and CIDR notation queries with libidn2. Please check.

@ppisar
Copy link
Contributor

ppisar commented Apr 26, 2017

I tested IPv4 and IPv6 adresses and CIDRs and some ASCII and IDN domain names. All of them works for me with HAVE_LIBDN2=1 HAVE_ICONV=1.

@andreasstieger
Copy link
Contributor Author

Did you get a chance to review the proposed fix for libidn2? @rockdaboot maybe?
@ppisar verified it as well and I would like to close this off, or improve if required.

@rockdaboot
Copy link

rockdaboot commented May 23, 2017

Wasn't aware of this patch, sorry. Just take a look right now.

If ipv6 and CIDRs are the only non-domain input to normalize_domain(), the code is correct.

There is just one thing I would like to mention:
We had a recent bug report from curl that a) was a bug and b) clarified the expectations of users.
As a consequence, you should use IDN2_TRANSITIONAL plus libidn v2.0.2 which fixes a bug in the transitional code. This bug leads to well-used IDNA 2003 domains not being accepted.

The code that curl and wget currently use is like

	if ((rc = idn2_lookup_u8((uint8_t *)src, (uint8_t **)&asc, IDN2_NONTRANSITIONAL)) != IDN2_OK)
		rc = idn2_lookup_u8((uint8_t *)src, (uint8_t **)&asc, IDN2_TRANSITIONAL);
	if (rc == IDN2_OK)

That is "first try pure IDNA 2008, if that fails fall back to IDNA 2008 transitional, which is very close to IDNA 2003".

Use the idn2_lookup_ul() function in your case (locale encoded input string).

@andreasstieger
Copy link
Contributor Author

Thank you for the hint. Commit 1e6a7b3 additionally tries IDNA 2008 transitional and passes all test cases here.

@andreasstieger
Copy link
Contributor Author

This patch has been in use by openSUSE Tumbleweed rolling release users for a month without bug reports. Will you merge it?

@rfc1036
Copy link
Owner

rfc1036 commented Jul 27, 2017

Thank you, I will have a new look at this before the next release.

@rockdaboot
Copy link

@rfc1036 libidn 2.0.3 (latest release) keeps the 'forbidden' chars to be compatible to libidn. This is STD3 ASCII rules are 'off' by default now. So if you check for that version, you maybe don't have to do much.

@rfc1036
Copy link
Owner

rfc1036 commented Aug 16, 2017

I do not like much calling idn2_lookup_ul() twice: do I still need this if I mandate using >= 2.0.3 which disables IDN2_USE_STD3_ASCII_RULES?

@rfc1036
Copy link
Owner

rfc1036 commented Aug 16, 2017

Or even checking for / and : at all?

@rockdaboot
Copy link

With >= 2.0.3 the / and : issue is gone. The double calling of idn2_lookup_ul() is a kind of 'maximum backward compatibility to IDNA 2003'. If you don't want/need that kind of compat, remove the second call. Then you have straight IDNA 2008 with TR46 preprocessing + disabled STD3 rules.

@andreasstieger
Copy link
Contributor Author

So indeed with libidn2 2.0.3 the problems are gone:

make clean; make all HAVE_LIBIDN2=1
./whois -h whois.ripe.net 193.0.0.0/21
./whois 2001:67c:2178:8::1

So neither d6e33ff nor 1e6a7b3 are required to fix CIDR notation queries, and the fix is in libidn2. So I think this can be closed.

@ppisar
Copy link
Contributor

ppisar commented Aug 18, 2017 via email

@andreasstieger
Copy link
Contributor Author

The debian package does not build with libidn2.

@rfc1036
Copy link
Owner

rfc1036 commented Aug 22, 2017

Thank you for your help, now I have spent some time learning about IDNA2003, IDNA2008 and TR46. Since the current trend is to adopt IDNA2008 non-transitional I think that the current code is right.
I will switch the Debian package to libidn2 when 2.0.3 will be packaged.

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.

5 participants