Bug 551415 - dereferencing invalid memory area
Summary: dereferencing invalid memory area
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: libtar
Version: 12
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Huzaifa S. Sidhpurwala
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 597154
TreeView+ depends on / blocked
 
Reported: 2009-12-30 15:35 UTC by Sven Herzberg
Modified: 2010-05-28 09:19 UTC (History)
1 user (show)

Fixed In Version: 1.2.11-16.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 597154 (view as bug list)
Environment:
Last Closed: 2010-01-04 21:16:25 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
testcase: libtar-main.c (2.16 KB, text/x-csrc)
2009-12-30 15:35 UTC, Sven Herzberg
no flags Details
proposed patch v1 (840 bytes, patch)
2009-12-30 16:42 UTC, Sven Herzberg
herzi: review?
Details | Diff

Description Sven Herzberg 2009-12-30 15:35:49 UTC
Created attachment 380957 [details]
testcase: libtar-main.c

Description of problem:
When executing a testcase for a toy app, I started to write, valgrind observed invalid memory access.

Version-Release number of selected component (if applicable):
$ rpm -q libtar{,-devel,-debuginfo} valgrind{,-devel}
libtar-1.2.11-15.fc12.i686
libtar-devel-1.2.11-15.fc12.i686
libtar-debuginfo-1.2.11-15.fc12.i686
valgrind-3.5.0-9.i686
valgrind-devel-3.5.0-9.i686

How reproducible:
Compile and valgrind the test case.

Steps to Reproduce:
1. gcc -o libtar-test libtar-main.c -ltar
2. valgrind --exit-errorcode=1 -q ./libtar-test
  
Actual results:
FAIL FOLLOWS:
==9338== Conditional jump or move depends on uninitialised value(s)
==9338==    at 0xBC3683: th_set_path (encode.c:85)
==9338==    by 0x8048890: main (libtar-main.c:60)
==9338== 

Expected results:
Silence and a happy valgrind.

Additional info:
This issue happens because of the TH_ISDIR() macro used in th_set_path(). It is defined like this:
> #define TH_ISDIR(t)     ((t)->th_buf.typeflag == DIRTYPE \
>                          || S_ISDIR((mode_t)oct_to_int((t)->th_buf.mode)) \
>                          || ((t)->th_buf.typeflag == AREGTYPE \
>                              && ((t)->th_buf.name[strlen((t)->th_buf.name) - 1] == '/')))

Now when we use an empty-initialized header, the name is an array of 100 '\0' bytes => strlen((t)->th_buf.name) == 0 => strlen((t)->th_buf.name) - 1 == -1 => (t)->th_buf.name[-1] == <last byte before the tar header>.

I'm not sure this behavior is an exploitable security issue. It's at least a major defect in that library preventing it from being used in properly valgrinded test cases.

Will provide a patch.

Comment 1 Sven Herzberg 2009-12-30 16:42:46 UTC
Created attachment 380965 [details]
proposed patch v1

This patch fixes TH_ISDIR() and oct_to_int() to properly handle empty strings for "name" and "mode" respectively.

Comment 2 Huzaifa S. Sidhpurwala 2009-12-31 04:11:04 UTC
https://admin.fedoraproject.org/updates/libtar-1.2.11-16.fc12 fixes this issue.

Comment 3 Fedora Update System 2010-01-02 03:29:21 UTC
libtar-1.2.11-16.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update libtar'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2010-0010

Comment 4 Fedora Update System 2010-01-04 21:16:21 UTC
libtar-1.2.11-16.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.