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 gg dissector arbitary length heap overflow #606

Merged
merged 1 commit into from Dec 18, 2014

Conversation

NickSampanis
Copy link
Contributor

copies length of bytes that we control and writes out of buffer bounds

@LocutusOfBorg
Copy link
Contributor

why do you cast to int? it is uint32 if I read correctly. is the cast really needed?

@NickSampanis
Copy link
Contributor Author

I should cast to int because i should check if gg_len-22 is less than zero, uint32 is always positive. I will give a brief example. Lets assume that gg_len is equals to 21
if ((gg->type == GG_LOGIN50_CMD) && !FROM_SERVER("gg", PACKET)) {
strncpy(tbuf2,gg_login50->description, (gg->len)-22);
tbuf2[(gg->len)-22]='\0';
}
will end up copying -1(0xffffffff) bytes from gg_login50->description to tbuf2. A simple way to avoid this is to check if gg->len-22 is less than zero, but gg->len is unsigned, so even if the gg->len is 21 the statment if (gg->len-22 < 0) would be false(because gg->len after the subtraction will contain the value 4294967295), unless of course if we cast back to signed integer. I recommend you to read "The Art of Software Security Assessment" if you really want to understand this or dive in to assembly and try to understand the difference between jle and jbe

@LocutusOfBorg
Copy link
Contributor

ops sorry, yes of course I understand now :)

I thought you had written different code, let me explain, I missed the "<" for a ">"

so now, what abut this code?

 if (gg->len < 22)
 return NULL;

instead of

 if ((int)gg->len-22 < 0)
 return NULL;

@NickSampanis
Copy link
Contributor Author

only if you do it like

if ((int)gg->len < 22)
return NULL;

i explained the signedness error above

LocutusOfBorg added a commit that referenced this pull request Dec 18, 2014
Fix gg dissector arbitary length heap overflow
@LocutusOfBorg LocutusOfBorg merged commit 5f4ea58 into Ettercap:master Dec 18, 2014
@LocutusOfBorg
Copy link
Contributor

Merging too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants