malformed rar crashes bsdtar #504

Open
kwrobot opened this Issue Apr 11, 2015 · 6 comments

Comments

Projects
None yet
4 participants
@kwrobot

kwrobot commented Apr 11, 2015

Original issue 396 created by Google Code user hanno@hboeck.de on 2015-02-03T03:44:10.000Z:

<b>What steps will reproduce the problem?</b>
1. run bsdtar -xf crash.rar
2. segfault

<b>What version are you using?</b>
git head (e6c9668f3202215ddb71617b41c19b6f05acf008)

<b>On what operating system?</b>
Linux

<b>How did you build?  (cmake, configure, or pre-packaged binary)</b>
cmake

<b>What compiler or development environment (please include version)?</b>
gcc 4.9.2

<b>Please provide any additional information below.</b>

Crash with malformed rar file, found with american fuzzy lop. I'll attach valgrind and address sanitizer output. Looks like a null ptr.

See attachment: crash.rar
See attachment: crash.rar.asan.txt
See attachment: crash.rar.valgrind.txt

@kwrobot

This comment has been minimized.

Show comment
Hide comment
@kwrobot

kwrobot Apr 11, 2015

Comment #1 originally posted by Google Code user tim@kientzle.com on 2015-02-07T06:33:13.000Z:

This is hitting a corner case in the RAR reader:  The reader decides the current header is a "split file in multivolume RAR" and returns a success code without actually filling in the entry.  I'm not sure whether this code (around lines 1540-1560) should return an error or should continue to read the next header.

kwrobot commented Apr 11, 2015

Comment #1 originally posted by Google Code user tim@kientzle.com on 2015-02-07T06:33:13.000Z:

This is hitting a corner case in the RAR reader:  The reader decides the current header is a "split file in multivolume RAR" and returns a success code without actually filling in the entry.  I'm not sure whether this code (around lines 1540-1560) should return an error or should continue to read the next header.

@kwrobot

This comment has been minimized.

Show comment
Hide comment
@kwrobot

kwrobot Apr 11, 2015

Comment #2 originally posted by kientzle on 2015-02-07T07:29:13.000Z:

I've committed a change to bsdtar so it will skip entries for which the format handler is unable to parse a filename.

This makes bsdtar itself resistant to this issue, but it would be better to fix the underlying parsing issue in the RAR reader.

kwrobot commented Apr 11, 2015

Comment #2 originally posted by kientzle on 2015-02-07T07:29:13.000Z:

I've committed a change to bsdtar so it will skip entries for which the format handler is unable to parse a filename.

This makes bsdtar itself resistant to this issue, but it would be better to fix the underlying parsing issue in the RAR reader.

@kientzle kientzle modified the milestone: 3.2 Aug 1, 2015

@dosomder

This comment has been minimized.

Show comment
Hide comment
@dosomder

dosomder Mar 19, 2016

Contributor

The rar reader saves the filename of the entry and when the next entry has the same filename, it assumes this is a multivolume archive. Did I understand this correctly? https://github.com/libarchive/libarchive/blob/master/libarchive/archive_read_support_format_rar.c#L1546

In my opinion the filename only needs to be saved if the flag FHD_SPLIT_AFTER is set. If the next entry then has the flag FHD_SPLIT_BEFORE and the same filename, it's a multivolume. See also here: http://www.forensicswiki.org/wiki/RAR

The entries in crash.rar don't have these flags set.

Contributor

dosomder commented Mar 19, 2016

The rar reader saves the filename of the entry and when the next entry has the same filename, it assumes this is a multivolume archive. Did I understand this correctly? https://github.com/libarchive/libarchive/blob/master/libarchive/archive_read_support_format_rar.c#L1546

In my opinion the filename only needs to be saved if the flag FHD_SPLIT_AFTER is set. If the next entry then has the flag FHD_SPLIT_BEFORE and the same filename, it's a multivolume. See also here: http://www.forensicswiki.org/wiki/RAR

The entries in crash.rar don't have these flags set.

@kientzle

This comment has been minimized.

Show comment
Hide comment
@kientzle

kientzle Mar 19, 2016

Contributor

Ah. That might explain it. A pull request would be greatly appreciated.

Contributor

kientzle commented Mar 19, 2016

Ah. That might explain it. A pull request would be greatly appreciated.

@kientzle

This comment has been minimized.

Show comment
Hide comment
@kientzle

kientzle Apr 3, 2016

Contributor

I believe the current fix is good enough for 3.2. I'll defer further work to 3.2.1.

Contributor

kientzle commented Apr 3, 2016

I believe the current fix is good enough for 3.2. I'll defer further work to 3.2.1.

@kientzle kientzle modified the milestones: 3.2.1, 3.2 Apr 3, 2016

@kientzle kientzle modified the milestones: 3.3, 3.2.1 Jun 20, 2016

@petterreinholdtsen

This comment has been minimized.

Show comment
Hide comment
@petterreinholdtsen

petterreinholdtsen Jul 8, 2016

According to https://security-tracker.debian.org/tracker/CVE-2015-8916 this is a security issue with ID CVE-2015-8916. I tested, and the crash happen with version 3.1.2 too.

According to https://security-tracker.debian.org/tracker/CVE-2015-8916 this is a security issue with ID CVE-2015-8916. I tested, and the crash happen with version 3.1.2 too.

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