Comment 2 for bug 1040682

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

This is a fairly old code base and there is a lot to look at. I could only provide a high level review, but I am quite confident there are many bugs lurking in it.

Security review:
* One CVE. Trivial to fix.
* Hardening options enabled. Should enable PIE.
* Several calls to malloc/calloc with no check for NULL followed by string operations in lib/ipmi_sel.c. ipmi_fru.c doesn't check a malloc call in ipmi_fru_query_new_value()
* There is lots of strcpy and sprintf with little bounds checking. Many are on the stack, so compiler hardening should catch it. I did not have time to verify if they are attacker controllable.
* There are a lot of compiler warnings
* There is an initscript for ipmievd. I couldn't start the daemon since I don't have any /dev/ipmi* files, but the initscript starts ipmievd as root and I don't see an calls to drop privileges, so I am assuming this is running as root.
* networking code is present for talking to remote servers

I am not confident in the code base. There are lots of compiler warnings and unchecked memory allocations followed by string operations. Lots of strcpy() with little bounds checking (though admittedly, some on the stack). It does have one thing going for it: other distributions also include it, so we should be able to collaborate on security fixes.

I would normally NAK this and advise to search for an alternative. In lieu of that, conditional ACK provided that the compiler warnings are addressed, that PIE is enabled and that an apparmor profile is provided for /usr/sbin/ipmievd and /usr/bin/ipmitool.