From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: percent-encode the quote character too
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Thu, 5 Jan 2023 07:42:17 -0700

On Thu, Jan 05, 2023 at 01:22:14PM +0100, Omar Polo wrote:
> noticed a line in my gotwebd logs:
> 
> : gotweb_process_request: 3a15e1807a369c0a7827363eca22c9f1a8598d9c>diff</a> | <a href=: bad object id string
> 
> No idea where it comes from, grepping the codebase we've not forgot
> any quotes around the `href' attribute, but this made me recheck
> various pages and at least I've found an issue, albeit not sure it's
> the one triggering that failure.
> 
> The issue is that gotweb_render_url doesn't percent-encode the "
> character, that can thus end up in the middle of the URL breaking the
> quoting.  This can also lead to security issues if the browser doesn't
> honour the Content-Security-Policy.
> 
> Note that the ' character was already quoted as mandated by RFC3986.
> 
> gotwebd from the latest release should be fine because it doesn't use
> the templates yet.  Previously all attribute were quoted with ', so a
> raw " in the middle didn't it, but with the move to the templates I've
> started to use only " to quote the value, unknwingly introducing this
> issue.
> 
> ok?
> 

Funny how those little things cause breakage. ok

> 
> commit 9087ededfe8472a4a54c7e8bbb97250595e8b06d (main)
> from: Omar Polo <op@omarpolo.com>
> date: Thu Jan  5 11:56:20 2023 UTC
>  
>  gotwebd: urlencode also the " character
>  
>  URLs are embedded as part of the HTML and, while it seems legal from
>  RFC3986 to leave that character unquoted, we need it quoted to avoid
>  breaking the HTML output.
>  
> diff 4cc7b95a1a3139b5fdf12363376cd6ef877bf3ea 9087ededfe8472a4a54c7e8bbb97250595e8b06d
> commit - 4cc7b95a1a3139b5fdf12363376cd6ef877bf3ea
> commit + 9087ededfe8472a4a54c7e8bbb97250595e8b06d
> blob - 08f7194204d5247935adcee192184a531f881ede
> blob + a8fdd952ec48f1a229440e115de67205cf20f049
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -1561,6 +1561,8 @@ should_urlencode(int c)
>  	case ',':
>  	case ';':
>  	case '=':
> +		/* needed because the URLs are embedded into the HTML */
> +	case '\"':
>  		return 1;
>  	default:
>  		return 0;
> 

-- 

Tracey Emery