Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FD_CLOEXEC bug fixes (filters) + support (actions). Avoid sockets descriptors leak. #167

Merged
merged 1 commit into from
Apr 13, 2013

Conversation

ncollignon
Copy link
Contributor

  • 001-fail2ban-server-socket-close-on-exec-no-leak.diff

Add code that marks server and client sockets with FD_CLOEXEC flags.
Avoid leaking file descriptors to processes spawned when handling
fail2ban actions (ex: iptables).

Unix sockets managed by fail2ban-server don't need to be passed to any
child process. Fail2ban already uses the FD_CLOEXEC flags in the filter
code.

This patch also avoids giving iptables access to fail2ban UNIX socket in
a SELinux environment (A sane SELinux policy should trigger an audit
event because "iptables" will be given read/write access to the fail2ban
control socket).

Some random references related to this bug:
http://sourceforge.net/tracker/?func=detail&atid=689044&aid=2086568&group_id=121032
http://www.redhat.com/archives/fedora-selinux-list/2009-June/msg00124.html
http://forums.fedoraforum.org/showthread.php?t=234230

  • 002-fail2ban-filters-close-on-exec-typo-fix.diff

There is a typo in the fail2ban server/filter.py source code. The
FD_CLOEXEC is correctly set but additional random flags are also set.
It has no side-effect as long as the fd doesn't match a valid flag :)
"fcntl.fcntl(fd, fcntl.F_SETFD, fd | fcntl.FD_CLOEXEC)" <== the 3rd
parameter should be flags, not a file descriptor.

  • 003-fail2ban-gamin-socket-close-on-exec-no-leak.diff

Add code that marks the Gamin monitor file descriptor with FD_CLOEXEC
flags. Avoid leaking file descriptors to processes spawned when handling
fail2ban actions (ex: iptables).


File descriptors in action process before patches:
dr-x------ 2 root root 0 .
dr-xr-xr-x 8 root root 0 ..
lr-x------ 1 root root 64 0 -> /dev/null <== OK
l-wx------ 1 root root 64 1 -> /tmp/test.log <== used by test action
lrwx------ 1 root root 64 2 -> /dev/null <== OK
lrwx------ 1 root root 64 3 -> socket:[116361] <== NOK (fail2ban.sock leak)
lr-x------ 1 root root 64 4 -> /proc/20090/fd <== used by test action
l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK
lrwx------ 1 root root 64 6 -> socket:[115608] <== NOK (gamin sock leak)

File descriptors in action process after patches:
dr-x------ 2 root root 0 .
dr-xr-xr-x 8 root root 0 ..
lr-x------ 1 root root 64 0 -> /dev/null <== OK
l-wx------ 1 root root 64 1 -> /tmp/test.log <== used by test action
lrwx------ 1 root root 64 2 -> /dev/null <== OK
lr-x------ 1 root root 64 3 -> /proc/18284/fd <== used by test action
l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK

 * 001-fail2ban-server-socket-close-on-exec-no-leak.diff

Add code that marks server and client sockets with FD_CLOEXEC flags.
Avoid leaking file descriptors to processes spawned when handling
fail2ban actions (ex: iptables).

Unix sockets managed by fail2ban-server don't need to be passed to any
child process. Fail2ban already uses the FD_CLOEXEC flags in the filter
code.

This patch also avoids giving iptables access to fail2ban UNIX socket in
a SELinux environment (A sane SELinux policy should trigger an audit
event because "iptables" will be given read/write access to the fail2ban
control socket).

Some random references related to this bug:
 http://sourceforge.net/tracker/?func=detail&atid=689044&aid=2086568&group_id=121032
 http://www.redhat.com/archives/fedora-selinux-list/2009-June/msg00124.html
 http://forums.fedoraforum.org/showthread.php?t=234230

 * 002-fail2ban-filters-close-on-exec-typo-fix.diff

There is a typo in the fail2ban server/filter.py source code. The
FD_CLOEXEC is correctly set but additional *random* flags are also set.
It has no side-effect as long as the fd doesn't match a valid flag :)
"fcntl.fcntl(fd, fcntl.F_SETFD, fd | fcntl.FD_CLOEXEC)" <== the 3rd
parameter should be flags, not a file descriptor.

 * 003-fail2ban-gamin-socket-close-on-exec-no-leak.diff

Add code that marks the Gamin monitor file descriptor with FD_CLOEXEC
flags. Avoid leaking file descriptors to processes spawned when handling
fail2ban actions (ex: iptables).

---

File descriptors in action process before patches:
dr-x------ 2 root root  0 .
dr-xr-xr-x 8 root root  0 ..
lr-x------ 1 root root 64 0 -> /dev/null        <== OK
l-wx------ 1 root root 64 1 -> /tmp/test.log    <== used by test action
lrwx------ 1 root root 64 2 -> /dev/null        <== OK
lrwx------ 1 root root 64 3 -> socket:[116361]  <== NOK (fail2ban.sock leak)
lr-x------ 1 root root 64 4 -> /proc/20090/fd   <== used by test action
l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK
lrwx------ 1 root root 64 6 -> socket:[115608]  <== NOK (gamin sock leak)

File descriptors in action process after patches:
dr-x------ 2 root root  0 .
dr-xr-xr-x 8 root root  0 ..
lr-x------ 1 root root 64 0 -> /dev/null        <== OK
l-wx------ 1 root root 64 1 -> /tmp/test.log    <== used by test action
lrwx------ 1 root root 64 2 -> /dev/null        <== OK
lr-x------ 1 root root 64 3 -> /proc/18284/fd   <== used by test action
l-wx------ 1 root root 64 5 -> /var/log/fail2ban.log <== OK
@coveralls
Copy link

Coverage remained the same when pulling 39667ff on ncollignon:master into f5ad99b on fail2ban:master.

View Details

@yarikoptic
Copy link
Member

Looks good to me -- thanks . Just to make sure -- are these patches "user tested" already? e.g. they were picked up from some packages of fail2ban (in Fedora?)

would you mind crafting a little unittest e.g. at least for the 001-fail2ban-server-socket-close-on-exec-no-leak.diff portion?

@yarikoptic yarikoptic merged commit 39667ff into fail2ban:master Apr 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants