Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heap buffer overflow in RAR reader #467

Closed
kwrobot opened this issue Apr 11, 2015 · 11 comments

Comments

@kwrobot
Copy link

@kwrobot kwrobot commented Apr 11, 2015

Original issue 359 created by Google Code user CheesePC on 2014-05-01T16:38:53.000Z:

<b>What steps will reproduce the problem?</b>
1. Create RAR archive with Nth file name shorter than (N-1)th
2. Build your own software using libarchive (I'm using callbacks-powered archiving decompression, should be reproducible in another modes though) with Clang++ Address Sanitizer (!)
3. Run it!

<b>What is the expected output? What do you see instead?</b>
Software is being expected to work. Address Sanitizer heap buffer overflow error instead.

<b>What version are you using?</b>
Current git master, 14.

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

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

<b>What compiler or development environment (please include version)?</b>
Clang 3.3

<b>Please provide any additional information below.</b>
I have looked around this issue and made a patch which fixes an issue for me. I'm attaching it here since I'm not good in all this github stuff. 

See attachment: 0001-Heap-buffer-overflow-fixed-at-rar-archive-handler.patch

@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #1 originally posted by kientzle on 2014-05-03T17:33:42.000Z:

Your change does not look quite right to me.  In particular, you are assuming that rar->filename and rar->filename_save are null-terminated, which does not seem to be the case.

Please try the attached:  It ensures that filename_save is in fact null terminated so strlen() will work for it and adjusts your patch to use filename_size instead of strlen(rar->filename).

See attachment: rar-not-fooled-by-prefix-filename.patch

@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #2 originally posted by Google Code user CheesePC on 2014-05-03T18:48:08.000Z:

Why aren't they null-terminated? rar->filename is null-terminated at filename_size at line 1419, rar->filename_save is null-terminated since it's copied from rar->filename including filename_size + 1, which keeps a null-terminator. 

So, I see only two possible solutions:
1) Make sure both strings are null-terminated and replace memcmp with strcmp, which will take care of their lengths differences;
2) Strings must not always be null-terminated by design, in which case we should keep rar->filename_save's length and compare both strings despite of their characters, as memory segments. 

In fact, I don't see any reasons to keep non-character data in those fields since filenames are supposed to be strings, not memory segments. There might be a glitch with some special encodings support in C, but it's up to maintainers to find it out.
@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #3 originally posted by kientzle on 2014-05-03T23:04:34.000Z:

"rar->filename is null-terminated at filename_size at line 1419..."

Ah. yes.  I had missed that.  The copious use of memcpy() made me think that the strings were not null-terminated.

We should use strcmp() and strcpy() where appropriate to make this clearer.

If you put together the patch, I'll commit it.


@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #4 originally posted by Google Code user CheesePC on 2014-05-04T05:48:35.000Z:

I'll take a look, maintainer ought to review it though, I'm not really an archiving software developer :) 
@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #5 originally posted by Google Code user CheesePC on 2014-05-04T09:43:47.000Z:

Here comes the patch.

See attachment: 0001-Heap-buffer-overflow-fixup-for-rar-archive-reader.patch

@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #6 originally posted by kientzle on 2014-05-04T16:47:17.000Z:

Thanks.  That's what I had in mind.  Unfortunately, it seems to break a couple of tests.

Please try running the test suite (make test) with and without your patch;  here I see two tests that verify rar unicode support fail with your patch but pass without.  Hopefully I'll have time this week to figure out why.



@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #7 originally posted by kientzle on 2014-05-04T17:44:56.000Z:

BTW, could you create a small RAR archive that triggers the problem you found?

I can fold that into a test case and make sure that -- once we fix this problem -- it never inadvertently reappears.

@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #8 originally posted by kientzle on 2014-05-05T03:06:04.000Z:

Please try this patch:  It does not break the existing tests and I think it fixes the issue you found (that comparing the current filename to the previous filename does not take into account the length of the two strings so will falsely trigger a match when the new filename is a prefix of the previous one).

I would still like to extend the tests to cover this case.

See attachment: libarchive-rar-preserve-saved-filename-size.patch

@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #9 originally posted by Google Code user CheesePC on 2014-05-05T12:52:15.000Z:

> BTW, could you create a small RAR archive that triggers the problem you found?
I can, but is heap buffer overflow catching possible with your testing framework? 

> Please try this patch. 
It will work, makes the same another way. But isn't there a convenient way to handle strings in C? I'm not really a C programmer so dropping standard string handling routines looks kinda weird for me.
@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #10 originally posted by Google Code user CheesePC on 2014-06-04T17:18:21.000Z:

So, is this going to be fixed soon?
@kwrobot

This comment has been minimized.

Copy link
Author

@kwrobot kwrobot commented Apr 11, 2015

Comment #11 originally posted by kientzle on 2014-06-05T00:20:51.000Z:

Thanks for the reminder; I've committed the patch to git master.

@kwrobot kwrobot closed this Apr 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.