[PATCH] Fix urlencode format string on signed char.

[PATCH] Fix urlencode format string on signed char.

[Julien Dusser]

Git credential fails with special char in password.
remote: Invalid username or password.
fatal: Authentication failed for

File ~/.git-credential contains badly urlencoded characters
%ffffffXX%ffffffYY instead of %XX%YY.

Add a cast to an unsigned char to fix urlencode use of %02x
on a char.

Signed-off-by: Julien Dusser <julien.dusser@free.fr>
---
 strbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb..4d5a9ce55 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
 		    (!reserved && is_rfc3986_reserved(ch)))
 			strbuf_addch(sb, ch);
 		else
-			strbuf_addf(sb, "%%%02x", ch);
+			strbuf_addf(sb, "%%%02x", (unsigned char)ch);
 	}
 }
 
-- 
2.15.1

Re: [PATCH] Fix urlencode format string on signed char.

[Junio C Hamano]

Julien Dusser <julien.dusser@free.fr> writes:

> Git credential fails with special char in password.
> remote: Invalid username or password.
> fatal: Authentication failed for
>
> File ~/.git-credential contains badly urlencoded characters
> %ffffffXX%ffffffYY instead of %XX%YY.
>
> Add a cast to an unsigned char to fix urlencode use of %02x
> on a char.
>
> Signed-off-by: Julien Dusser <julien.dusser@free.fr>
> ---
>  strbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index 323c49ceb..4d5a9ce55 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
>  		    (!reserved && is_rfc3986_reserved(ch)))
>  			strbuf_addch(sb, ch);
>  		else
> -			strbuf_addf(sb, "%%%02x", ch);
> +			strbuf_addf(sb, "%%%02x", (unsigned char)ch);
>  	}
>  }

The issue is not limited to credential but anywhere where we need to
show a byte with hi-bit set, and it is obvious and straight-forward.

I briefly wondered if the data type for the strings involved in the
codepaths that reach this place should all be "uchar*" but it feels
strange to have "unsigned char *username" etc., and the signeness
matters only here, so the patch smells like the best one among other
possibilities.

Thanks.

Re: [PATCH] Fix urlencode format string on signed char.

[Julien Dusser]

Thank you for review. I didn't find any other error.
Code in http.c:quote_ref_url() is almost the same but ch is a signed 
int, so there's no issue.

Le 22/12/2017 à 22:48, Junio C Hamano a écrit :
> Julien Dusser <julien.dusser@free.fr> writes:
> 
>> Git credential fails with special char in password.
>> remote: Invalid username or password.
>> fatal: Authentication failed for
>>
>> File ~/.git-credential contains badly urlencoded characters
>> %ffffffXX%ffffffYY instead of %XX%YY.
>>
>> Add a cast to an unsigned char to fix urlencode use of %02x
>> on a char.
>>
>> Signed-off-by: Julien Dusser <julien.dusser@free.fr>
>> ---
>>   strbuf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index 323c49ceb..4d5a9ce55 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,
>>   		    (!reserved && is_rfc3986_reserved(ch)))
>>   			strbuf_addch(sb, ch);
>>   		else
>> -			strbuf_addf(sb, "%%%02x", ch);
>> +			strbuf_addf(sb, "%%%02x", (unsigned char)ch);
>>   	}
>>   }
> 
> The issue is not limited to credential but anywhere where we need to
> show a byte with hi-bit set, and it is obvious and straight-forward.
> 
> I briefly wondered if the data type for the strings involved in the
> codepaths that reach this place should all be "uchar*" but it feels
> strange to have "unsigned char *username" etc., and the signeness
> matters only here, so the patch smells like the best one among other
> possibilities.
> 
> Thanks.
> 

Re: [PATCH] Fix urlencode format string on signed char.

[Johannes Schindelin]

Hi Julien,

On Sat, 23 Dec 2017, Julien Dusser wrote:

> Thank you for review. I didn't find any other error.
> Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so
> there's no issue.

But that ch comes from a signed char *, so it actually *is* an issue: if
you cast a signed char of value -1 to an int, it will still be -1. So
we'll also need:

-- snipsnap --
diff --git a/http.c b/http.c
index 117ddae..ed8221f 100644
--- a/http.c
+++ b/http.c
@@ -1347,7 +1347,7 @@ void finish_all_active_slots(void)
 }
 
 /* Helpers for modifying and creating URLs */
-static inline int needs_quote(int ch)
+static inline int needs_quote(unsigned char ch)
 {
 	if (((ch >= 'A') && (ch <= 'Z'))
 			|| ((ch >= 'a') && (ch <= 'z'))
@@ -1363,11 +1363,11 @@ static char *quote_ref_url(const char *base, const char *ref)
 {
 	struct strbuf buf = STRBUF_INIT;
 	const char *cp;
-	int ch;
+	unsigned char ch;
 
 	end_url_with_slash(&buf, base);
 
-	for (cp = ref; (ch = *cp) != 0; cp++)
+	for (cp = ref; (ch = (unsigned char)*cp); cp++)
 		if (needs_quote(ch))
 			strbuf_addf(&buf, "%%%02x", ch);
 		else