Re: [PATCH] Support IPv6 addresses in URLs

Demi Marie Obenour <[email protected]> Wed, 19 Aug 2026 00:12:17 -0400
Newsgroups	gmane.network.openssh.devel
Message-ID	<[email protected]>
On 7/23/26 19:44, Demi Marie Obenour wrote:
> 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).

Damien, would you mind looking at this again?  I'm fine with the
version you proposed, either as-is or with the suggestions I made
above.  I hope not having an OpenBSD box to test this is okay.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

_______________________________________________
openssh-unix-dev mailing list
[email protected]
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
OpenPGP_signature.asc (application/pgp-signature, 833 B)