heap out of bounds read in copyTdEntry() #133

Closed
hannob opened this Issue Jan 25, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@hannob

hannob commented Jan 25, 2017

The attached file will cause an out of bounds heap read access when passed to rpm (tested with rpm -i --test [input]). Found with american fuzzy lop and address sanitizer.

oob-heap-copyTdEntry.zip

Stack trace from asan:

==25558==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61a000012501 at pc 0x0000004b56e5 bp 0x7ffe1fa11e90 sp 0x7ffe1fa11640
READ of size 592 at 0x61a000012501 thread T0
    #0 0x4b56e4 in __asan_memcpy (/r/rpm/rpm+0x4b56e4)
    #1 0x5dd92e in copyTdEntry /f/rpm/rpm/lib/header.c:1074:23
    #2 0x5d82af in intGetTdEntry /f/rpm/rpm/lib/header.c:1294:7
    #3 0x5d71b1 in headerGet /f/rpm/rpm/lib/header.c:1317:10
    #4 0x6373a9 in rpmpkgRead /f/rpm/rpm/lib/package.c:365:6
    #5 0x6373a9 in rpmReadPackageFile /f/rpm/rpm/lib/package.c:432
    #6 0x579658 in tryReadHeader /f/rpm/rpm/lib/rpminstall.c:353:17
    #7 0x579658 in rpmInstall /f/rpm/rpm/lib/rpminstall.c:537
    #8 0x5057ae in main /f/rpm/rpm/rpmqv.c:295:12
    #9 0x7f9d10ee078f in __libc_start_main (/lib64/libc.so.6+0x2078f)
    #10 0x41c648 in _start (/r/rpm/rpm+0x41c648)

0x61a000012501 is located 0 bytes to the right of 1153-byte region [0x61a000012080,0x61a000012501)
allocated by thread T0 here:
    #0 0x4cc7a8 in malloc (/r/rpm/rpm+0x4cc7a8)
    #1 0x674ff4 in rmalloc /f/rpm/rpm/rpmio/rpmmalloc.c:44:13
    #2 0x636804 in rpmpkgReadHeader /f/rpm/rpm/lib/package.c:262:9
    #3 0x6371da in rpmpkgRead /f/rpm/rpm/lib/package.c:340:10
    #4 0x6371da in rpmReadPackageFile /f/rpm/rpm/lib/package.c:432
    #5 0x579658 in tryReadHeader /f/rpm/rpm/lib/rpminstall.c:353:17
    #6 0x579658 in rpmInstall /f/rpm/rpm/lib/rpminstall.c:537
    #7 0x5057ae in main /f/rpm/rpm/rpmqv.c:295:12

SUMMARY: AddressSanitizer: heap-buffer-overflow (/r/rpm/rpm+0x4b56e4) in __asan_memcpy
@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Jan 25, 2017

Contributor

Need to check if this happens before GPG verification.

Contributor

cgwalters commented Jan 25, 2017

Need to check if this happens before GPG verification.

@pmatilai

This comment has been minimized.

Show comment
Hide comment
@pmatilai

pmatilai Jan 26, 2017

Contributor

Right, this is (yet another) case where rpm unnecessarily shoots itself in the foot: it does a relatively complex thing of reconstructing the original header from the one we just read and imported in order to check the digest and signature, when it could just check the raw data before looking inside the header at all.

Removing the rain dance with retrieving the immutable header region is a work in progress and have some preliminary patches for that, it'll make a whole class of these issues go away. In the meanwhile we'll need to patch up the existing versions with something less drastic though.

Contributor

pmatilai commented Jan 26, 2017

Right, this is (yet another) case where rpm unnecessarily shoots itself in the foot: it does a relatively complex thing of reconstructing the original header from the one we just read and imported in order to check the digest and signature, when it could just check the raw data before looking inside the header at all.

Removing the rain dance with retrieving the immutable header region is a work in progress and have some preliminary patches for that, it'll make a whole class of these issues go away. In the meanwhile we'll need to patch up the existing versions with something less drastic though.

@pmatilai

This comment has been minimized.

Show comment
Hide comment
@pmatilai

pmatilai Feb 2, 2017

Contributor

Just FWIW, this is enough to catch all of #133, #135, #136, #138 and #139:

--- a/lib/header.c
+++ b/lib/header.c
@@ -255,6 +255,8 @@ static rpmRC hdrblobVerifyInfo(hdrblob blob, char **emsg)
        if (end > info.offset)
            goto err;
 
+       if (info.tag < HEADER_I18NTABLE)
+           goto err;
        if (hdrchkType(info.type))
            goto err;
        if (hdrchkAlign(info.type, info.offset))

Hysterically there are no checks whatsoever on the tag values in rpm. In this particular case it's an out-of-place immutable region tag which causes assumptions in the code fail, in #135, #136, #138 and #139 it's a required tag replaced with a negative value. So catching stuff below the normal tag range will minimally cover all these. There are other layers present too, like missing sanity checks on tag types all over the place.

Also it's perhaps worth pointing out that none of the packages in the series crash nor pass through 'rpm -K' verification.

Contributor

pmatilai commented Feb 2, 2017

Just FWIW, this is enough to catch all of #133, #135, #136, #138 and #139:

--- a/lib/header.c
+++ b/lib/header.c
@@ -255,6 +255,8 @@ static rpmRC hdrblobVerifyInfo(hdrblob blob, char **emsg)
        if (end > info.offset)
            goto err;
 
+       if (info.tag < HEADER_I18NTABLE)
+           goto err;
        if (hdrchkType(info.type))
            goto err;
        if (hdrchkAlign(info.type, info.offset))

Hysterically there are no checks whatsoever on the tag values in rpm. In this particular case it's an out-of-place immutable region tag which causes assumptions in the code fail, in #135, #136, #138 and #139 it's a required tag replaced with a negative value. So catching stuff below the normal tag range will minimally cover all these. There are other layers present too, like missing sanity checks on tag types all over the place.

Also it's perhaps worth pointing out that none of the packages in the series crash nor pass through 'rpm -K' verification.

@hannob

This comment has been minimized.

Show comment
Hide comment
@hannob

hannob Feb 2, 2017

Also it's perhaps worth pointing out that none of the packages in the series crash nor pass through 'rpm -K' verification.

Maybe a bit offtopic here, but I noted that the "-K" parameter no longer works in the current git code. Is this intentional? (and if yes: why?) Because I specifically wanted to test this and look for pre-signature-verification-bugs, but I couldn't.

hannob commented Feb 2, 2017

Also it's perhaps worth pointing out that none of the packages in the series crash nor pass through 'rpm -K' verification.

Maybe a bit offtopic here, but I noted that the "-K" parameter no longer works in the current git code. Is this intentional? (and if yes: why?) Because I specifically wanted to test this and look for pre-signature-verification-bugs, but I couldn't.

@pmatilai

This comment has been minimized.

Show comment
Hide comment
@pmatilai

pmatilai Feb 2, 2017

Contributor

-K aka --checksig has been just a popt alias to rpmkeys for quite some time now, and popt aliases don't work so great from a git checkout. Try using ./rpmkeys -K instead.

Note that what that codepath does is vastly different from what happens during eg rpm -U or rpm -q which have their own, differently flawed signature checking.

Contributor

pmatilai commented Feb 2, 2017

-K aka --checksig has been just a popt alias to rpmkeys for quite some time now, and popt aliases don't work so great from a git checkout. Try using ./rpmkeys -K instead.

Note that what that codepath does is vastly different from what happens during eg rpm -U or rpm -q which have their own, differently flawed signature checking.

pmatilai added a commit that referenced this issue Feb 3, 2017

Sanity check header tag values. Like, doh.
There's a check for total number of tags, and their types and all
but absolutely no check for the actual tag numbers. So we end up
accepting negative tags which should not exist. The tag type should
really be uint32_t but that's another can of worms, lets have something
easily backportable for now.

This is enough to fix issues #133, #135, #136, #138 and #139 on the
level of detecting header structural inconsistency.
@pmatilai

This comment has been minimized.

Show comment
Hide comment
@pmatilai

pmatilai Feb 7, 2017

Contributor

Fixed in commit 3a07ba3. This one needs backporting to older versions so keeping open for now.

Contributor

pmatilai commented Feb 7, 2017

Fixed in commit 3a07ba3. This one needs backporting to older versions so keeping open for now.

pmatilai added a commit that referenced this issue Feb 16, 2017

Sanity check header tag values. Like, doh.
There's a check for total number of tags, and their types and all
but absolutely no check for the actual tag numbers. So we end up
accepting negative tags which should not exist. The tag type should
really be uint32_t but that's another can of worms, lets have something
easily backportable for now.

This is enough to fix issues #133, #135, #136, #138 and #139 on the
level of detecting header structural inconsistency.

Backported from commit 3a07ba3:
headerVerifyInfo() is so different in git master we can't use the
same exact thing here. Instead we do things in two steps,
headerVerifyInfo() catches totally garbage values and duplicate
regions are caught in regionSwab().
@pmatilai

This comment has been minimized.

Show comment
Hide comment
@pmatilai

pmatilai Feb 17, 2017

Contributor

Rpm 4.13.0.1 released with the fix, closing.

Contributor

pmatilai commented Feb 17, 2017

Rpm 4.13.0.1 released with the fix, closing.

@pmatilai pmatilai closed this Feb 17, 2017

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