[PATCH] Support IPv6 addresses in URLs

[Demi Marie Obenour]

On 7/21/26 01:19, Damien Miller wrote:
> On Thu, 16 Jul 2026, Demi Marie Obenour wrote:
> 
>> 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 IDs
>> and properly percent-decodes them.
>>
>> Full regression tests are included.
> 
> 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:
> 
> 
> diff --git a/misc.c b/misc.c
> index 517fa7a97..1e3c93151 100644
> --- a/misc.c
> +++ b/misc.c
> @@ -59,6 +59,7 @@
>  #endif
>  
>  #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;
>  }
>  
> +/*
> + * 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] != '2' || percent[2] != '5')
> +		return -1;

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

	if (*percent++ != '2' || *percent++ != '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-encoded.
> +	 * Validate that here.  urldecode() will check for bad characters after
> +	 * the '%', as well as for %00.
> +	 */
> +	for (; *percent != '\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) != 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 = strchr(address, '%')) != NULL &&
> +	    check_ipv6url_percent(percent) != 0)
> +		return NULL;

call check_ipv6url_percent(percent + 1) here?

> +	/* URL-decode the fragment portion. */
> +	if ((output = urldecode(address)) == 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 = strchr(output, '%')) != NULL)
> +		*percent = '\0';
> +
> +	if (addr_pton(output, &n) == -1 || n.af != AF_INET6)
> +		return NULL;

Should this give a non-generic error message?

> +	/* Restore the zone identifier if present. */
> +	if (percent != NULL)
> +		*percent = '%';
> +
> +	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-04
> @@ -1097,7 +1161,7 @@ int
>  parse_uri(const char *scheme, const char *uri, char **userp, char **hostp,
>      int *portp, char **pathp)
>  {
> -	char *uridup, *cp, *tmp, ch;
> +	char *uridup, *cp, *tmp_host, *tmp, ch;
>  	char *user = NULL, *host = NULL, *path = NULL;
>  	int port = -1, ret = -1;
>  	size_t len;
> @@ -1140,9 +1204,16 @@ parse_uri(const char *scheme, const char *uri, char **userp, char **hostp,
>  	/* Extract mandatory hostname */
>  	if ((cp = hpdelim2(&tmp, &ch)) == NULL || *cp == '\0')
>  		goto out;
> -	host = xstrdup(cleanhostname(cp));
> -	if (!valid_domain(host, 0, NULL))
> -		goto out;
> +	tmp_host = cleanhostname(cp);
> +	if (strchr(tmp_host, ':') != NULL) {
> +		/* Validate the IPv6 address and URL-decode the zone ID. */
> +		if ((host = decode_ipv6(tmp_host)) == NULL)
> +			goto out;
> +	} else {
> +		if (!valid_domain(tmp_host, 0, NULL))
> +			goto out;
> +		host = xstrdup(tmp_host);
> +	}
>  
>  	if (tmp != NULL && *tmp != '\0') {
>  		if (ch == ':') {

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).
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20260723/3414087c/attachment-0001.asc>