[Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Sergey Nikiforov]

Not all invalid characters were ignored by base64 decoder
causing data corruption and reads beyond decode table
(faults under ASAN).

Added corresponding check into base64 unit test.

Fixes: #5627
---

Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing
Issue: https://github.com/tarantool/tarantool/issues/5627

 test/unit/base64.c      | 23 ++++++++++++++++++++++-
 test/unit/base64.result |  5 ++++-
 third_party/base64.c    |  3 ++-
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/test/unit/base64.c b/test/unit/base64.c
index ada497adf..c0f53a5e1 100644
--- a/test/unit/base64.c
+++ b/test/unit/base64.c
@@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
 	base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
 }
 
+static void
+base64_invalid_chars_test(void)
+{
+	/* Upper bit must be cleared */
+	const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
+	char outbuf[8];
+
+	plan(1);
+
+	/* Invalid chars should be ignored, not decoded into garbage */
+	is(base64_decode(invalid_data, sizeof(invalid_data),
+	                 outbuf, sizeof(outbuf)),
+	   0, "ignoring invalid chars");
+
+	check_plan();
+}
+
 int main(int argc, char *argv[])
 {
-	plan(28);
+	plan(28
+	     + 1 /* invalid chars test */
+	     );
 	header();
 
 	const char *option_tests[] = {
@@ -78,6 +97,8 @@ int main(int argc, char *argv[])
 		base64_nowrap_test(option_tests[i]);
 	}
 
+	base64_invalid_chars_test();
+
 	footer();
 	return check_plan();
 }
diff --git a/test/unit/base64.result b/test/unit/base64.result
index cd1f2b3f6..3bc2c2275 100644
--- a/test/unit/base64.result
+++ b/test/unit/base64.result
@@ -1,4 +1,4 @@
-1..28
+1..29
 	*** main ***
     1..3
     ok 1 - length
@@ -175,4 +175,7 @@ ok 27 - subtests
     ok 3 - decode length ok
     ok 4 - encode/decode
 ok 28 - subtests
+    1..1
+    ok 1 - ignoring invalid chars
+ok 29 - subtests
 	*** main: done ***
diff --git a/third_party/base64.c b/third_party/base64.c
index 8ecab23eb..7c69315ea 100644
--- a/third_party/base64.c
+++ b/third_party/base64.c
@@ -222,7 +222,8 @@ base64_decode_value(int value)
 		32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
 		44, 45, 46, 47, 48, 49, 50, 51
 	};
-	static const int decoding_size = sizeof(decoding);
+	static const int decoding_size =
+		sizeof(decoding) / sizeof(decoding[0]);
 	int codepos = value;
 	codepos -= 43;
 	if (codepos < 0 || codepos >= decoding_size)
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..c0f53a5e1 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>  	base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>  }
>  
> +static void
> +base64_invalid_chars_test(void)
> +{
> +	/* Upper bit must be cleared */
> +	const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
> +	char outbuf[8];
> +
> +	plan(1);
> +
> +	/* Invalid chars should be ignored, not decoded into garbage */
> +	is(base64_decode(invalid_data, sizeof(invalid_data),
> +	                 outbuf, sizeof(outbuf)),
> +	   0, "ignoring invalid chars");
> +
> +	check_plan();
> +}
> +
>  int main(int argc, char *argv[])
>  {
> -	plan(28);
> +	plan(28
> +	     + 1 /* invalid chars test */
> +	     );

Why not just 29? It looks kind of weird now.

The patch looks good. Get a second review from somebody, and I
will commit this.

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Leonid Vasiliev]

Hi! Thank you for the patch.
Generally LGTM.
See some comments below:

According to 
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
"Description is what the patch does, started from lowercase letter,
without a dot in the end, in the imperative mood."
("properly...").
I could be wrong, but it seems like <description> is not written in an
imperative mood.

On 15.12.2020 17:25, Sergey Nikiforov wrote:
> Not all invalid characters were ignored by base64 decoder
> causing data corruption and reads beyond decode table
> (faults under ASAN).
> 
> Added corresponding check into base64 unit test.
> 
> Fixes: #5627
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing
> Issue: https://github.com/tarantool/tarantool/issues/5627
> 
>   test/unit/base64.c      | 23 ++++++++++++++++++++++-
>   test/unit/base64.result |  5 ++++-
>   third_party/base64.c    |  3 ++-
>   3 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..c0f53a5e1 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>   	base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>   }
>   
> +static void
> +base64_invalid_chars_test(void)
> +{
> +	/* Upper bit must be cleared */
> +	const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
> +	char outbuf[8];
> +
> +	plan(1);

Usually `plan ()` is called as the first call in a function. It's just
easier to see how many checks there will be. I don't know any rule about
this. So, it's up to you.

> +
> +	/* Invalid chars should be ignored, not decoded into garbage */
> +	is(base64_decode(invalid_data, sizeof(invalid_data),
> +	                 outbuf, sizeof(outbuf)),
> +	   0, "ignoring invalid chars");
> +
> +	check_plan();
> +}
> +
>   int main(int argc, char *argv[])
>   {
> -	plan(28);
> +	plan(28
> +	     + 1 /* invalid chars test */
> +	     );

I agree with Vlad. Why `+ 1` and not just 29?

>   	header();
>   
>   	const char *option_tests[] = {
> @@ -78,6 +97,8 @@ int main(int argc, char *argv[])
>   		base64_nowrap_test(option_tests[i]);
>   	}
>   
> +	base64_invalid_chars_test();
> +
>   	footer();
>   	return check_plan();
>   }
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..3bc2c2275 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,4 +1,4 @@
> -1..28
> +1..29
>   	*** main ***
>       1..3
>       ok 1 - length
> @@ -175,4 +175,7 @@ ok 27 - subtests
>       ok 3 - decode length ok
>       ok 4 - encode/decode
>   ok 28 - subtests
> +    1..1
> +    ok 1 - ignoring invalid chars
> +ok 29 - subtests
>   	*** main: done ***
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 8ecab23eb..7c69315ea 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -222,7 +222,8 @@ base64_decode_value(int value)
>   		32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
>   		44, 45, 46, 47, 48, 49, 50, 51
>   	};
> -	static const int decoding_size = sizeof(decoding);
> +	static const int decoding_size =
> +		sizeof(decoding) / sizeof(decoding[0]);
>   	int codepos = value;
>   	codepos -= 43;
>   	if (codepos < 0 || codepos >= decoding_size)
> 

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Alexander Turenko]

On Thu, Dec 17, 2020 at 12:41:48PM +0300, Leonid Vasiliev via Tarantool-patches wrote:
> Hi! Thank you for the patch.
> Generally LGTM.
> See some comments below:
> 
> According to https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
> "Description is what the patch does, started from lowercase letter,
> without a dot in the end, in the imperative mood."
> ("properly...").
> I could be wrong, but it seems like <description> is not written in an
> imperative mood.

A commit subject should be in the imperative mood. But there is more
freedom for a commit message body.

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Sergey Nikiforov]

Hi!

On 17.12.2020 12:41, Leonid Vasiliev wrote:
> Hi! Thank you for the patch.
> Generally LGTM.
> See some comments below:
> 
> According to 
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message 
> 
> "Description is what the patch does, started from lowercase letter,
> without a dot in the end, in the imperative mood."
> ("properly...").
> I could be wrong, but it seems like <description> is not written in an
> imperative mood.

Mmm. I am not that great in English, but how "Properly ignore..." is not 
"imperative mood"? What would you suggest?
I should, however, use lowercase (alas, force of habit).

> On 15.12.2020 17:25, Sergey Nikiforov wrote:
>> Not all invalid characters were ignored by base64 decoder
>> causing data corruption and reads beyond decode table
>> (faults under ASAN).
>>
>> Added corresponding check into base64 unit test.
>>
>> Fixes: #5627
>> ---
>>
>> Branch: 
>> https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing 
>>
>> Issue: https://github.com/tarantool/tarantool/issues/5627
>>
>>   test/unit/base64.c      | 23 ++++++++++++++++++++++-
>>   test/unit/base64.result |  5 ++++-
>>   third_party/base64.c    |  3 ++-
>>   3 files changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/test/unit/base64.c b/test/unit/base64.c
>> index ada497adf..c0f53a5e1 100644
>> --- a/test/unit/base64.c
>> +++ b/test/unit/base64.c
>> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>>       base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>>   }
>> +static void
>> +base64_invalid_chars_test(void)
>> +{
>> +    /* Upper bit must be cleared */
>> +    const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
>> +    char outbuf[8];
>> +
>> +    plan(1);
> 
> Usually `plan ()` is called as the first call in a function. It's just
> easier to see how many checks there will be. I don't know any rule about
> this. So, it's up to you.

I would move it if you like (if there would be another patch revision). 
I have just tried to be C89-friendly. Force of habit (useful one).

>> +
>> +    /* Invalid chars should be ignored, not decoded into garbage */
>> +    is(base64_decode(invalid_data, sizeof(invalid_data),
>> +                     outbuf, sizeof(outbuf)),
>> +       0, "ignoring invalid chars");
>> +
>> +    check_plan();
>> +}
>> +
>>   int main(int argc, char *argv[])
>>   {
>> -    plan(28);
>> +    plan(28
>> +         + 1 /* invalid chars test */
>> +         );
> 
> I agree with Vlad. Why `+ 1` and not just 29?

Using "magic" values without explanation is a bad idea for readability. 
Should I "decode" how 28 was calculated as well (using constants where 
appropriate) or no one bothers so much with tests?

>>       header();
>>       const char *option_tests[] = {
>> @@ -78,6 +97,8 @@ int main(int argc, char *argv[])
>>           base64_nowrap_test(option_tests[i]);
>>       }
>> +    base64_invalid_chars_test();
>> +
>>       footer();
>>       return check_plan();
>>   }
>> diff --git a/test/unit/base64.result b/test/unit/base64.result
>> index cd1f2b3f6..3bc2c2275 100644
>> --- a/test/unit/base64.result
>> +++ b/test/unit/base64.result
>> @@ -1,4 +1,4 @@
>> -1..28
>> +1..29
>>       *** main ***
>>       1..3
>>       ok 1 - length
>> @@ -175,4 +175,7 @@ ok 27 - subtests
>>       ok 3 - decode length ok
>>       ok 4 - encode/decode
>>   ok 28 - subtests
>> +    1..1
>> +    ok 1 - ignoring invalid chars
>> +ok 29 - subtests
>>       *** main: done ***
>> diff --git a/third_party/base64.c b/third_party/base64.c
>> index 8ecab23eb..7c69315ea 100644
>> --- a/third_party/base64.c
>> +++ b/third_party/base64.c
>> @@ -222,7 +222,8 @@ base64_decode_value(int value)
>>           32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
>>           44, 45, 46, 47, 48, 49, 50, 51
>>       };
>> -    static const int decoding_size = sizeof(decoding);
>> +    static const int decoding_size =
>> +        sizeof(decoding) / sizeof(decoding[0]);
>>       int codepos = value;
>>       codepos -= 43;
>>       if (codepos < 0 || codepos >= decoding_size)
>>

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Leonid Vasiliev]

Hi!

On 17.12.2020 16:04, Sergey Nikiforov wrote:
> Hi!
> 
> On 17.12.2020 12:41, Leonid Vasiliev wrote:
>> Hi! Thank you for the patch.
>> Generally LGTM.
>> See some comments below:
>>
>> According to 
>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message 
>>
>> "Description is what the patch does, started from lowercase letter,
>> without a dot in the end, in the imperative mood."
>> ("properly...").
>> I could be wrong, but it seems like <description> is not written in an
>> imperative mood.
> 
> Mmm. I am not that great in English, but how "Properly ignore..." is not 
> "imperative mood"? What would you suggest?
> I should, however, use lowercase (alas, force of habit).

I will refer to my phrase:"I could be wrong") Leave it as is.

> 
>> On 15.12.2020 17:25, Sergey Nikiforov wrote:
>>> Not all invalid characters were ignored by base64 decoder
>>> causing data corruption and reads beyond decode table
>>> (faults under ASAN).
>>>
>>> Added corresponding check into base64 unit test.
>>>
>>> Fixes: #5627
>>> ---
>>>
>>> Branch: 
>>> https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing 
>>>
>>> Issue: https://github.com/tarantool/tarantool/issues/5627
>>>
>>>   test/unit/base64.c      | 23 ++++++++++++++++++++++-
>>>   test/unit/base64.result |  5 ++++-
>>>   third_party/base64.c    |  3 ++-
>>>   3 files changed, 28 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/test/unit/base64.c b/test/unit/base64.c
>>> index ada497adf..c0f53a5e1 100644
>>> --- a/test/unit/base64.c
>>> +++ b/test/unit/base64.c
>>> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>>>       base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>>>   }
>>> +static void
>>> +base64_invalid_chars_test(void)
>>> +{
>>> +    /* Upper bit must be cleared */
>>> +    const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
>>> +    char outbuf[8];
>>> +
>>> +    plan(1);
>>
>> Usually `plan ()` is called as the first call in a function. It's just
>> easier to see how many checks there will be. I don't know any rule about
>> this. So, it's up to you.
> 
> I would move it if you like (if there would be another patch revision). 
> I have just tried to be C89-friendly. Force of habit (useful one).
> 

Ok.

>>> +
>>> +    /* Invalid chars should be ignored, not decoded into garbage */
>>> +    is(base64_decode(invalid_data, sizeof(invalid_data),
>>> +                     outbuf, sizeof(outbuf)),
>>> +       0, "ignoring invalid chars");
>>> +
>>> +    check_plan();
>>> +}
>>> +
>>>   int main(int argc, char *argv[])
>>>   {
>>> -    plan(28);
>>> +    plan(28
>>> +         + 1 /* invalid chars test */
>>> +         );
>>
>> I agree with Vlad. Why `+ 1` and not just 29?
> 
> Using "magic" values without explanation is a bad idea for readability. 
> Should I "decode" how 28 was calculated as well (using constants where 
> appropriate) or no one bothers so much with tests?

AFAIK in `plan()` we always just write the "total" number of checks.

> 
>>>       header();
>>>       const char *option_tests[] = {
>>> @@ -78,6 +97,8 @@ int main(int argc, char *argv[])
>>>           base64_nowrap_test(option_tests[i]);
>>>       }
>>> +    base64_invalid_chars_test();
>>> +
>>>       footer();
>>>       return check_plan();
>>>   }
>>> diff --git a/test/unit/base64.result b/test/unit/base64.result
>>> index cd1f2b3f6..3bc2c2275 100644
>>> --- a/test/unit/base64.result
>>> +++ b/test/unit/base64.result
>>> @@ -1,4 +1,4 @@
>>> -1..28
>>> +1..29
>>>       *** main ***
>>>       1..3
>>>       ok 1 - length
>>> @@ -175,4 +175,7 @@ ok 27 - subtests
>>>       ok 3 - decode length ok
>>>       ok 4 - encode/decode
>>>   ok 28 - subtests
>>> +    1..1
>>> +    ok 1 - ignoring invalid chars
>>> +ok 29 - subtests
>>>       *** main: done ***
>>> diff --git a/third_party/base64.c b/third_party/base64.c
>>> index 8ecab23eb..7c69315ea 100644
>>> --- a/third_party/base64.c
>>> +++ b/third_party/base64.c
>>> @@ -222,7 +222,8 @@ base64_decode_value(int value)
>>>           32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
>>>           44, 45, 46, 47, 48, 49, 50, 51
>>>       };
>>> -    static const int decoding_size = sizeof(decoding);
>>> +    static const int decoding_size =
>>> +        sizeof(decoding) / sizeof(decoding[0]);
>>>       int codepos = value;
>>>       codepos -= 43;
>>>       if (codepos < 0 || codepos >= decoding_size)
>>>

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Leonid Vasiliev]

Hi!
Just to be clear. Perhaps before I have not clearly expressed my
opinion.
If you ok with 29 instead
```
+    plan(28
+         + 1 /* invalid chars test */
+         );
```
then LGTM.

On 17.12.2020 17:52, Leonid Vasiliev via Tarantool-patches wrote:
> Hi!
> 
> On 17.12.2020 16:04, Sergey Nikiforov wrote:
>> Hi!
>>
>> On 17.12.2020 12:41, Leonid Vasiliev wrote:
>>> Hi! Thank you for the patch.
>>> Generally LGTM.
>>> See some comments below:
>>>
>>> According to 
>>> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message 
>>>
>>> "Description is what the patch does, started from lowercase letter,
>>> without a dot in the end, in the imperative mood."
>>> ("properly...").
>>> I could be wrong, but it seems like <description> is not written in an
>>> imperative mood.
>>
>> Mmm. I am not that great in English, but how "Properly ignore..." is 
>> not "imperative mood"? What would you suggest?
>> I should, however, use lowercase (alas, force of habit).
> 
> I will refer to my phrase:"I could be wrong") Leave it as is.
> 
>>
>>> On 15.12.2020 17:25, Sergey Nikiforov wrote:
>>>> Not all invalid characters were ignored by base64 decoder
>>>> causing data corruption and reads beyond decode table
>>>> (faults under ASAN).
>>>>
>>>> Added corresponding check into base64 unit test.
>>>>
>>>> Fixes: #5627
>>>> ---
>>>>
>>>> Branch: 
>>>> https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing 
>>>>
>>>> Issue: https://github.com/tarantool/tarantool/issues/5627
>>>>
>>>>   test/unit/base64.c      | 23 ++++++++++++++++++++++-
>>>>   test/unit/base64.result |  5 ++++-
>>>>   third_party/base64.c    |  3 ++-
>>>>   3 files changed, 28 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/test/unit/base64.c b/test/unit/base64.c
>>>> index ada497adf..c0f53a5e1 100644
>>>> --- a/test/unit/base64.c
>>>> +++ b/test/unit/base64.c
>>>> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>>>>       base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>>>>   }
>>>> +static void
>>>> +base64_invalid_chars_test(void)
>>>> +{
>>>> +    /* Upper bit must be cleared */
>>>> +    const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
>>>> +    char outbuf[8];
>>>> +
>>>> +    plan(1);
>>>
>>> Usually `plan ()` is called as the first call in a function. It's just
>>> easier to see how many checks there will be. I don't know any rule about
>>> this. So, it's up to you.
>>
>> I would move it if you like (if there would be another patch 
>> revision). I have just tried to be C89-friendly. Force of habit 
>> (useful one).
>>
> 
> Ok.
> 
>>>> +
>>>> +    /* Invalid chars should be ignored, not decoded into garbage */
>>>> +    is(base64_decode(invalid_data, sizeof(invalid_data),
>>>> +                     outbuf, sizeof(outbuf)),
>>>> +       0, "ignoring invalid chars");
>>>> +
>>>> +    check_plan();
>>>> +}
>>>> +
>>>>   int main(int argc, char *argv[])
>>>>   {
>>>> -    plan(28);
>>>> +    plan(28
>>>> +         + 1 /* invalid chars test */
>>>> +         );
>>>
>>> I agree with Vlad. Why `+ 1` and not just 29?
>>
>> Using "magic" values without explanation is a bad idea for 
>> readability. Should I "decode" how 28 was calculated as well (using 
>> constants where appropriate) or no one bothers so much with tests?
> 
> AFAIK in `plan()` we always just write the "total" number of checks.
> 
>>
>>>>       header();
>>>>       const char *option_tests[] = {
>>>> @@ -78,6 +97,8 @@ int main(int argc, char *argv[])
>>>>           base64_nowrap_test(option_tests[i]);
>>>>       }
>>>> +    base64_invalid_chars_test();
>>>> +
>>>>       footer();
>>>>       return check_plan();
>>>>   }
>>>> diff --git a/test/unit/base64.result b/test/unit/base64.result
>>>> index cd1f2b3f6..3bc2c2275 100644
>>>> --- a/test/unit/base64.result
>>>> +++ b/test/unit/base64.result
>>>> @@ -1,4 +1,4 @@
>>>> -1..28
>>>> +1..29
>>>>       *** main ***
>>>>       1..3
>>>>       ok 1 - length
>>>> @@ -175,4 +175,7 @@ ok 27 - subtests
>>>>       ok 3 - decode length ok
>>>>       ok 4 - encode/decode
>>>>   ok 28 - subtests
>>>> +    1..1
>>>> +    ok 1 - ignoring invalid chars
>>>> +ok 29 - subtests
>>>>       *** main: done ***
>>>> diff --git a/third_party/base64.c b/third_party/base64.c
>>>> index 8ecab23eb..7c69315ea 100644
>>>> --- a/third_party/base64.c
>>>> +++ b/third_party/base64.c
>>>> @@ -222,7 +222,8 @@ base64_decode_value(int value)
>>>>           32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
>>>>           44, 45, 46, 47, 48, 49, 50, 51
>>>>       };
>>>> -    static const int decoding_size = sizeof(decoding);
>>>> +    static const int decoding_size =
>>>> +        sizeof(decoding) / sizeof(decoding[0]);
>>>>       int codepos = value;
>>>>       codepos -= 43;
>>>>       if (codepos < 0 || codepos >= decoding_size)
>>>>

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Vladislav Shpilevoy]

Alexander Tikh., please, tell if we can merge branch
void234/gh-5627-fix-base64-invalid-chars-processing-v3.

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Alexander V. Tikhonov]

Hi Sergey, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/230156990

On Tue, Dec 15, 2020 at 05:25:27PM +0300, Sergey Nikiforov via Tarantool-patches wrote:
> Not all invalid characters were ignored by base64 decoder
> causing data corruption and reads beyond decode table
> (faults under ASAN).
> 
> Added corresponding check into base64 unit test.
> 
> Fixes: #5627
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/void234/gh-5627-fix-base64-invalid-chars-processing
> Issue: https://github.com/tarantool/tarantool/issues/5627
> 
>  test/unit/base64.c      | 23 ++++++++++++++++++++++-
>  test/unit/base64.result |  5 ++++-
>  third_party/base64.c    |  3 ++-
>  3 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/test/unit/base64.c b/test/unit/base64.c
> index ada497adf..c0f53a5e1 100644
> --- a/test/unit/base64.c
> +++ b/test/unit/base64.c
> @@ -58,9 +58,28 @@ base64_nowrap_test(const char *str)
>  	base64_test(str, BASE64_NOWRAP, symbols, lengthof(symbols));
>  }
>  
> +static void
> +base64_invalid_chars_test(void)
> +{
> +	/* Upper bit must be cleared */
> +	const char invalid_data[] = { '\x7b', '\x7c', '\x7d', '\x7e' };
> +	char outbuf[8];
> +
> +	plan(1);
> +
> +	/* Invalid chars should be ignored, not decoded into garbage */
> +	is(base64_decode(invalid_data, sizeof(invalid_data),
> +	                 outbuf, sizeof(outbuf)),
> +	   0, "ignoring invalid chars");
> +
> +	check_plan();
> +}
> +
>  int main(int argc, char *argv[])
>  {
> -	plan(28);
> +	plan(28
> +	     + 1 /* invalid chars test */
> +	     );
>  	header();
>  
>  	const char *option_tests[] = {
> @@ -78,6 +97,8 @@ int main(int argc, char *argv[])
>  		base64_nowrap_test(option_tests[i]);
>  	}
>  
> +	base64_invalid_chars_test();
> +
>  	footer();
>  	return check_plan();
>  }
> diff --git a/test/unit/base64.result b/test/unit/base64.result
> index cd1f2b3f6..3bc2c2275 100644
> --- a/test/unit/base64.result
> +++ b/test/unit/base64.result
> @@ -1,4 +1,4 @@
> -1..28
> +1..29
>  	*** main ***
>      1..3
>      ok 1 - length
> @@ -175,4 +175,7 @@ ok 27 - subtests
>      ok 3 - decode length ok
>      ok 4 - encode/decode
>  ok 28 - subtests
> +    1..1
> +    ok 1 - ignoring invalid chars
> +ok 29 - subtests
>  	*** main: done ***
> diff --git a/third_party/base64.c b/third_party/base64.c
> index 8ecab23eb..7c69315ea 100644
> --- a/third_party/base64.c
> +++ b/third_party/base64.c
> @@ -222,7 +222,8 @@ base64_decode_value(int value)
>  		32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43,
>  		44, 45, 46, 47, 48, 49, 50, 51
>  	};
> -	static const int decoding_size = sizeof(decoding);
> +	static const int decoding_size =
> +		sizeof(decoding) / sizeof(decoding[0]);
>  	int codepos = value;
>  	codepos -= 43;
>  	if (codepos < 0 || codepos >= decoding_size)
> -- 
> 2.25.1
> 

Re: [Tarantool-patches] [PATCH v2] base64: Properly ignore invalid characters

[Alexander Turenko]

On Wed, Dec 23, 2020 at 04:17:29PM +0100, Vladislav Shpilevoy via Tarantool-patches wrote:
> Alexander Tikh., please, tell if we can merge branch
> void234/gh-5627-fix-base64-invalid-chars-processing-v3.

Pushed to master, 2.6, 2.5 and 1.10.

WBR, Alexander Turenko.