[Bug 1017978] Re: [MIR] libfcgi, ceph (radosgw)

[Jamie Strandboge]

I welcome the day when a build-depends for a universe binary won't
require the build-dep to be in main. But that day is not today...

Security review:
No CVE history. It is compiled with hardening options. No initscripts/upstart jobs, dbus services, setuid, fscaps usage, sudo/su/pkexec usage or cron jobs.

There are compiler warnings:
fcgiapp.c:1490:13: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
fcgiapp.c:1495:9: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
fcgiapp.c:1498:9: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
os_unix.c:1175:54: warning: pointer targets in passing argument 3 of 'accept' differ in signedness [-Wpointer-sign]
os_unix.c:1275:35: warning: pointer targets in passing argument 3 of 'getpeername' differ in signedness [-Wpointer-sign]
cgi-fcgi.c:390:13: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
cgi-fcgi.c:461:5: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
cgi-fcgi.c:477:4: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
cgi-fcgi.c:643:9: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' [-Wformat]
cgi-fcgi.c:725:9: warning: variable 'numFDs' set but not used [-Wunused-but-set-variable]
threaded.c:27:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
threaded.c:80:44: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

A shallow code audit reveals that this is using old C coding styles using low level C routines rather than higher level libraries that are safe. That said, old code does not necessarily mean bad code and in this case it means mature, working code that is in production.
 * malloc: Malloc() is implemented and it does proper error checking and aborts on error. malloc() is also used here and there, and return codes checked and exited.
 * strcpy: there are a lot of uses of strcpy without checks. Ones in libfcgi/os_unix.c and cgi-fcgi.c look to not be attacker controlled and are also covered by stack-protector. There is also a StringCopy() that is safe.
 * memcpy: spot checked: seemed generally ok
 * sprintf: fcgiapp.c:FCGX_VFPrintF() has a lot of sprintf usage and while there are efforts to ensure the allocated buffer is correct, it is a very complicated loop with many branches. I am not saying the code is wrong, but I am saying I don't have high confidence there aren't any bugs in this section. Thankfully, it looks like if there is a bug, it will happen on the stack and stack-protector would provide protections here.
 * getenv: not seeing any input validation here, but the code seems ok.

My preference to keep this out of main because it doesn't seem strategic
itself and the application needs it as a build-dep is also not strategic
and resides in universe (and this is not likely to change (see ceph
MIR)).

If this is critically important to the server team (I'll let them
decide), then conditional ACK on all compiler warnings are addressed.

** Changed in: libfcgi (Ubuntu)
       Status: Triaged => In Progress

** Changed in: libfcgi (Ubuntu)
     Assignee: Jamie Strandboge (jdstrand) => James Page (james-page)

-- 
You received this bug notification because you are a member of Ubuntu
Server Team, which is subscribed to libfcgi in Ubuntu.
https://bugs.launchpad.net/bugs/1017978

Title:
  [MIR] libfcgi, ceph (radosgw)

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/ceph/+bug/1017978/+subscriptions