-----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 <security@freeradius.org>, 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-----