Created attachment 398884 [details] Example files There are two trivial stack buffer overflows in diffball's parsers for bdelta and gdiff files: * bdeltaReconstructDCBuff reads a uint8 size field and then reads three times that number of bytes from the input file into a 12-byte stack buffer, with no further checks: #define BUFF_SIZE 12 unsigned char buff[BUFF_SIZE]; ... cread(patchf, buff, 1); int_size = buff[0]; ... cread(patchf, buff, 3 * int_size); * gdiffReconstructDCBuff reads 13 bytes of input data into a 5-byte buffer. This is annoying to reproduce and probably isn't as exploitable. If *buff == 255 it effectively does this: unsigned char buff[5]; ... ob=8; lb=4; cread(patchf, buff + 1, lb + ob) Sample files demonstrating both of these issues are attached, use them like so: patcher hello_kitty.jp2 diffball-differ-crash.bdelta /dev/null I haven't reported this upstream because there doesn't appear to be much of an upstream to speak of; diffball was developed by a Gentoo developer, is mainly used by Gentoo (primarily in emerge-delta-webrsync) and upstream development appears dead.
Make that three, the xdelta1 parser has another one. This function is called with buff pointing to a 32-byte stack buffer: unsigned long inline readXDInt(cfile *patchf, unsigned char *buff) { unsigned long num=0; signed int count=-1; do { count++; cread(patchf, buff + count, 1); } while(buff[count] & 0x80); for(; count >= 0; count--) { num <<= 7; num |= (buff[count] & 0x7f); } return num; } Also, it appears I can gain control of the instruction pointer using the bdeltaReconstructDCBuff buffer overflow. Stack smashing protection doesn't help much here - since dcbuff is allocated in the parent's stack frame and contains function pointers that are called by bdeltaReconstructDCBuff, we can overwrite them and gain control of EIP before the stack canary ever gets checked. Guessing this means it's an arbitrary code execution bug on non-hardened.
(In reply to Aidan Thornton from comment #0) Thanks for the reports. Adding the maintainer for comment
Hi Aidan, FYI, we are working on it. Unfortunately I hadn't enough time to work on this properly yet, but I'll update you here when I have patches for the reported issues. thanks
CC'ing Brian Harring, diffball author
Looks of it someone took a fuzzer to the codebase.... Regarding the bdelta format, if you've not looked at the bdelta implementation in the tree I'd suggest so. I'd assume it has a similar issue offhand. Bdelta == deltup format- basically is dead as far as I know. I'll take you at your word for the readXDInt bit; I don't think it's as trivial as it appears due to the high bit requirement, but that's probably just a matter of window dressing to bypass it. Either way, the fixes are simple and a 1.0.2 can be cut pretty much now; I've already confirmed the overflows are fixed. @gentoo security peeps, k_f in particular; you're aparently asking that the fixes be kept private until there is a sign off from Aidan? Can you clarify that?
(In reply to Brian Harring from comment #5) > Looks of it someone took a fuzzer to the codebase.... > Indeed, been popular recently, and a good thing, thanks :) > Either way, the fixes are simple and a 1.0.2 can be cut pretty much now; > I've already confirmed the overflows are fixed. Very good. > > @gentoo security peeps, k_f in particular; you're aparently asking that the > fixes be kept private until there is a sign off from Aidan? Can you clarify > that? (i) If a new release is cut from what is considered upstream of the package making the issues public we can make it public here as well (publication should include proper credits for discovery). (ii) If there is an external upstream we need to privately alert them. (iii) We also need to decide whether these fixes merits zero, one or more CVEs, what is your take on this?
Hey, sorry, I kind of lost track of what's happening with this. Could anyone fill me in. Again, sorry about this, I'm not normally in the security reporting business.
(In reply to Aidan Thornton from comment #7) > Hey, sorry, I kind of lost track of what's happening with this. Could anyone > fill me in. Again, sorry about this, I'm not normally in the security > reporting business. From my last update, it seems that Brian (upstream maintainer) is working on patches.
(In reply to Brian Harring from comment #5) > @gentoo security peeps, k_f in particular; you're aparently asking that the > fixes be kept private until there is a sign off from Aidan? Can you clarify > that? Brian, since you are the upstream maintainer of this code (unless I am wrong). You are free to let us know to make it public any time you would like to. We made it confidential until upstream (you) were notified and then can choose to release it publicly or keep it private until you have patches ready. As a side note, when you make it public we encourage you to give Aidan credit for the discovery. We can also help request the CVE number, as part of publishing it to oss-security.
(In reply to Rafael Martins from comment #8) > From my last update, it seems that Brian (upstream maintainer) is working on > patches. Rafael do you have any updates on this?
Are we ready to make this public?
Two months have went by, is this public material now? Also what about a fix? the patch is here.
This has been not fixed in a year. Please advise if this is to be fixed, or do we remove it from Tree.
Talked with rafaelmartins - He will try to contact upstream. If not successful will try to patch himself.
This bug has been here for two (2) years. Rafael, please either fix or drop from portage.
ping...
Rafael ack'ed in IRC that we can las-rite package. Treecleaners, please start your magic!
(In reply to Thomas Deutschmann from comment #17) > Rafael ack'ed in IRC that we can las-rite package. Treecleaners, please > start your magic! just confirming it here. I have no time/energy to work on this at this point. if someone wants to take care of it, that's the time to step up. sorry guys :(
$ rdep */diffball == rdep of dev-util/diffball == app-arch/tarsync-0.2.1-r1 app-portage/distpatch-0.1.2-r1 app-portage/emerge-delta-webrsync-3.7.5 app-portage/emerge-delta-webrsync-3.7.6 The first one is unmaintained. The second one is also Rafael's, so I suppose he'd be fine with it. The third one is high-profile dev-portage package, so I don't think users will be happy seeing it gone.
Sent upstream PR: https://github.com/rafaelmartins/diffball/pull/2 Sent gentoo PR: https://github.com/gentoo/gentoo/pull/13471
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=c1faef3a67ae5904fcd8e95495b005e268f1bc76 commit c1faef3a67ae5904fcd8e95495b005e268f1bc76 Author: Zac Medico <zmedico@gentoo.org> AuthorDate: 2019-10-27 22:14:09 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2019-10-27 23:51:03 +0000 dev-util/diffball: revbump to 1.0.1-r1 for bug 543310 Fix stack buffer overflows reported in bug 543310. Bug: https://bugs.gentoo.org/543310 See: https://github.com/zmedico/diffball/pull/1 Reported-by: Aidan Thornton <makosoft@googlemail.com> Package-Manager: Portage-2.3.78, Repoman-2.3.17 Signed-off-by: Zac Medico <zmedico@gentoo.org> dev-util/diffball/Manifest | 2 ++ dev-util/diffball/diffball-1.0.1-r1.ebuild | 38 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
amd64 stable
ppc stable
ia64 stable
x86 stable. Maintainer(s), please cleanup. Security, please vote.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=31862fbc2744fd8fda8a30611daafe22ed97dbf5 commit 31862fbc2744fd8fda8a30611daafe22ed97dbf5 Author: Zac Medico <zmedico@gentoo.org> AuthorDate: 2020-03-18 16:11:59 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2020-03-18 16:14:49 +0000 dev-util/diffball: remove vulnerable versions (bug 543310) Bug: https://bugs.gentoo.org/543310 Package-Manager: Portage-2.3.94, Repoman-2.3.21 Signed-off-by: Zac Medico <zmedico@gentoo.org> dev-util/diffball/Manifest | 1 - dev-util/diffball/diffball-1.0.1-r1.ebuild | 38 ------------------------------ dev-util/diffball/diffball-1.0.1.ebuild | 34 -------------------------- 3 files changed, 73 deletions(-)
GLSA Vote: No Repository is clean, all done!
