Comment 28 for bug 1029549

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Review:
 * No CVEs,
 * Hardening options are present. Would be nice to enable PIE.
 * No initscript/upstart jobs, dbus system services, setuid, fscaps usage, sudo/su/pkexec or cron jobs. Has two session services.
 * does use malloc with strcpy, but these are safe. Spot-checking other operations seems fine and it seems to be generally well coded using typical C++ style.
 * This was an interesting find in the code:
./lib/signond/signond.pc:prefix=/home/mardy/tmp/signond
./lib/signond/signond.pc:libdir=/home/mardy/tmp/signond/lib64
 * no significant compiler warnings
 * lintian warnings:
libsignon-plugins-doc_8.41-0ubuntu3_all.deb:
W: libsignon-plugins-doc: embedded-javascript-library usr/share/doc/signon-plugins/html/jquery.js
libsignon-qt-doc_8.41-0ubuntu3_all.deb:
W: libsignon-qt-doc: embedded-javascript-library usr/share/doc/libsignon-qt/html/jquery.js
signond_8.41-0ubuntu3_amd64.deb:
W: signond: binary-without-manpage usr/bin/signonclient
W: signond: binary-without-manpage usr/bin/signond
W: signond: binary-without-manpage usr/bin/signonpluginprocess
signond-doc_8.41-0ubuntu3_all.deb:
W: signond-doc: embedded-javascript-library usr/share/doc/signon/html/jquery.js
signon-plugin-ssotest_8.41-0ubuntu3_amd64.deb:
E: signon-plugin-ssotest: description-synopsis-is-duplicated

signond is running in the same context as the user. This means that any application can request credentials. While I didn't look to see what kind of protections are in place for signond, it doesn't matter-- an attacker able to execute arbitrary code could execute a hacked signond in the same user context and access the data, etc. Since signond contains sensitive information, this should be moved to a separate user with its db in a protected directory or encryption (maemo mentions this: "Server stores credentials in optionally encrypted database. It is running under different user id than applications.") Ideally, signond would also run under apparmor confinement.

The ~/.signon directory is created with curious permissions:
$ ls -ld ~/.signon/
drwxrwx--x 2 jamie jamie 4096 Aug 15 15:55 /home/jamie/.signon/

While the signon.db is created safely:
$ ls -l ~/.signon/
-rw-r----- 1 jamie jamie 26624 Aug 15 15:29 signon.db

For defense in depth, it makes sense to tighten the directory permissions to 0700.

I'm going to give this an ACK for now with the understanding that some changes might have to be performed. This will be discussed outside of this bug.