-----BEGIN PGP SIGNED MESSAGE----- Response to Suse Audit Report on FreeRADIUS. 1. Introduction The version audited was 1.0.2.5-5. Version 1.0.4 has been out for almost two months now, and would have been a better choice for auditing. 3. Methodology It would be useful to contribute the "fuzzer" back to FreeRADIUS, so that the code could be tested automatically before any new release. 5.1 rlm_ldap.c * The escaping of additional characters in ldap_escape_func() is a valid consideration, and will be fixed. However, the impact of this vulnerability is extemely small. Unlike SQL, the use of LDAP in the server is read-only. As a result, any "exploit" of this vulnerability means that the attacker can add additional constraints to an LDAP search. The likely result is that the information needed by the server will not be found, and the request will be rejected. * an unavailable LDAP server will not block the daemon. See multiple references to "timeout" in rlm_ldap.c 5.3 rlm_sql.c * The upper limit for the input conversion is set by the caller of sql_escape_func(). It is the callers responsibility to ensure that the output buffer is large enough to contain all possible inputs. The code, and queries have already been audited to ensure that there is no exploit vector. And there is no "Auth-Type := SQL" in the server. A cursory examination of the source code would reveal this information. The attack vector (User-Name "johndoedorian" being truncated to "johndoe") postulated in this section is, in fact, impossible to exploit through the SQL module. The queries used to obtain passwords take only the user name from the RADIUS packet as input. That user name can be a maximum of 253 characters long. The buffer used to create the query is 4k bytes, which is more than sufficient to hold any possible user name value, even if it consisted solely of characters that would be escaped. * The sql_set_user function doesn't do escaping, and is designed to not do escaping. If it did escaping, then there would be multiple layers of escaping, which would be a bug. See the comments in the cvs commit of revision 1.134 of rlm_sql.c 5.4 sql_unixodbc.c * The one-byte overflow in sql_error, lines 362-363 is a valid consideration, and will be fixed. However, the nature of the memory allocation means that there is a high likelihood that the size of the memory will be aligned to 32 or 64 bits, and that the "extra" bytes after the request area will be zero. This side-effect will minimize the impact of the vulnerability. Further, the vulnerability is limited to situations where the query to the external database fails. Unless the external database is under control of an attacker, this vulnerability is not exploitable. If the external database is under control of an attacker, then there are very many other, and worse, things that they can do. A cursory examination of the callers code path indicates that the only use of this data is by the "radlog" (i.e. snprintf) function. The most likely side-effect of this vulnerability is that the server walks off the end of allocated memory, and SEGV's. Finally, a futher examination of the callers code path indicates that there is another bug, not previously known. The "sql_error" function should *not* allocate memory, as none of the callers "free" the string returned by sql_error. This bug will be fixed before the next release of the server. 5.7 rlm_realm.c * There is no need to add a trailing zero to vp->strvalue in check_for_realm(), line 209. All strings in VALUE_PAIR's are NUL terminated. The code does not have to add a trailing NUL because the source is a VALUE_PAIR, which is guaranteed to have a trailing NUL, and the destination is a VALUE_PAIR, which is guaranteed to have enough room to store the source data. * There is no need to add a trailing zero to vp->strvalue in check_for_realm(), line 210. See the preceding comment for details. 5.9 session.c * The max FD in rad_check_ts(), line 202 is a valid consideration, but the suggestion of adding Linux-specific code (OPEN_MAX_LINUX) to the server means that it is guaranteed to not build on other platforms. FreeRADIUS is a cross-platform product, and will continue to be a cross-platform product. Also, OPEN_MAX_LINUX is not available on 2.4 kernels. This issue is mitigated by the fact that the program being executed is "checkrad", which is part of the server core. If an attacker can access open FD's in checkrad, they can write to any program on the system, in which case there are very many other, and worse, things that they can do. * The strings used as command-line options in radius_exec_program() line 224 do not need to be sanitized for command-line options. The only command-line option that checkrad takes is "-d", which is required to be the first parameter. That first parameter is taken from the local configuration file (nastype), and cannot be added by an attacker. The only way to exploit this issue is if an attacker can write to the server configuration files, in which case there are very many other, and worse, things that they can do. 5.11 log.c * Integer wrapping in vradlog(), line 133 is impossible, unless ctime_r returns more than 8k of data. Since ctime_r always returns less than 256 bytes, the buffer overflow and "exploit" is non-existent. * Checking the buffer bounds in vradlog(), line 135 is a valid consideration, and will be addressed. However, the only way to access this function is by through code internal to the server, and none of the server log messages contain sufficient user data to fill the 8k buffer. So while potentially an exploit, there is currently no code path in the server by which this buffer can be over-run. * The strcat in vradlog(), line 150 may overflow the buffer in theory, but a close examination of the code shows that the preceding comment applies here, too. 5.12 auth.c * There is no need to add a trailing zero in rad_auth_log(), lines 131, 143, and 145. These comments are based on a misunderstanding of the server internals. See the comments above for section 5.7. * Yes, in rad_authenticate() line 678, auth_item->strvalue is guaranteed to have a trailing zero. See the comments above for Section 5.7. 5.13 radius.c * rad_decode() Yes, we know that the Vendor-Specific attribute allows for sub-attributes. The Vendor-Specific attribute MAY also contain non-RFC vendor attributes, or plain text (if the vendor does not follow the RFC's). The code in rad_decode() was written to specifically handle these situations, and to deal with it gracefully. Further, the line you've flagged as "AUD: not even checked by rad_recv" (attrlen = *ptr++) is, in fact, checked by the code few lines above (via subptr and sublen), which you've quoted on page 5 of the report.. It is apparent that you did not read the code you quoted, or the comments describing how it works, and what it does. Your comment that "rad_decode() actually interprets the (unchecked) content of the VSA" is simply not true. It should be obvious that it's not true through two pieces of information: 1) the code you quoted on page 5 does, in fact, check the data 2) You indicated that you were unable to trigger the bug using your Fuzzer. As for the "else chunk of the last if() condition", it is unclear what code you're referring to. We suggest reading the code in more detail, and observing the servers response when you send it malformed VSA's. You will see that it collects the data, and places it into a Vendor-Specific attribute of type "octets". This attribute will then hold any data in any format, including non-VSA formats. There is no potential for overflow, as indicated on page 6 of the report. Further, the comment that the "overflow can probably be triggered without knowing any secret" is not entirely correct. The secret is used to validate the packet a few lines above the code you quoted in rad_decode(). For response packets, and for Access-Request packets containing a Message-Authenticator attribute, the code you indicate as "potential overflow" will not be reached. Instead, the packet will be silently discarded as failing validation. For Access-Request packets that do not contain a Message-Authenticator attribute, the "potential overflow" will not occur, because as indicated above, the data from the packet is validated before being used. * The "overflow" you indicate in rad_pwdecode() does not exist in the code. See src/include/radiusd.h. The maximum possible length for secrets is 32 characters. This buffer is more than large enough for any possible secret that can be read from a configuration file. Even if it was exploitable, the server would only be vulnerable to an attacker who could write to the server configuration files. In that case, there are very many other, and worse, things that they can do. * The "overflow" you indicate in rad_tunnel_pwencode() has the same response as the preceding comment. 5.14 xlat.c * Bounds checking in decode_attribute. This is a bug, and will be fixed. However, it is an exploint only for attackers who have access to the server configuration files. In that case, there are very many other, and worse, things that they can do. * radius_xlat, line 725. We have no idea why this comment is relevant. The problem of "SQL characters in DNS hostnames" is, quite simply, irrelevant. For outputs which are not used by the SQL module, escaping DNS data is the wrong thing to do. For outputs which are used by the SQL module, the "sql_escape_func" is used, which you refer to earlier in your analysis. So there is no vulnerability. And if this statement was true, there are many, many, other places in the code where DNS names are used by the server, and those should have been caught by the audit, too. 5.15 exec.c * The closing of file descriptors could be done better. See also the comments above in section 5.9. In addition, the program being executed is under control of the local administrator. If an attacker can access open FD's in the executed program, they can write to any program on the system, in which case there are very many other, and worse, things that they can do. * radius_exec_program(), lines 117-233. Escaping of strings for "_" or "-" will not be done in the server, for the simple reason that any escaping will be wrong. A little time spent thinking about possible solutions and side-effects should lead you to the same conclusion. e.g. "foo-bar" is a valid Unix command. Escaping the "-" is wrong. It is the responsibility of the administrator to ensure that the configuration he creates is not vulnerable to issues such as this. All the server can do is to not create more issues itself, as your solution would do. * Checking for MAX_ENVP is a valid consideration, and will be fixed. It is mitigated by the "security" section of the server, where the server will not accept more than 200 attributes, and MAX_ENVP is set here to 1024. 6. Summary. The summary paragraph is wrong. The comment that "FreeRADIUS tries hardly to enforce the rules of the RADIUS RFC" is condescending, and is designed to inflame. The comments in the source code that you quoted in the report indicates *why* the server processes "malformated packages". (Perhaps you mean "malformed attributes"?) The purpose behind accepting attributes that do not follow the RFC recommendations is vendor interoperability. This purpose is clearly described in the code, so we're not sure how it was missed. See also the comments above for section 5.13. Making recommendations based on misunderstandings and/or misreading of the source code is inappropriate. We suggest that future audit reports do not contain recommendations such as these. There are some valid points in the document. Others, however, show a lack of understanding of RADIUS, and a failure to adequately read the code being audited. 7. The RADIUS Fuzzer It would be good to make the code public. 8. Status The "decoding functions in radius.c need more attention" by Suse, not by FreeRADIUS. The code has been audited multiple times, and there are no known issues. Further, the issues raised by Suse about that code indicate that THE CODE WAS NOT READ. The data flagged as "unchecked" is, in fact, checked by the code a few lines above it, which are quoted in the report (see 5.13, above). This statement indicates severe flaws with the analysis methods used to generate the report. Our Response ------------ By our analysis, the issues Suse raises may be grouped into the following categories: (a) low-impact, non-exploitable externally accessible issues 5.1 (1) ldap escaping (b) low-impact non-exploitable, non-externally accessible issues. 5.4 sql_unixodbc memory allocation error (c) issues exploitable by the RADIUS server administrator 5.9 (1) max_fd 5.11 (2) vradlog, line 135 5.11 (3) vradlog, line 150 5.15 (1) max_fd 5.15 (3) MAX_ENVP not checked (d) Misunderstandings or misreading of the source: 5.1 (2) unavailable LDAP blocking the server 5.3 (1) sql_escape_func 5.3 (2) rlm_sql_authorize, line 747 5.7 (1) check_for_realm, line 209 5.7 (2) check_for_realm, line 210 5.9 (2) arguments to checkrad 5.11 (1) vradlog, line 133 5.12 (1) rad_auth_log 5.12 (2) rad_authenticate line 678 5.13 (2) rad_decode() 5.13 (3) rad_pwdecode() 5.13 (4) rad_tunnel_pwdecode() 5.14 (2) DNS escaping for SQL 5.15 (2) escaping command-line options Out of 21 reported issues, 14 are not real. A false-positive rate of 67% is indicates serious issues with the analysis methods. The issues listed under (d), above are not appropriate for public release. We suggest you delete them from any vulnerability list you have compiled for FreeRADIUS. The other issues will be addressed in 1.0.5, which we were already planning to release this week. Our analysis indicates that an attacker can not use knowledge of these issues to successfully propogate any attack which would result in server compromise, or DoS of the server. As a result, there is no need to keep knowledge of these issues a secret, and we have decided to fix them in CVS. Our Response to later comments by Suse and RedHat ------------------------------------------------- We have concerns with a message Suse sent to the vendor-sec mailing list. Our response is here: http://www.freeradius.org/security/suse-vendor-sec.txt Shortly after our response, RedHat posted a message on Bugtraq, which led to the following vulnerability listing on Security Focus: http://www.securityfocus.com/bid/14775 The comments under "discuss" say: http://www.securityfocus.com/bid/14775/discuss The first issues are memory handling vulnerabilities. These issues may allow remote attackers to crash affected services, or possibly execute arbitrary machine code in the context of the vulnerable application. FreeRADIUS is also affected by a possible file descriptor leak. This may be exploited to gain access to files that an attacker may not normally have access to. Based on our analysis, both of these comments are, quite simply, false. For the issues found by Suse, there is no possible external exploit vector that crashes the system or allows execution of arbitrary machine code. If Suse or RedHat knows of such exploits, it is imperative that they inform the FreeRADIUS team as soon as possible, using the contact address , as indicated on the FreeRADIUS web site. At this time, no such contact has been received. The second comment, that an attacker can use a file descriptor leak "to gain access to files that an attacker may not normally have access to" is again not true. The analysis above in Sections 5.9 and 5.15 explain why this "exploit" is nonsense. RedHat also posted the following bug report on their web site: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=167676 It appears that RedHat did not perform any additional analysis of the project, in order to make an independent determination of any vulnerabilities in FreeRADIUS. This determination is apparent from the fact that other issues discovered by Primoz Bratanic are listed in "doc/ChangeLog", but are not found in RedHat's bug report, or on the Security Focus page. We curious as to Suse's response to our analysis. An earlier version of this response to Suse's report was sent to Suse, and they have not, as yet, responded to the criticisms raised here-in. We believe that Suse should address it's analysis methods, but we have no reason to expect that will happen. In summary, we have released version 1.0.5 of the server in order to address the issues raised by Suse, by Primoz Bratanic, and from internal analysis. We are willing to communicate with anyone about vulnerabilities in the server (potential or otherwise). We welcome the chance to educate security analysts about the design of the server, and about how apparent vulnerabilities may, in fact, be non-exploitable. Alan DeKok Project Leader The FreeRADIUS Server Project. 2005/09/09 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.1 (GNU/Linux) iQCVAwUBQyH6s6kul4vkAkl9AQGfqQP+JieThbHOYEXiJmxQxnmGjmMEdp5esSwY JWNwNxp9Ei9WDsdkJyfpphGfyJBbITYM6G6/3n4AG1zOmg788IX5nkycRIkmBX8j JfPI0mqmbJNNAqsu2kHqKPSaJrC1Z3E69XlvR2UucO8IQLfON5FVqo1OPtggPOVF HqtfGR4K6fI= =iJTz -----END PGP SIGNATURE-----