Enable hardening options in the build #3445
Conversation
The mode argument to open() is not optional when O_CREAT is used. The compiler doesn't check for it by default but it breaks the build under _FORTIFY_SOURCE Fixes build regression on default Fedora flags introduced in commit 6e19c16
fchmodat() on glibc has warn_unused_result attribute and so this emits a compiler warning with -fhardened, and that in turn breaks the build with -Werror. Using += is enough to trick the compiler to think we actually used the value for something. We SHOULD handle failure here somehow intelligently (eg differentiate between ENOENT and possible other harmless errors etc) but can't dive to that just now.
pmatilai
requested review from
ffesti and
dmnks
and removed request for
a team
pmatilai
force-pushed
the
hardened-pr
branch
from
6065aea
to
41a367a
Compare
| @@ -82,8 +84,13 @@ static rpmRC audit_tsm_post(rpmPlugin plugin, rpmts ts, int res) | |||
| rasprintf(&eventTxt, | |||
| "op=%s %s sw_type=rpm key_enforce=%u gpg_res=%u %s", | |||
| op, nevra, enforce, verified, dir); | |||
| audit_log_user_comm_message(auditFd, AUDIT_SOFTWARE_UPDATE, | |||
| eventTxt, NULL, NULL, NULL, NULL, result); | |||
| if (audit_log_user_comm_message(auditFd, AUDIT_SOFTWARE_UPDATE, | |||
There was a problem hiding this comment.
Maybe this deserves a TODO comment, like the one in removeSBITS(), just so we can grep for these (in theory) in the future.
There was a problem hiding this comment.
Figured it's actually just as easy to do the right thing and make it a warning, just filter out the message you get when auditd isn't running, similarly to what we do in systemd_inhibit and dbus_announce.
There was a problem hiding this comment.
Oh neat, seems like the innocent nitpick turned into something more useful 😄
There was a problem hiding this comment.
Yup. This is the kind of stuff that happens when you just want something quickly out of the way and instead of stopping to think for a second (because this is not the thing you're interested just now) you think to work around it somehow. And then later realize doing the right thing would've been at least as simple 🤦 😄
There was a problem hiding this comment.
Yep, having someone else stop you in the tracks, if just for a second, unlocks your hidden potential sometimes 😄
pmatilai
force-pushed
the
hardened-pr
branch
2 times, most recently
from
e4b2c63
to
154ece0
Compare
audit_log_user_comm_message has warn_unused_result attribute and so this emits a compiler warning with -fhardened, and that in turn breaks the build with -Werror. Emit a warning if audit log message fails, but suppress ECONNREFUSED to silence spurious warnings in environments where audit daemon isn't available, such as containers (like our test-suite) or rescue images. This isn't entirely ideal but is consistent with what we do in similar cases in eg systemd_inhibit (708e613) and dbus_announce (071be75) plugins. For extra entertainment, something in the GH CI environment causes runroot_user tests to fail with EPERM, whereas no such errors occur locally. Filter it out too.
This probably explains some of the oddities and seemingly missing warnings we've been seeing...
This enables all manner of highly useful safety checks that we generally want enabled.
pmatilai
force-pushed
the
hardened-pr
branch
from
154ece0
to
b44af5a
Compare
|
Yay, all green now. Looks good now, thanks! |
Commit 6e19c16 broke the build under Fedora default compiler flags due to missing mode argument to open(). Fix that and couple of not-handled return code cases, and make the build default to -fhardened to avoid this happening again.