Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ssl_context_info.c to correctly check for EOF - type-limit bug #3449

Closed
wants to merge 2 commits into from
Closed

Fix ssl_context_info.c to correctly check for EOF - type-limit bug #3449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89cf0f3
programs/ssl: Fixes incorrect EOF check in ssl_context_info.c
naynajain Aug 13, 2020
b0aadc4
programs/ssl: add EOF testcase for ssl_context_info.c
naynajain Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ChangeLog.d/bugfix_type_limit_warning_ssl_context_info.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Bugfix
* read_next_b64_code() isn't correctly checking against EOF because it checks against 'char' rather than 'int'. This is identified via type-limit warning reported by gcc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format changelog entries to 80 columns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please word changelog entries for users of the libraries, not for maintainers. read_next_b64_code is not meaningful to users, ssl_context_info is. This is a bug in ssl_context_info which, on some platforms, is silently truncating base64 data at character \377 rather than ignoring it.

Reported and fix contributed by naynajain in #3449.
4 changes: 2 additions & 2 deletions programs/ssl/ssl_context_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,13 +379,13 @@ size_t read_next_b64_code( uint8_t **b64, size_t *max_len )
int valid_balance = 0; /* balance between valid and invalid characters */
size_t len = 0;
char pad = 0;
char c = 0;
int c = 0;

while( EOF != c )
{
char c_valid = 0;

c = (char) fgetc( b64_file );
c = fgetc( b64_file );

if( pad > 0 )
{
Expand Down
7 changes: 6 additions & 1 deletion tests/context-info.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,12 @@ run_test "Wrong base64 format" \
"def_bad_b64.txt" \
-m "ERROR" \
-u "The length of the base64 code found should be a multiple of 4" \
-n "bytes left to analyze from context"
-n "bytes left to analyze from context" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be a trailing backslash here since there is no continuation line.


run_test "EOF base64 format" \
"def_bad_b64_eof.txt" \
-m "Finished. No valid base64 code found" \
-n "ERROR" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be a trailing backslash here since there is no continuation line.


run_test "Too much data at the beginning of base64 code" \
"def_b64_too_big_1.txt" \
Expand Down
1 change: 1 addition & 0 deletions tests/data_files/base64/def_bad_b64_eof.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/w==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no “EOFish” character (byte 0xff) here. This is a well-formed file containing Base64 data. The encoded data isn't a valid SSL context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no “EOFish” character (byte 0xff) here. This is a well-formed file containing Base64 data. The encoded data isn't a valid SSL context.

Thanks for your review.
It is actually base64 encoding of 0xFF. So, not sure what do you mean by "no "EOFish""

And yes, it is not a valid SSL Context. It is just meant to represent EOF to test that it can be interpreted correctly by read_next_b64_code() function.

Am I missing something ?

Thanks & Regards,
- Nayna

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug was in the parsing of the base64-encoded, not in the handling of the decoded data. So the fact that the decoded data contains a 0xff is irrelevant. What matters is when the file itself contains a 0xff.