Re: [PATCH] Support IPv6 addresses in URLs

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--===============5954251747598975654==
Content-Language: en-US
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature";
 boundary="------------lUwn0pk60SXoziHA9omd3wi4"

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--------------lUwn0pk60SXoziHA9omd3wi4
Content-Type: multipart/mixed; boundary="------------juSf0jRH01qDtoioVegvfdmu";
 protected-headers="v1"; hp="clear"
Message-ID: <[email protected]>
Date: Thu, 23 Jul 2026 19:44:49 -0400
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Support IPv6 addresses in URLs
To: Damien Miller <[email protected]>
Cc: [email protected]
References: <[email protected]>
 <[email protected]>
Content-Language: en-US
From: Demi Marie Obenour <[email protected]>
Autocrypt: [email protected]; keydata=
 xsFNBFp+A0oBEADffj6anl9/BHhUSxGTICeVl2tob7hPDdhHNgPR4C8xlYt5q49yB+l2nipd
 aq+4Gk6FZfqC825TKl7eRpUjMriwle4r3R0ydSIGcy4M6eb0IcxmuPYfbWpr/si88QKgyGSV
 Z7GeNW1UnzTdhYHuFlk8dBSmB1fzhEYEk0RcJqg4AKoq6/3/UorR+FaSuVwT7rqzGrTlscnT
 DlPWgRzrQ3jssesI7sZLm82E3pJSgaUoCdCOlL7MMPCJwI8JpPlBedRpe9tfVyfu3euTPLPx
 wcV3L/cfWPGSL4PofBtB8NUU6QwYiQ9Hzx4xOyn67zW73/G0Q2vPPRst8LBDqlxLjbtx/WLR
 6h3nBc3eyuZ+q62HS1pJ5EvUT1vjyJ1ySrqtUXWQ4XlZyoEFUfpJxJoN0A9HCxmHGVckzTRl
 5FMWo8TCniHynNXsBtDQbabt7aNEOaAJdE7to0AH3T/Bvwzcp0ZJtBk0EM6YeMLtotUut7h2
 Bkg1b//r6bTBswMBXVJ5H44Qf0+eKeUg7whSC9qpYOzzrm7+0r9F5u3qF8ZTx55TJc2g656C
 9a1P1MYVysLvkLvS4H+crmxA/i08Tc1h+x9RRvqba4lSzZ6/Tmt60DPM5Sc4R0nSm9BBff0N
 m0bSNRS8InXdO1Aq3362QKX2NOwcL5YaStwODNyZUqF7izjK4QARAQABzTxEZW1pIE1hcmll
 IE9iZW5vdXIgKGxvdmVyIG9mIGNvZGluZykgPGRlbWlvYmVub3VyQGdtYWlsLmNvbT7CwXgE
 EwECACIFAlp+A0oCGwMGCwkIBwMCBhUIAgkKCwQWAgMBAh4BAheAAAoJELKItV//nCLBhr8Q
 AK/xrb4wyi71xII2hkFBpT59ObLN+32FQT7R3lbZRjVFjc6yMUjOb1H/hJVxx+yo5gsSj5LS
 9AwggioUSrcUKldfA/PKKai2mzTlUDxTcF3vKx6iMXKA6AqwAw4B57ZEJoMM6egm57TV19kz
 PMc879NV2nc6+elaKl+/kbVeD3qvBuEwsTe2Do3HAAdrfUG/j9erwIk6gha/Hp9yZlCnPTX+
 VK+xifQqt8RtMqS5R/S8z0msJMI/ajNU03kFjOpqrYziv6OZLJ5cuKb3bZU5aoaRQRDzkFIR
 6aqtFLTohTo20QywXwRa39uFaOT/0YMpNyel0kdOszFOykTEGI2u+kja35g9TkH90kkBTG+a
 EWttIht0Hy6YFmwjcAxisSakBuHnHuMSOiyRQLu43ej2+mDWgItLZ48Mu0C3IG1seeQDjEYP
 tqvyZ6bGkf2Vj+L6wLoLLIhRZxQOedqArIk/Sb2SzQYuxN44IDRt+3ZcDqsPppoKcxSyd1Ny
 2tpvjYJXlfKmOYLhTWs8nwlAlSHX/c/jz/ywwf7eSvGknToo1Y0VpRtoxMaKW1nvH0OeCSVJ
 itfRP7YbiRVc2aNqWPCSgtqHAuVraBRbAFLKh9d2rKFB3BmynTUpc1BQLJP8+D5oNyb8Ts4x
 Xd3iV/uD8JLGJfYZIR7oGWFLP4uZ3tkneDfYzsFNBFp+A0oBEAC9ynZI9LU+uJkMeEJeJyQ/
 8VFkCJQPQZEsIGzOTlPnwvVna0AS86n2Z+rK7R/usYs5iJCZ55/JISWd8xD57ue0eB47bcJv
 VqGlObI2DEG8TwaW0O0duRhDgzMEL4t1KdRAepIESBEA/iPpI4gfUbVEIEQuqdqQyO4GAe+M
 kD0Hy5JH/0qgFmbaSegNTdQg5iqYjRZ3ttiswalql1/iSyv1WYeC1OAs+2BLOAT2NEggSiVO
 txEfgewsQtCWi8H1SoirakIfo45Hz0tk/Ad9ZWh2PvOGt97Ka85o4TLJxgJJqGEnqcFUZnJJ
 riwoaRIS8N2C8/nEM53jb1sH0gYddMU3QxY7dYNLIUrRKQeNkF30dK7V6JRH7pleRlf+wQcN
 fRAIUrNlatj9TxwivQrKnC9aIFFHEy/0mAgtrQShcMRmMgVlRoOA5B8RTulRLCmkafvwuhs6
 dCxN0GNAORIVVFxjx9Vn7OqYPgwiofZ6SbEl0hgPyWBQvE85klFLZLoj7p+joDY1XNQztmfA
 rnJ9x+YV4igjWImINAZSlmEcYtd+xy3Li/8oeYDAqrsnrOjb+WvGhCykJk4urBog2LNtcyCj
 kTs7F+WeXGUo0NDhbd3Z6AyFfqeF7uJ3D5hlpX2nI9no/ugPrrTVoVZAgrrnNz0iZG2DVx46
 x913pVKHl5mlYQARAQABwsFfBBgBAgAJBQJafgNKAhsMAAoJELKItV//nCLBwNIP/AiIHE8b
 oIqReFQyaMzxq6lE4YZCZNj65B/nkDOvodSiwfwjjVVE2V3iEzxMHbgyTCGA67+Bo/d5aQGj
 gn0TPtsGzelyQHipaUzEyrsceUGWYoKXYyVWKEfyh0cDfnd9diAm3VeNqchtcMpoehETH8fr
 RHnJdBcjf112PzQSdKC6kqU0Q196c4Vp5HDOQfNiDnTf7gZSj0BraHOByy9LEDCLhQiCmr+2
 E0rW4tBtDAn2HkT9uf32ZGqJCn1O+2uVfFhGu6vPE5qkqrbSE8TG+03H8ecU2q50zgHWPdHM
 OBvy3EhzfAh2VmOSTcRK+tSUe/u3wdLRDPwv/DTzGI36Kgky9MsDC5gpIwNbOJP2G/q1wT1o
 Gkw4IXfWv2ufWiXqJ+k7HEi2N1sree7Dy9KBCqb+ca1vFhYPDJfhP75I/VnzHVssZ/rYZ9+5
 1yDoUABoNdJNSGUYl+Yh9Pw9pE3Kt4EFzUlFZWbE4xKL/NPno+z4J9aWemLLszcYz/u3XnbO
 vUSQHSrmfOzX3cV4yfmjM5lewgSstoxGyTx2M8enslgdXhPthZlDnTnOT+C+OTsh8+m5tos8
 HQjaPM01MKBiAqdPgksm1wu2DrrwUi6ChRVTUBcj6+/9IJ81H2P2gJk3Ls3AVIxIffLoY34E
 +MYSfkEjBz0E8CLOcAw7JIwAaeBT
In-Reply-To: <[email protected]>

--------------juSf0jRH01qDtoioVegvfdmu
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On 7/21/26 01:19, Damien Miller wrote:
> On Thu, 16 Jul 2026, Demi Marie Obenour wrote:
>=20
>> URLs didn't distinguish between IPv6 addresses and domain names, so a
>> bracketed IPv6 address was treated as a syntax error.  Fix this by
>> parsing them properly.  The parser also handles percent-encoded zone I=
Ds
>> and properly percent-decodes them.
>>
>> Full regression tests are included.
>=20
> Thank you very much for writing regress tests for this.

You're welcome.  I hope they passed on OpenBSD; I only ever ran them
in a Fedora VM I use for OpenSSH development (on Qubes OS).

> IMO the decode_ipv6 function was pretty long and hard to read.

I agree.

> Also, it called fatal() whereas the other URL parsing paths did not.

I tried to provide specific error messages, instead of a generic error
indicating that the URL was incorrect.  Calling fatal() was an easy
way to do that.

> Please take a look at this version:
>=20
>=20
> diff --git a/misc.c b/misc.c
> index 517fa7a97..1e3c93151 100644
> --- a/misc.c
> +++ b/misc.c
> @@ -59,6 +59,7 @@
>  #endif
> =20
>  #include "xmalloc.h"
> +#include "addr.h"
>  #include "misc.h"
>  #include "log.h"
>  #include "ssh.h"
> @@ -1081,6 +1082,69 @@ urldecode(const char *src)
>  	return ret;
>  }
> =20
> +/*
> + * Check the optional portion of an IPv6 URL following % (inclusive)
> + * returns 0 on valid, -1 on invalid.
> + */
> +int
> +check_ipv6url_percent(const char *percent)
> +{
> +	/*
> +	 * Only the fragment specifier is allowed to be %-encoded.
> +	 * Therefore, the first %-encoded octet must itself be %,
> +	 * which is encoded as %25.
> +	 */
> +	if (percent[1] !=3D '2' || percent[2] !=3D '5')
> +		return -1;

If you go with my suggestion below, this can be written as:

	if (*percent++ !=3D '2' || *percent++ !=3D '5')
		return -1;

Also, it might be nicer to tell users that % must be encoded as %25.

> +	/*
> +	 * RFC6874 says that non-unreserved characters MUST be percent-encode=
d.
> +	 * Validate that here.  urldecode() will check for bad characters aft=
er
> +	 * the '%', as well as for %00.
> +	 */
> +	for (; *percent !=3D '\0'; percent++) {
> +		if (isalnum((u_char)*percent) ||

Is isalnum() guaranteed to be independent of locale?  If not, is
OpenSSH always in the C locale (or C.UTF-8) at this point?

> +		    strchr("-._~%", *percent) !=3D NULL)
> +			continue;
> +		/* bad */
> +		return -1;

Should this give an error message stating *which* character was
forbidden, and/or at what position it is in?

> +	}
> +	return 0;
> +}

This function has a somewhat confusing precondition: it assumes
that percent is a NUL-terminated *nonempty* string.  Would it be
clearer to replace 1 and 2 with 0 and 1 above, and...

> +static char *
> +decode_ipv6(const char *address)
> +{
> +	struct xaddr n;
> +	char *output, *percent;
> +
> +	/* Validate fragments before decoding the entire address */
> +	if ((percent =3D strchr(address, '%')) !=3D NULL &&
> +	    check_ipv6url_percent(percent) !=3D 0)
> +		return NULL;

call check_ipv6url_percent(percent + 1) here?

> +	/* URL-decode the fragment portion. */
> +	if ((output =3D urldecode(address)) =3D=3D NULL)
> +		return NULL;
> +
> +	/*
> +	 * We can't used getaddrinfo() here as some validate the scope/zone
> +	 * ID relates to an existing local link. Chop off the zone and treat
> +	 * it as a raw address.
> +	 */
> +	if ((percent =3D strchr(output, '%')) !=3D NULL)
> +		*percent =3D '\0';
> +
> +	if (addr_pton(output, &n) =3D=3D -1 || n.af !=3D AF_INET6)
> +		return NULL;

Should this give a non-generic error message?

> +	/* Restore the zone identifier if present. */
> +	if (percent !=3D NULL)
> +		*percent =3D '%';
> +
> +	return output;
> +}
> +
>  /*
>   * Parse an (scp|ssh|sftp)://[user@]host[:port][/path] URI.
>   * See https://tools.ietf.org/html/draft-ietf-secsh-scp-sftp-ssh-uri-0=
4
> @@ -1097,7 +1161,7 @@ int
>  parse_uri(const char *scheme, const char *uri, char **userp, char **ho=
stp,
>      int *portp, char **pathp)
>  {
> -	char *uridup, *cp, *tmp, ch;
> +	char *uridup, *cp, *tmp_host, *tmp, ch;
>  	char *user =3D NULL, *host =3D NULL, *path =3D NULL;
>  	int port =3D -1, ret =3D -1;
>  	size_t len;
> @@ -1140,9 +1204,16 @@ parse_uri(const char *scheme, const char *uri, c=
har **userp, char **hostp,
>  	/* Extract mandatory hostname */
>  	if ((cp =3D hpdelim2(&tmp, &ch)) =3D=3D NULL || *cp =3D=3D '\0')
>  		goto out;
> -	host =3D xstrdup(cleanhostname(cp));
> -	if (!valid_domain(host, 0, NULL))
> -		goto out;
> +	tmp_host =3D cleanhostname(cp);
> +	if (strchr(tmp_host, ':') !=3D NULL) {
> +		/* Validate the IPv6 address and URL-decode the zone ID. */
> +		if ((host =3D decode_ipv6(tmp_host)) =3D=3D NULL)
> +			goto out;
> +	} else {
> +		if (!valid_domain(tmp_host, 0, NULL))
> +			goto out;
> +		host =3D xstrdup(tmp_host);
> +	}
> =20
>  	if (tmp !=3D NULL && *tmp !=3D '\0') {
>  		if (ch =3D=3D ':') {

Looks correct to me.  As mentioned above, I'd prefer to have
check_ipv6url_percent() take a possibly-empty NUL-terminated string,
and more specific error messages might be better for users.  That said,
I don't mind this patch being applied as-is either (though please do
give me credit in the commit message).
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)

--------------juSf0jRH01qDtoioVegvfdmu--

--------------lUwn0pk60SXoziHA9omd3wi4
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEopQtqVJW1aeuo9/sszaHOrMp8lMFAmpip3MACgkQszaHOrMp
8lMxaRAApJ9OcPCUgqKM1eDMbxTTBiTKBcLIVc7SB4Jo55bclBrEms+DhSL3cXK1
I9B9ntawc8nLlPPLzWdZfiWvFWb8aphmjdq/l0rUu7OdHnuMZLe4sJdR0TX8wxrI
atNYJsKbiBo1b6iaDKuhPYwmkf+g/6pM18XapaMjAgsjlXcI7WN7Pd446bKDXLKK
EUbASqeO6Yix0bHdxkEOCIeq8J3p8fMhDPzqWlJCpgti4ARTvDh8Ecmxbl1rPSPY
QcSsqrWLmNUSq+HKuLJ6s2S2Xa3ZtJRojkzbkQFqwoyvq2kyMrCnE8NVaMP0i9cR
SD5U1lZIECvdbYsAy4tJF8PeTvPFLZR994chUaYi0fY2x557YtH9LOyTnViyyEEJ
zpIojuEDA48OdONUEHT+l8YJYOZgpVfAhEXzebVCwD39ddTPRajs7J0nkYcF5Y4a
Ck/RMpRqHfiP8WBpM+AFKZ/B55+h8aLHR8atMcNAuUPefQvmg0LoBSCjUDPcxnXh
425trgtwaZe9GMYOQ5b05nT9NSixaEj7vaz2YCCDcBt+n978DTfDY/AxA02pho//
pJzXVBALOZS1wAPCWg6quk/VYp5pc0SDZ9sF+KOdnYNfQUbvs/GeDfaawFJuWqvh
SQer6eqrahvzfFGVEaglg/NP/eHgSl0LeMAvsu0aQVbZLz2WKmI=
=ZCbP
-----END PGP SIGNATURE-----

--------------lUwn0pk60SXoziHA9omd3wi4--

--===============5954251747598975654==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
openssh-unix-dev mailing list
[email protected]
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

--===============5954251747598975654==--