FIXED: Incorrect error checking (negative array read) #20

Closed
MegaManSec opened this Issue Aug 25, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@MegaManSec

Hi,

in ParseMaraRc.c in the new_dvar() function:
450 num = dkeyword2num(name);
451 if(dvar[num] != 0 || num < 0 || num > DKEYCOUNT)

The check for "num < 0" should be done before "dvar[num] != 0", as dkeyword2num may return a negative number.

Thanks,

@Remmy

This comment has been minimized.

Show comment
Hide comment
@Remmy

Remmy Aug 25, 2015

Also, does this not suffer the same problem as issue #19, and should the check be for num >= DKEYCOUNT?

Remmy commented Aug 25, 2015

Also, does this not suffer the same problem as issue #19, and should the check be for num >= DKEYCOUNT?

@MegaManSec

This comment has been minimized.

Show comment
Hide comment
@MegaManSec

MegaManSec Aug 25, 2015

Yes, you're right :)
Well, issue #19 should be patched by changing the check to "index > DKEYCOUNT".
This one should be fixed by moving the 'dvar[num] != 0' check to the end, and changing the dkey check to "index > DKEYCOUNT", too.

Thanks

Yes, you're right :)
Well, issue #19 should be patched by changing the check to "index > DKEYCOUNT".
This one should be fixed by moving the 'dvar[num] != 0' check to the end, and changing the dkey check to "index > DKEYCOUNT", too.

Thanks

samboy added a commit that referenced this issue Aug 26, 2015

Fixing two buffer overflows:
#19 is non-exploitable, since
index will never be DKEYCOUNT in production code

#20 would only be exploitable
if

1) An attacker controls one’s mararc file (at this point, the attacker
   would probably need to already be root)
2) The memory location two pointers below the beginning of dvar is set
   to zero

This is not serious enough for me to make a 2.0.13 MaraDNS release, but
2.0.13 will have the fix, along with a note that a minor security
problem has been fixed.

Thanks for the bug reports.
@samboy

This comment has been minimized.

Show comment
Hide comment
@samboy

samboy Aug 26, 2015

Owner

The exploit vector requires the attacker to control a mararc file, a file usually only edited by root, and requires whatever memory location two pointers before the beginning of dvar to be precisely zero, which should be unlikely.

I’ve fixed this, but this isn’t serious enough for me to waste two to three hours making a new MaraDNS release, especially since I just made a release last week.

Owner

samboy commented Aug 26, 2015

The exploit vector requires the attacker to control a mararc file, a file usually only edited by root, and requires whatever memory location two pointers before the beginning of dvar to be precisely zero, which should be unlikely.

I’ve fixed this, but this isn’t serious enough for me to waste two to three hours making a new MaraDNS release, especially since I just made a release last week.

@samboy samboy closed this Aug 26, 2015

@samboy

This comment has been minimized.

Show comment
Hide comment
@samboy

samboy Sep 1, 2015

Owner

I just checked, and, yes, MaraDNS 1.0.00 from 2002 has this particular bug. Most of the things like this people are finding these days come from the 2001-2002 codebase; I wrote too much code too quickly because there was a hurry to have an open-source DNS server that wasn’t BIND at the time.

Owner

samboy commented Sep 1, 2015

I just checked, and, yes, MaraDNS 1.0.00 from 2002 has this particular bug. Most of the things like this people are finding these days come from the 2001-2002 codebase; I wrote too much code too quickly because there was a hurry to have an open-source DNS server that wasn’t BIND at the time.

@samboy

This comment has been minimized.

Show comment
Hide comment
@samboy

samboy Sep 2, 2015

Owner

Another thing: The exploit is actually very limited. You can’t write to any memory with this bug; you can only force MaraDNS to read from a memory location she should not read from.

Owner

samboy commented Sep 2, 2015

Another thing: The exploit is actually very limited. You can’t write to any memory with this bug; you can only force MaraDNS to read from a memory location she should not read from.

@samboy samboy changed the title from Incorrect error checking (negative array read) to FIXED: Incorrect error checking (negative array read) May 6, 2016

Repository owner locked and limited conversation to collaborators May 21, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.