Incorporate a kludge to comply with FORTIFY_SOURCE

[GoogleCodeExporter]

Hello Michal,
please would you consider accepting following patch 
to your code? This patch tries to make memory allocation 
compliant with FORTIFY_SOURCE, MALLOC_CHECK and maybe some
other canary based checks for buffer owerflow? 

I believe that enabling FORTIFY_SOURCE can bring some additional security
to the project. 

Best regards
Michal Ambroz

Original issue reported on code.google.com by michal.a...@gmail.com on 23 Apr 2010 at 2:39

Attachments:

• skipfish-fortify.patch.gz

[GoogleCodeExporter]

Clever, although looks like a better patch would be to fix FORTIFY_SOURCE to 
have 
malloc_usable_size() not return more than is actually, well, usable by programs 
(i.e., 
aware of the canaries). Have you considered reaching out to them first?

Original comment by lcam...@gmail.com on 23 Apr 2010 at 2:53

• Changed title: Incorporate a kludge to comply with FORTIFY_SOURCE
• Added labels: Priority-Low, Type-Enhancement
• Removed labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

[ Specifically, I am concerned that there might be some malloc implementation 
where 
"usable" is always kept higher than requested size for speculative performance 
benefits; and that by reallocating there, we might be actually causing a 
performance 
impact rather than forcing alignment. ]

Original comment by lcam...@gmail.com on 23 Apr 2010 at 2:55

[GoogleCodeExporter]

Hello Michale,
> better patch would be to fix FORTIFY_SOURCE to have 
> malloc_usable_size() not return more than is actually, well,
> usable by programs

I believe both of you are using space between requested and allocated in 
honorable
way, but in undocumented manner and you are both using grey areas of the malloc
implementation. I believe both of you should move one step back and code in a 
way,
which is predictable to keep it secure.

I have found these related to malloc_usable_size:
Glibc code for malloc_usable_size (malloc/malloc.c) speaks in your favour, 
athough it
is directly discouraging such use of malloc_usable_size:

These are saying that malloc_usable_size is not replacement for realloc and you
should not rely on its results:
http://fuse4bsd.creo.hu/localcgi/man-cgi.cgi?malloc+3
http://www.linuxjournal.com/node/6390/print
http://sourceware.org/bugzilla/show_bug.cgi?id=1349
http://archives.seul.org/or/cvs/May-2009/msg00183.html

According this documentation it seems that malloc_usable_size doesn't report the
space you are allowed to use now in given chunk of memory, but it rather tells 
you
what is the maximum usable space in given chunk. 

You might have got this space if you would requested it, but you didn't and you 
use
it anyway (setting it to zeros). 

Patch calling realloc is only legalizing what you want to do. 

>Have you considered reaching out to them first?
Yes I did. 
Please have a look at the discussion in the Review Request for the Fedora 
package
https://bugzilla.redhat.com/show_bug.cgi?id=576431

Tomas Mraz has reached Jakub Jelinek (who maintains the FORTIFY_SOURCE) and 
asked him
for analysis. He claims it is bug in skipfish ;). 

Same way I am reporting issue to you, I am going to report the issue to glibc /
FORTIFY_SOURCE, but man ... you do not want me spent rest of my life contacting 
all
the maintainers of all the memory protections which are out there and based on
canaries won't you? From the top of my head I can name at least 4.

Best regards
Michal Ambroz

Original comment by michal.a...@gmail.com on 23 Apr 2010 at 4:31

[GoogleCodeExporter]

I would certainly hope that other maintainers don't conveniently forget about 
malloc_usable_size() having to be adjusted for any memory they decided to use 
for 
their purposes ;-)

The reason I'm using it this way is specifically because I want 
sanity-checking, 
memory-zeroing allocation and reallocation functions to *improve* security, and 
to 
catch memory management errors early on. This is available on FreeBSD through 
malloc_options[], but not so on Linux; there is also a more suitable function 
called 
malloc_size(), but again, it is not available on Linux.

I can't think of another way to have a memsetting realloc() without increasing 
extra 
overhead. Adding my own canaries and buffer size data is a possibility, but it 
complicates it a bit.

Original comment by lcam...@gmail.com on 23 Apr 2010 at 5:09

[GoogleCodeExporter]

Hello, 
Tomas Mraz has created more systematic patch which should be iso C compliant.
It should be more portable than using malloc_usable_size which is specific to 
only
glibc malloc.

Please could you consider adding this patch?

Best regards
Michal Ambroz

Original comment by michal.a...@gmail.com on 9 May 2010 at 12:34

Attachments:

• skipfish-fortify.patch

[GoogleCodeExporter]

Sorry for not getting back to you sooner; I sort of dislike this fix, because 
it adds a new layer where things can go wrong (i.e., specify the parameter 
incorrectly, and end up with memory corruption).

Another approach, suggested by Damien Miller, is to allocate (requested_size + 
sizeof(size_t)), return the pointer to buffer + sizeof(size+t), and use that 
leading region to store buffer size information for future allocations; a 
canary to prevent doing this for buffers not allocated by this code in the 
first place would be necessary. I will probably have a look at this instead?

Original comment by lcam...@gmail.com on 5 Jul 2010 at 9:19

[GoogleCodeExporter]

This is now implemented.

Original comment by lcam...@gmail.com on 5 Jul 2010 at 10:00

• Changed state: Fixed