[PATCH] base64: refactor l_base64_decode and validate bytes consumed

[PATCH] base64: refactor l_base64_decode and validate bytes consumed

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

The way the base64 data was being validated was accurate, but included a
second loop to iterate the padding. There was nothing wrong with this but
for more concise/readable code the padding can be calculated and validated
all after the first loop by incrementing pad_len when '=' is found rather
than breaking out. The valid base64 character check did need a slight
modification to also check that pad_len has not been incremented, which
ensures there is no data after the padding (prior this was done by the
second, pad checking, loop).

Then, after the loop, pad_len has been calculated and can be checked against
what was expected based on the valid base64 characters found.

This refactoring also makes checking the bytes consumed much easier since
the first loop iterates until the very end of the base64 data (including
padding).
---
 ell/base64.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/ell/base64.c b/ell/base64.c
index e430470..340550f 100644
--- a/ell/base64.c
+++ b/ell/base64.c
@@ -33,7 +33,7 @@ LIB_EXPORT uint8_t *l_base64_decode(const char *in, size_t in_len,
 {
 	const char *ptr, *in_end = in + in_len;
 	uint8_t *out_buf, *out;
-	int base64_len = 0, pad_len;
+	int base64_len = 0, pad_len = 0;
 	uint16_t reg = 0;
 
 	for (ptr = in; ptr < in_end; ptr++)
@@ -42,32 +42,25 @@ LIB_EXPORT uint8_t *l_base64_decode(const char *in, size_t in_len,
 			continue;
 		else if (*ptr == '=')
 			/* Final padding */
-			break;
-		else if (l_ascii_isalnum(*ptr) || *ptr == '+' || *ptr == '/')
+			pad_len++;
+		else if (!pad_len && (l_ascii_isalnum(*ptr) || *ptr == '+' ||
+					*ptr == '/'))
 			/* Base64 character */
 			base64_len++;
 		else
 			/* Bad character */
 			return NULL;
 
-	in_end = ptr;
+	if (ptr != in_end)
+		return NULL;
+
+	in_end = ptr - pad_len;
 
 	if ((base64_len & 3) == 1)
 		/* Invalid length */
 		return NULL;
 
-	pad_len = (4 - base64_len) & 3;
-	for (; ptr < in + in_len && pad_len; ptr++)
-		if (l_ascii_isspace(*ptr))
-			/* Whitespace */
-			continue;
-		else if (*ptr == '=')
-			/* Final padding */
-			pad_len--;
-		else
-			/* Bad character */
-			return NULL;
-	if (pad_len)
+	if (pad_len != ((4 - base64_len) & 3))
 		return NULL;
 
 	*n_written = base64_len * 3 / 4;
-- 
2.34.1

Re: [PATCH] base64: refactor l_base64_decode and validate bytes consumed

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]

Hi James,

On 2/16/22 18:35, James Prestwood wrote:
> The way the base64 data was being validated was accurate, but included a
> second loop to iterate the padding. There was nothing wrong with this but
> for more concise/readable code the padding can be calculated and validated
> all after the first loop by incrementing pad_len when '=' is found rather
> than breaking out. The valid base64 character check did need a slight
> modification to also check that pad_len has not been incremented, which
> ensures there is no data after the padding (prior this was done by the
> second, pad checking, loop).
> 
> Then, after the loop, pad_len has been calculated and can be checked against
> what was expected based on the valid base64 characters found.
> 
> This refactoring also makes checking the bytes consumed much easier since
> the first loop iterates until the very end of the base64 data (including
> padding).
> ---
>   ell/base64.c | 25 +++++++++----------------
>   1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/ell/base64.c b/ell/base64.c
> index e430470..340550f 100644
> --- a/ell/base64.c
> +++ b/ell/base64.c
> @@ -33,7 +33,7 @@ LIB_EXPORT uint8_t *l_base64_decode(const char *in, size_t in_len,
>   {
>   	const char *ptr, *in_end = in + in_len;
>   	uint8_t *out_buf, *out;
> -	int base64_len = 0, pad_len;
> +	int base64_len = 0, pad_len = 0;
>   	uint16_t reg = 0;
>   
>   	for (ptr = in; ptr < in_end; ptr++)
> @@ -42,32 +42,25 @@ LIB_EXPORT uint8_t *l_base64_decode(const char *in, size_t in_len,
>   			continue;
>   		else if (*ptr == '=')
>   			/* Final padding */
> -			break;
> -		else if (l_ascii_isalnum(*ptr) || *ptr == '+' || *ptr == '/')
> +			pad_len++;
> +		else if (!pad_len && (l_ascii_isalnum(*ptr) || *ptr == '+' ||
> +					*ptr == '/'))
>   			/* Base64 character */
>   			base64_len++;
>   		else
>   			/* Bad character */
>   			return NULL;
>   
> -	in_end = ptr;
> +	if (ptr != in_end)
> +		return NULL;
> +
> +	in_end = ptr - pad_len;
>   
>   	if ((base64_len & 3) == 1)
>   		/* Invalid length */
>   		return NULL;
>   
> -	pad_len = (4 - base64_len) & 3;
> -	for (; ptr < in + in_len && pad_len; ptr++)
> -		if (l_ascii_isspace(*ptr))
> -			/* Whitespace */
> -			continue;
> -		else if (*ptr == '=')
> -			/* Final padding */
> -			pad_len--;
> -		else
> -			/* Bad character */
> -			return NULL;
> -	if (pad_len)
> +	if (pad_len != ((4 - base64_len) & 3))

This was in the old code, but it might be more understandable to write this as:

pad_len = align_len(base64_len, 4) - base64_len.

>   		return NULL;
>   
>   	*n_written = base64_len * 3 / 4;
> 

Looks good to me.  But test-pem is now failing:
PASS: unit/test-gvariant-message
./build-aux/test-driver: line 112: 26410 Aborted                 "$@" >> 
"$log_file" 2>&1
FAIL: unit/test-pem
PASS: unit/test-tls
PASS: unit/test-key

Regards,
-Denis