Review Rules and staticly linked packages agains dietlibc
Enrico Scholz
enrico.scholz at informatik.tu-chemnitz.de
Fri Feb 24 07:17:19 UTC 2006
j.w.r.degoede at hhs.nl (Hans de Goede) writes:
>> 'malloc' and 'free' are the only higher level functions, all other
>> functions are simple syscall wrappers and ARE implemented unexploitable
>> (the related code are perhaps 20 assembly lines). It is right that the
>> dietlibc 'malloc(3)' implementation suffered the known integer overflow
>> some time ago. But in the meantime, the related 162 lines of code in
>> dietlibc have been reviewed several times so it can be assumed as
>> error-free.
>>
>
> Thats one assumption which I would rather not make
Please, look at the code before doing such unreasoned statements. The
malloc() code is
| static void* REGPARM(1) __small_malloc(size_t _size) {
| __alloc_t *ptr;
| size_t size=_size;
| size_t idx;
|
| idx=get_index(size);
| ptr=__small_mem[idx];
|
| if (ptr==0) { /* no free blocks ? */
| register int i,nr;
| ptr=do_mmap(MEM_BLOCK_SIZE);
| if (ptr==MAP_FAILED) return MAP_FAILED;
|
| __small_mem[idx]=ptr;
|
| nr=__SMALL_NR(size)-1;
| for (i=0;i<nr;i++) {
| ptr->next=(((void*)ptr)+size);
| ptr=ptr->next;
| }
| ptr->next=0;
|
| ptr=__small_mem[idx];
| }
|
| /* get a free block */
| __small_mem[idx]=ptr->next;
| ptr->next=0;
|
| return ptr;
| }
|
| static void* _alloc_libc_malloc(size_t size) {
| __alloc_t* ptr;
| size_t need;
| #ifdef WANT_MALLOC_ZERO
| if (!size) return BLOCK_RET(zeromem);
| #else
| if (!size) goto err_out;
| #endif
| size+=sizeof(__alloc_t);
| if (size<sizeof(__alloc_t)) goto err_out;
| if (size<=__MAX_SMALL_SIZE) {
| need=GET_SIZE(size);
| ptr=__small_malloc(need);
| }
| else {
| need=PAGE_ALIGN(size);
| if (!need) ptr=MAP_FAILED; else ptr=do_mmap(need);
| }
| if (ptr==MAP_FAILED) goto err_out;
| ptr->size=need;
| return BLOCK_RET(ptr);
| err_out:
| (*__errno_location())=ENOMEM;
| return 0;
| }
|
| static void REGPARM(2) __small_free(void*_ptr,size_t _size) {
| __alloc_t* ptr=BLOCK_START(_ptr);
| size_t size=_size;
| size_t idx=get_index(size);
|
| memset(ptr,0,size); /* allways zero out small mem */
|
| ptr->next=__small_mem[idx];
| __small_mem[idx]=ptr;
| }
|
| static void _alloc_libc_free(void *ptr) {
| register size_t size;
| if (ptr) {
| size=((__alloc_t*)BLOCK_START(ptr))->size;
| if (size) {
| if (size<=__MAX_SMALL_SIZE)
| __small_free(ptr,size);
| else
| munmap(BLOCK_START(ptr),size);
| }
| }
| }
It is definitively not exploitable.
>> So the when-static-library-contains-flaw-we-have-to-rebuild-everything
>> argument does not hold because the "when-static-library-contains-flaw"
>> condition can never occur in *this* case.
>
> never say never :)
Correctness of a program can be proved mathematically. So I can say that
exploits in ipsvd are NEVER caused by dietlibc.
>> I admit that there are problems in dietlibc but none of them are
>> interesting in *this* case.
>
> Again jumping to conclusions rather quickly, you've clearly given this
> many thought and done your "homework" but there just is no such thing
> as bug free code, thats where we disagree.
bug-free code exists. The malloc() implementation and the named syscall
wrappers in dietlibc are examples.
>>>>> many the same reasons why the packaging guidelines state that
>>>>> packages should not compile and (staticly) link against their own
>>>>> version fo system libs,
>>>> The "should" in the packaging guidelines was intentionally. It leaves
>>>> room to link statically when this is the better choice and in this case,
>>>> dietlibc is the better choice.
>>>>
>>> Not when this is a better choice, it doesn't say when this is a better
>>> choice anywhere, it says "Static libraries should only be included in
>>> exceptional circumstances."
>> This sentence means packaging of %_libdir/*.a files but NOT linking
>> against static libraries.
>>
>
> If you want to take things literaty the dietlibc package does fall
> under thus rule
I do not see where. dietlibc makes sense with static libraries only. The
dynamic linking support is experimental and does not exist for all
platforms. So, there exist "exceptional circumstances" and there is
still the "should" which allows the packager to do the best thing (and
static dietlibc libraries are the best thing).
> and thus should be changed to only provide .so or removed since I see
> no reason for allowing an exeption for it. Don't you see that either
> way it is the same we don't want static libs because we don't want
> static linking learn to read between the lines.
These static-linking rules were not written to ban static libraries
completely but to avoid things like the static-zlib desaster. Therefore,
it is a "should" but not "must" rule.
>>> I guess I can come up with a zillion more small programs which will
>>> be smaller and faster with dietlibc, thats not what this is about,
>>> the should is there in case its impossible to avoid this without
>>> tons of work.
>> It really depends. It's hard to find a reason to link programs like
>> | int main() { write(1, "Hello world\n", 12); }
>> dynamically against glibc instead of using dietlibc. But I would not
>> write a bugreport only because of this inefficiency.
>
> So you agree the gain isn't big enough
No, Fedora Extras rules do neither forbid nor encourage usage of dietlibc
so it is finally the decision of the packager to do the best thing. When
he does not want to add dietlibc deps to his package, I have to accept
it. Arguing with effiency would result into endless discussions without a
result as this thread is showing.
> to warrent doing this for other packages, then it also isn't big
> enough for this package. And more in general the gain of dietlibc
> (for soem programs) isn't big enough to warrant it an exception to
> the no .a files rules, so dietlibc should be changed or removed. Now
> if we're talking about moving the entire distro over to dietlibc,
> that would be interesting!
I never wanted to move the entire distro to dietlibc, only few programs
benefit from this library. It is obviously that complex libraries whose
bugfreeness can not be proven mathematically should be linked dynamically.
> But for a few packages its ridiculous.
I prefer to chose things in case-per-case decision instead of making
generalisations which hold for most but not all packages.
"Static linking is insecure" is such a generalisation which does not
apply to the 'ipsvd' case.
>>>>> that is exactly what you're doing now linking against an own
>>>>> version of system libs.
>>>> ??? I do not see where 'ipvsd' links against a "local copy of a
>>>> library that exists on the system".
> ...
> See above, the intention of this rule is imho clear and it extends to
> what you're doing.
This rule means only that packages should not use local copies of
libraries (e.g. 'zlib', 'db4') but use the existing ones from the
system. There are no deaper intentions or extends of this rule.
>>>>> is the exception that confirms the rule. Also notice: "Static
>>>>> libraries should only be included in exceptional circumstances."
>>>> 'ipvsd' does not provide static libraries.
>>> Nor should it use them,
>> That is not stated there and was never an intention of this paragraph
>> which covers packages shipping libraries only. 'ipvsd' is not such a
>> package.
>
> Dietlibc is, so remove/fix that please.
NOTABUG. This rule is a "should" only and there exist "exceptional
circumstances".
>>>> when there are ways to make things work better, these ways should
>>>> be gone. Again: linking against 'dietlibc' has only advantages for
>>>> 'ipvsd'.
>>> When the tradeof is a small gain in speed and footprint versus
>>> maintainability and security then the disadvantages of your choice
>>> outway the advantages.
>> I do not see how this is related to *this* case and do not know
>> what you mean with "maintainablility". The "security" argument was
>> disproved above.
>
> maintainability means that if we allow this and a few other packages
> will get linked against dietlibc too and a bug in dietlibc is found
> then all those packages will need a rebuild,
You mean that you want to add a rule to the guidelines like
| A package MUST NOT be usable as an example for other unrelated packages
| which might do bad things
-1 from me...
I speak only about 'ipvsd' currently but not about potential other
packages maintained by other people.
Enrico
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 482 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-extras-list/attachments/20060224/309395fa/attachment.sig>
More information about the fedora-extras-list
mailing list