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 radius stack overflow #607

Merged
merged 1 commit into from
Dec 18, 2014
Merged

Conversation

NickSampanis
Copy link
Contributor

function radius_get_attribute pass by reference attr_len, which is used as length argument in strncpy. radius_get_attribute does a subtraction -2, which may have a negative result (0xffff-0xfffe for u_int16). This value will lead to stack corruption

@LocutusOfBorg
Copy link
Contributor

Hi @NickSampanis sorry for taking so long to look at them, but I'm worried about this one.
I don't understand why we should use uint16 if the value won't be greater than uint8.
Can you please clarify?

If 8 bits is enough I propose to change the attr_len variable type, otherwise I propose to add a length check instead of the cast.

(maybe I'm looking at the wrong problem)

@NickSampanis
Copy link
Contributor Author

It's ok, The cves will be published shortly and some denial of service exploits too :). This bug has to do with how the c language does mathematical calculations with non int data types. C language automatically casts non int datatypes to int in order to do arithmetic calculations. So the statement
attr_len = *(begin + 1) - 2

if the next character of the begin pointer is equal to one the result will be 0xffffffff, but because attr_len is 2 bytes it could hold only 0xffff and thats why i used the cast. Now in all my patches i am trying to wipe out the security bug without modifying the code despite what i think on how efficient is the implementation. It would be better if attr_len type is uint8 but in order to do that you should modify dissector_radius function too

@LocutusOfBorg
Copy link
Contributor

Feel free to do that :D

LocutusOfBorg added a commit that referenced this pull request Dec 18, 2014
Fix radius stack overflow
@LocutusOfBorg LocutusOfBorg merged commit 037aa9a into Ettercap:master Dec 18, 2014
@LocutusOfBorg
Copy link
Contributor

Merging in the meanwhile

@LocutusOfBorg
Copy link
Contributor

I did the change, Nick, can you please check
#635 ? thanks!

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