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.

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 invalid read at CleanMSG #2391

Merged
merged 2 commits into from
Feb 6, 2019
Merged

Fix invalid read at CleanMSG #2391

merged 2 commits into from
Feb 6, 2019

Conversation

cristgl
Copy link
Contributor

@cristgl cristgl commented Jan 17, 2019

This PR fixes the next invalid read:

==1724== Invalid read of size 1
==1724==    at 0x128411: OS_CleanMSG (cleanevent.c:70)
==1724==    by 0x1330C6: w_decode_event_thread (analysisd.c:2012)
==1724==    by 0x585C6DA: start_thread (pthread_create.c:463)
==1724==    by 0x5B9588E: clone (clone.S:95)
==1724==  Address 0x72da740 is 0 bytes after a block of size 16 alloc'd
==1724==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1724==    by 0x5B119B9: strdup (strdup.c:42)
==1724==    by 0x13220C: ad_input_main (analysisd.c:1687)
==1724==    by 0x585C6DA: start_thread (pthread_create.c:463)
==1724==    by 0x5B9588E: clone (clone.S:95)

The code was intended to fix the next specific log with an umlaut: Mär 02 17:30:52.
This character is stored in two bytes, 195 164, and the message received at CleanMSG has 4 bytes as minimum. Accessing pieces[1] could cause an invalid read as the message would have 5 bytes counting the id. To fix it, the length of the log has to be higher than 4, because Mär counts as 4 bytes.

@cristgl cristgl requested a review from chemamartinez January 17, 2019 15:25
@chemamartinez chemamartinez added the type/bug Something isn't working label Jan 17, 2019
@chemamartinez chemamartinez self-assigned this Jan 21, 2019
@@ -67,7 +67,7 @@ int OS_CleanMSG(char *msg, Eventinfo *lf)
* repair to only one slot so we can detect the correct date format in the next step
* ex: Mär 02 17:30:52
*/
if (pieces[1] == (char) 195) {
if (loglen >= 5 && pieces[1] == (char) 195) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi @cristgl,

Why loglen must be greater or equal than 5?
I would say that it must be 2:

Let pieces be { 195, 164, 0 }.
strlen(pieces) = 2
_loglen = strlen(pieces) + 1 = 3

loglen = 3 guarantees that pieces[0], pieces[1] and pieces[2] are defined.

if (loglen >= 3 && pieces[1] == 195 && pieces[2] == 164) { ... }

I think it fits. Am I wrong? Please clarify your example.

Cheers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vikman90,

You are completely right, I was taking into account the 'id' because somehow I thought loglen was counting the string composed by "id:msg". Thank you for your observation.

Cheers

@vikman90 vikman90 self-assigned this Feb 6, 2019
Copy link
Member

@vikman90 vikman90 left a comment

Choose a reason for hiding this comment

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

It makes much sense now.
LGTM!

@vikman90 vikman90 changed the base branch from 3.8 to 3.9 February 6, 2019 07:01
@vikman90 vikman90 merged commit e8f663a into 3.9 Feb 6, 2019
@vikman90 vikman90 deleted the fix-invalid-read-analysisd branch February 6, 2019 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants