Review Rules and staticly linked packages agains dietlibc

[Hans de Goede]

Enrico Scholz wrote:
> j.w.r.degoede at hhs.nl (Hans de Goede) writes:
> 
>>>> Why? Because static linking is BAD for lots of reasons,
>>> Please tell me, why static linking is BAD in *this* case.
>> I just did a quick grep for your email in owners.list and I'm amazed by
>> the fact that someone who maintains as many security related packages
>> as you needs to ask.
> 
> Perhaps because I understand the reasons behind dynamic linking instead
> of repeating things blindly without knowing why they are there?
> 
> Ok, let's look at 'ipsvd'; when linking the 'tcpsvd' program dynamically
> (a '--without dietlibc' rpm switch should do it), we create a binary
> which imports
> 
> 
> | __errno_location
> | __fxstat
> | __gmon_start__
> | __libc_start_main
> | __xstat
> | _exit
> | accept
> | bind
> | close
> | connect
> | execve
> | fcntl
> | fork
> | free
> | getgrnam
> | gethostname
> | getpeername
> | getpid
> | getppid
> | getpwnam
> | getservbyname
> | getsockname
> | gettimeofday
> | listen
> | lseek
> | malloc
> | mmap
> | munmap
> | open
> | poll
> | read
> | recv
> | send
> | setgid
> | setgroups
> | setsockopt
> | setuid
> | sigaction
> | sigaddset
> | sigemptyset
> | sigprocmask
> | sigsuspend
> | socket
> | time
> | unlink
> | unlink
> | waitpid
> | write
> 
> [The static linked dietlibc version will not use more of these functions.]
> 
> '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 and that seems to be 
one of the 2 fundumantel parts of our difference of opinion.


> 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 :)

> 
> 
> 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.

> 
>> Even in glibc which is widely used and audited security holes turn up
>> quite regular, so this will most probably happen for dietlibc atleast
>> as often as for glibc. When this happens we don't want to have to track
>> which packages all are staticly linked against it. With the SSL stuff
>> in ipvsd chances for holes are even bigger, so I would not only like to
>> argue that ipvsd should not staticly link against dietlibc,
> 
> 1. package can not be shipped with 'matrixssl' due to licensing issues;
>    therefore, 'matrixssl' has nothing todo with the *current* issue.
> 

Yes I saw that after my mail, sorry thats because the flag is called 
with-iossl instead of with-matrixssl. So I thought these were 2 
different things.

> 2. 'ipvsd' supports only static linking of 'matrixssl'
> 

Which would be unacceptable for something as security sensitive as an 
ssl lib.

> 
> 
>>>> 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 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.

> 
>> 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 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! But for a few packages its 
ridiculous.

> 
>>>> 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".
>>>
>> Its staticly linked, this it gets its own private copy hardcoded into
>> the binary.
> 
> "local copy" in this sentence means a library which is both shipped in
> the package and exists in the system already. 'matrixssl' might be an
> example in this case when it would exist in FE already.
> 

See above, the intention of this rule is imho clear and it extends to 
what you're doing.

> 
>>>> 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.

> 
>>> 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, since some maintainers are not 
all that responsive to bugreports, this will be a pain, who is going to 
coordinate this and make sure all affected packages get rebuild.

This is exactly why we don't want static libs and if we don't have them 
we can't use them because we don;t want static linking either (it 
_really_ is the same)




> 
>> So saying that there are only advantages is false as there are clear
>> disadvantages.
> 
> Please tell me the disadvantages in *this* case.
> 

See above.

> 
>> But this entire discussion is mood anyways. We have clear guidelines and
>> you are in clear (and unnescesarry) violation of the guidelines.
> 
> Where does 'ipvsd' violate the guidelines?
> 

See above, and dietlibc clear violates them too.

Regards,

Hans