[MIR] libfcgi, ceph (radosgw)

Status changed to 'Confirmed' because the bug affects multiple users.

Note that I want to raise additional concerns about libfcgi's upstream. Their official mailing list[1] bounces, and the archives' last message is from May of 2011 saying "ok new mailing list is setup and working".

I think that contact with the authors must be made before we can make any decisions about this. The library is of a high quality and the implementation seems sound, so perhaps we just need upstream to fix the mailing list and assert that they are there and ready to respond to any bugs.

[1] http://mailman.chelsea.net/mailman/listinfo/fastcgi-developers

I agree with Clint, please feel free to grab more information from upstream and reassign it back to me once you will have more information about their involvement in maintaining it.

I managed to get in contact with the upstream mailing list maintainer through one of the original project founders.

The mailing list is now functional again:

  http://mailman.fastcgi.com/mailman/listinfo/fastcgi-developers

Project is still active; things got broken by DNS changes....

Thanks for the precision :)

Ok, the packaging mostly seems good, I would though require that the .la files are either removed or emptied to not create additional issues for people installed the -dev package:
E: libfcgi-dev: non-empty-dependency_libs-in-la-file usr/lib/libfcgi++.la
E: libfcgi-dev: non-empty-dependency_libs-in-la-file usr/lib/libfcgi.la

In addition to that as I'm quite unconfortable with all cgi code, I would like the security team do a quick code review.

Once both are done and OK, +1 from me (reassign me so that I promote the package)

Recommendation from Didier included in latest upload to quantal:

libfcgi (2.4.0-8.1ubuntu2) quantal; urgency=low

  * d/rules,control: Remove dependency_libs from .la files inline with
    recommendation from Ubuntu MIR review, specify debhelper (>= 7.0.50~)
    for override usage.
  * d/libfcgi0ldbl.manpages,rules: Don't install manpage using rules file.
  * d/libfcgi0ldbl.docs: Rationalized to prevent duplicate install of docs.
  * d/dirs: Dropped - not actually required and generates linitan warning.
  * d/control,rules: Drop use of quilt dh addon as this is provided by
    debhelper.
  * d/control: Bumped Standards-Version to 3.9.3:
    - d/copyright: Converted to machine readable format.

Also had a general tidy of the package as it was a little confused in some areas (all changes submitted back to Debian).

Closing the ceph task. It is in main already and the radosgw package will stay in universe.

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

Thanks for the review Jamie.

I think that it is still worth enabling this feature for ceph even if it is in Universe; I'm concerned that as soon as a user wants to try out/use the RADO gateway they are forced to either cut their own package or revert to using upstream packages (which although in itself is not bad it drives users away from the distro for supported components).

Also in time this may become more of a strategic feature as/when integration with keystone is delivered and the RADOS gateway can be integrated more tightly into Openstack.

I've review/resolved most of the compiler warnings and submitted back upstream; the only two I have left are the two in example/threaded.c - this is only an example and AFAICT the casting int <-> void* should be OK as it relates to both ends of the same pthread_create call and associated function.

libfcgi (2.4.0-8.1ubuntu3) quantal; urgency=low

  * d/patches/re-libtoolize: Updated acinclude.m4 to correctly detect
    features in <sys/socket.h> if header is present, regenerated tooling.
  * d/patches/fix_compiler_warnings.patch: Fixup misc complier warnings
    in the codebase.

Date: Mon, 13 Aug 2012 12:30:49 +0100
Changed-By: James Page <email address hidden>
Maintainer: Ubuntu Developers <email address hidden>

MIR team, given jdstrand's conditions for ACK have been addressed, can we go ahead and seed libfcgi?

After discussing with yehudasa and Sage on #ceph (openprojects), I think there is a desire to have radosgw in main. It is strategic from the standpoint of being a backend for glance already and a good stand alone S3 service even without keystone support.

So, now that libfcgi seems ready for main, I think we should also re-evaluate the latest radosgw code and consider promoting it to main for quantal.

A reluctant ACK to libfcgi.

Regarding radosgw, did they rewrite it? The lack of defensive programming coupled with processing network traffic and the large amount of code made me very uneasy, so much so I made it a condition of the MIR to compile with --without-radosgw.

We fixed all the issues that were pointed out, and it was already ready for precise. At the time it was communicated that the main issue for not including radosgw was libfcgi, however, that doesn't appear to be an issue any more.
We've also created a test suite for radosgw (github.com/ceph/s3-tests.git) that beyond functionality tests it also does fuzzing.

FYI, I've uploaded a new version of ceph to quantal, 0.48-1ubuntu4, which builds radosgw and rest-bench and depends on libfcgi. It would appear that at least for the near-term, radosgw will remain in universe.

Yehuda, the comments I see from Sage indicate that non-radosgw malloc sites were addressed:

https://bugs.launchpad.net/ubuntu/+source/ceph/+bug/932898/comments/9

So perhaps there's more that can be looked at?

At the time we went through the entire code base. It didn't make sense going just through the ceph code and not through the radosgw code, they reside on the same tree and share code. Also, the radosgw code size is not as big as it was implied at the time.
In any case, we audited that code again, and it mostly looks ok (other than a single issue of not checking realloc, which was probably missed at the first time). A trivial fix for that is ready. All the other issues have been fixed.
There are a few isolated cases of strcpy/sprintf that are being used. We went through all of them and verified that they are safe (size of source is known, destination has enough space allocated). We can change these, they're pretty trivial. though we're hesitant to add unnecessary changes, we don't want to break anything by mistake.

All in all there are really a few affected call sites, and as I said, the code in question is really not that big.

Sounds like it is worth a second look at radosgw then, thanks for the clarification.

MIR team, would appreciate a second review of the contents of the radosgw binary package to see if it can be promoted to main as well. Thanks!

Assigning back to Didier then.

Moved libfcgi to main.

The other fly in the ointment for radosgw is that to actually use it you need to use apache2 with libapache2-mod-fastcgi (which is in multiverse).

This configuration is not currently automated in the packaging (I picked this from the upstream docs) so the packages don't express this in their dependencies.

Note that while libapache2-mod-fastcgi is currently the better way to go, there's also libapache2-mod-fcgid, or nginx/lighttpd with fastcgi.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 28/08/12 23:44, Yehuda Sadeh wrote:
> Note that while libapache2-mod-fastcgi is currently the better way
> to go, there's also libapache2-mod-fcgid, or nginx/lighttpd with
> fastcgi.

Thanks for the pointers Yehuda; I had a go with libapache2-mod-fcgid
but was not able to get it working using the instructions here:

http://ceph.com/wiki/RADOS_Gateway#Using_fcgid_module

I hacked around with the options a bit but with no success; I kept
getting:

[Wed Aug 29 09:11:25 2012] [notice] Apache/2.2.22 (Ubuntu)
mod_fcgid/2.3.7 configured -- resuming normal operations
radosgw: must specify 'rgw socket path' to run as a daemon
[Wed Aug 29 09:11:33 2012] [warn] [client 192.168.1.50]
(104)Connection reset by peer: mod_fcgid: error reading data from
FastCGI server
[Wed Aug 29 09:11:33 2012] [error] [client 192.168.1.50] Premature end
of script headers: s3gw.fcgi

When "rgw socket path" was specified in /etc/ceph/ceph.conf and

[Wed Aug 29 09:55:48 2012] [notice] Apache/2.2.22 (Ubuntu)
mod_fcgid/2.3.7 configured -- resuming normal operations
radosgw daemon started with pid 1816
[Wed Aug 29 09:56:03 2012] [warn] [client 192.168.1.50]
(104)Connection reset by peer: mod_fcgid: error reading data from
FastCGI server
[Wed Aug 29 09:56:03 2012] [error] [client 192.168.1.50] Premature end
of script headers: s3gw.fcgi

When I dropped this option and passed it directly as a parameter in
the s3gw.fcgi wrapper using the --rgw-socket-path option.

- --
James Page
Ubuntu Core Developer
Debian Maintainer
<email address hidden>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBCAAGBQJQPduPAAoJEL/srsug59jDVRoQAI48c5Il6Clk7NIxVAh1O22G
WMuAzR6+mLaYq7jwh1lwRKlbCSnEHPTewgEbKMX8vjejzA8YvkvUYne+9GqFYemw
DUQDyVHOUuaItwQBA59XJpRBiY4xUu2XOffwcDk73tKjBUb28cxwWM3S14vlnQ3m
I5aK0TDLgEFF4g1L5tAjtYc2JOPx/0P2Z2FH5IGZBShegtOK0Dmbh3LQ//ui+7qm
1H68NqIvSrXoYDduYezTBJ3Mq/Eel8E4xyw6gnmjPbX9IdbX2k4Jam1HwcxG+Ed8
HukyCAiw7ZjxoATXWaidKBksCVqlIagLic560c3fvgVK/qcgghzicxKo3jSSlVxn
hTAAST6wUkIPe/JYI/2zrS4Eo9Exc2bqGJ/xyZSTqm5nfYZ0kgmW80T0BqE8fdV7
80ayU0F4ZTzM/n2+1vVIJ2TiYwoV4t4/33HNT4BsHQVhTuJhTjUT04zTdh8oIUL2
iXNxsAhUjM39c0f2rVp59etSQho8pu9MiEPyMkJ+50a/fQ/4//UHOSw7M1jJLff9
IYtla7Ihmqqw5VzIMN/eBCX+I60wjT0+lPdHZ3vLcEWoIaGVP3i18C06IR5AYMJm
0inKX4BbaFj+5kzVu2qGkGPbpY+3ogHrWFD8Y/l/wF0ovZmQ7cE0JE+1he0PAuzz
XF0qHoexMEYi2gEA0+//
=V68W
-----END PGP SIGNATURE-----

Uh - radosgw is now showing up as located in main:

Filename: pool/main/c/ceph/radosgw_0.48.1-0ubuntu1_amd64.deb

I'm pretty sure this is not correct AFAICT the MIR review has not been completed yet.

I'm guessing because the source is in main, the NEW binaries defaulted to main.

An AA will need to manually move it if it fails to be approved for main.

I queried this in #ubuntu-release; it was a mistake and radosgw is now back in universe.

ceph depends on xml2, which is missing a MIR

Please complete the xml2 content so that I can review ceph (and why we need another xml library in main)

<doko> jamespage, I assume I'll just have to add rest-bench-dbg to Extra-Excludes
<jamespage> doko, yes please - I really don't think the rest-bench packages need to go into main

Ok, it mostly looks good here. Here are a small feedback on the packaging itself:
I don't understand the manual ldconfig call in librados2.post{inst,rm}. It should be added automatically isn't it (didn't see anything in changelog or README explaining it)? then, that will enable us to remove the 2 files as the rest is just DEBHELPER call.

I noticed that the same happens for rbd1 and cephfs1 - looking at these as well.

Hmm:

    dh_makeshlibs -n # we do the postinst/postrm scripts manually

I don't really understand why - I'll endeavor to find out.

https://github.com/ceph/ceph/commit/fd8ba5d5f21e314f0e1ed5c02cf04377b2608498 explains things a bit more - looking for a better way todo this.

This bug was fixed in the package ceph - 0.48.1-0ubuntu2

---------------
ceph (0.48.1-0ubuntu2) quantal; urgency=low

  * Remove manual calls to ldconfig (LP: #1017978):
    - d/lib{rados2|rbd1|cephfs1}.post*: Dropped - all these do is call
      ldconfig which will automatically be done.
    - d/rules: Let dh_makeshlibs do its magic with postinst/postrm
      scripts but ensure that the .so's in ceph package are excluded.
 -- James Page <email address hidden> Wed, 12 Sep 2012 10:36:57 +0100

Didier

I've updated the Ubuntu packaging do deal with generating postinst/postrm scripts for all shared libraries a little more elegantly.

Changes also submitted back upstream (the master source of packaging) - https://github.com/ceph/ceph/pull/23

@James: yeah, I looked at the upload :) Thanks for the quick answer and fix!

radosgw 0.48.1-0ubuntu2 in quantal amd64: universe/admin/optional -> main
radosgw 0.48.1-0ubuntu1 in quantal armel: universe/admin/optional -> main
radosgw 0.48.1-0ubuntu1 in quantal armhf: universe/admin/optional -> main
radosgw 0.48.1-0ubuntu2 in quantal i386: universe/admin/optional -> main
radosgw 0.48.1-0ubuntu1 in quantal powerpc: universe/admin/optional -> main
python-ceph 0.48.1-0ubuntu2 in quantal amd64: universe/python/optional -> main
python-ceph 0.48.1-0ubuntu1 in quantal armel: universe/python/optional -> main
python-ceph 0.48.1-0ubuntu1 in quantal armhf: universe/python/optional -> main
python-ceph 0.48.1-0ubuntu2 in quantal i386: universe/python/optional -> main
python-ceph 0.48.1-0ubuntu1 in quantal powerpc: universe/python/optional -> main
Override [y|N]? y

This was approved, but there was still a TODO item for me to review the updates to the code, which I haven't done yet. Based on upstream's comments, I'm guessing this is probably ok, so let's leave it where it is for now and if there is a problem I will comment.

This has a comeback in 2021
We see in component mismatches libfcgi-perl -> libfcgi

libfcgi-perl is in main since >=Xenial.
The new version 0.79+ds-2 now depends on libfcgi.

libfcgi itself was in main already based on this MIR bug here - main in Trusty/Xenial.

It was subscribed by the openstack team as ceph was (back then) the reason to bring it in.
The bug influx on this is very low, it has no Delta and also did not get "worse" since its promotion.

The full chain of the new reason to show up in mismatches is from
mysql-server-8.0 -> libhtml-template-perl -> libcgi-pm-perl -> libcgi-fast-perl -> libfcgi-perl -> libfcgi

That is for a 2008 mysql-8.0 feature:
* Add recommendation on libhtml-template-perl to -server package, used by
  ndb_size. (closes: #462265)

And the new bit in this chain is in libfcgi-perl
In the new build 0.79+ds-2 that changed

https://launchpad.net/ubuntu/+source/libfcgi-perl/0.79-1build1
Depends: perl (>= 5.32.0-4), perlapi-5.32.0, libc6 (>= 2.28)

https://launchpad.net/ubuntu/+source/libfcgi-perl/0.79+ds-2
Depends: perl, perlapi-5.32.0, libc6 (>= 2.4), libfcgi0ldbl (>= 2.4.2)

This is due to
0.79+ds-1
 21 [ gregor herrmann ]
 22 * Add repacking framework to get rid of embedded libfcgi files.
 23 (Closes: #971368)

And that means we already had those bits&bytes in main - just in an embedded form.
So having it in main "properly" now seems right.

I'll subscribe the server Team for >=Hirsute and Openstack can stay subscribed for T+X.

We have an approved MIR, we have a dependency that pulls it in, and we have realized that the code already was in main recently as well.

I'll mark the task (Fix Committed) to reflect this can be promoted (again) and assign ubuntu-archive to it.

$ ./change-override -c main -S libfcgi
Override component to main
libfcgi 2.4.2-2 in hirsute: universe/libs -> main
libfcgi-bin 2.4.2-2 in hirsute amd64: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute arm64: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute armhf: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute i386: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute ppc64el: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute riscv64: universe/utils/optional/100% -> main
libfcgi-bin 2.4.2-2 in hirsute s390x: universe/utils/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute amd64: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute arm64: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute armhf: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute i386: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute ppc64el: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute riscv64: universe/libdevel/optional/100% -> main
libfcgi-dev 2.4.2-2 in hirsute s390x: universe/libdevel/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute amd64: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute arm64: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute armhf: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute i386: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute ppc64el: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute riscv64: universe/libs/optional/100% -> main
libfcgi0ldbl 2.4.2-2 in hirsute s390x: universe/libs/optional/100% -> main
Override [y|N]? y
22 publications overridden.