search

FreeRADIUS / pam_radius

This is the PAM to RADIUS authentication module. It allows any Linux, OSX or Solaris machine to become a RADIUS client for authentication and password change requests.
GNU General Public License v2.0
102 stars 90 forks source link

Unsafe usage of select() can lead to stack corruption #24

Closed mweissen13 closed 7 years ago

mweissen13 commented 7 years ago

I recently experienced instability issues and or erratic behavior with MariaDB and Dovecot when combined with pam_radius_auth for authentication. The problem only occurred when the server was under load. After a while of bug hunting i was able to pinpoint the exact problem: pam_radius_auth uses a call to select(), which by itself is not a problem. But it can become a problem when the fd numbers go beyond 1024 (at least on linux, where FD_SETSIZE is defined as 1024). If this happens, the stack can get corrupted leading to arbitrary behavior. Please see "man 2 select" under NOTES. In case someone is able to somehow affect the process to open more than 1024 file descriptors this problem could also be leveraged to gain unauthorized access. So i also consider this as a security problem. I have created a patched version of pam_radius_auth which replaces select() with poll(). In our environment this has now been running for some time without any further problems. I will post this patch as a pull request.

alandekok commented 7 years ago

Wow... that's utterly retarded. Which version of Linux are you using? I don't see any such notes on the ones I have.

mweissen13 commented 7 years ago

Here is an online version: http://man7.org/linux/man-pages/man2/select.2.html#NOTES

I am refering to the following: An fd_set is a fixed size buffer. Executing FD_CLR() or FD_SET() with a value of fd that is negative or is equal to or larger than FD_SETSIZE will result in undefined behavior. Moreover, POSIX requires fd to be a valid file descriptor.

It's not really obvious, but internally the select() uses a 1024 bit wide field, which is why going over this boundary will corrupt the process' memory. At the very least we should fail gracefully (maybe by returning PAM_ABORT) when the limit is crossed.

Unfortunately i am still wresting with github on how i can submit a patch via a pull request ;-)