On 20/10/2016 15:12, Alan DeKok wrote:

Those two identities don't have to be related in any way whatsoever.

Exactly. So I'm intentionally demonstrating the case where they are different; a potential attacker can choose whichever outer identity they like.

...

Note how I've chosen "steve" as the anonymous identity. "steve" isn't really an "anonymous" identity. For a discussion of anonymous identities, see my RFC:

https://tools.ietf.org/html/rfc7542#section-2.4 Yes I know. An attacker can choose whichever identity they like. Even the stock Android client lets you enter whichever "anonymous" identity you like; no special tools required.

...

What happens is that is in the second Access-Challenge response, steve's attributes are returned: Which is what you told it to do. It's what the default config tells it to do. I was surprised. It does not seem like desirable default behaviour: specifically, an unauthorized user being able to generate RADIUS reply attributes which belong to some other user.

...

It's clearly wrong to return steve's authorization attributes, since we've not authenticated at all (and certainly not as steve) - although since this only an Access-Challenge, hopefully the NAS will ignore them. The EAP exchange does complete successfully. See the comments in the raddb/sites-available/default. Look for "Access-Challenge". The documentation describes the problem, and shows how to fix it.

I had read that already. It says that "older configurations" had this problem, and it is commented out. The implication is that the current configuration *doesn't* have this problem. But I see this behaviour with the current sample configuration which is supplied with freeradius 3.0.12.

...

My other concern is that it does an unnecessary database lookup for "steve" - actually the live config which started this investigation is an LDAP one, which is how I noticed this.

Now, the default site has in its authorize section:

eap { ok = return }

But at this step we're getting "updated". So it looks like it would be reasonable to change this to:

eap { ok = return updated = return } It depends. Given the stable nature of 3.0, I'm inclined to leave it.

Do you mean leave the sample config as-is, or leave the behaviour of the rlm_eap module as-is? Would you say rlm_eap is behaving as intended by returning 'updated' for this particular step of the exchange?

...

... and this does seem to work. But I wonder why it's done this way in the default config. Is this a mistake, or this there some subtle point I am missing? Other peoples systems may behave differently from yours. So I'm inclined to leave the current configuration. Well, obviously people's configs are different, but if they copy-pasted this part of eap config then they should behave the same.

This seems to be an under-documented area of freeradius. For example: http://wiki.freeradius.org/modules/Rlm_eap /usr/share/doc/freeradius-3.0.11/modules/rlm_eap I couldn't find any reference to return codes, or "ok" or "updated", in either of these.

We will be fixing all of this in v4. Not by accident, but by design.

...

Under what circumstances does rlm_eap return "updated" instead of "ok"? I want to be sure that there's no security impact by dropping out of the authorize section at this point, for example if someone uses a non-tunneled version of EAP like EAP-TLS or EAP-PWD. That's what tests are for. The server comes with configuration files for eapol_test (see src/tests/eap*.conf).

I can't see any tests which exercise the individual modules and check their return codes under different scenarios. I can see high-level tests exercise the general behaviour ("does radclient return 0?"). I'm not sure it actually checks the returned RADIUS attributes at each stage of the exchange, nor scenarios which require a reject rather than an accept. But that's just from a cursory scan of the scripts. Anyway, I'm not necessarily saying that the module behaviour is wrong. What I am saying is: * the module returns different return values at different parts of the EAP exchange, and this appears to be undocumented * the sample config doesn't take this into consideration, and ends up sometimes dropping through to later parts of the outer config part-way through the EAP exchange (but not every time) In fact, it explicitly says: # The EAP module returns "ok" if it is not yet ready to # authenticate the user. The configuration below checks for # that code, and stops processing the "authorize" section if # so. # # Any LDAP and/or SQL servers will not be queried for the # initial set of packets that go back and forth to set up # TTLS or PEAP. which means I think I was right to expect that eap { ok = return } would catch everything except the end of the exchange. * therefore, the sample config could be improved to match the description (or the rlm_eap behaviour changed to return "ok", but that's a matter for you) IMO freeradius's configurable failover mechanism is subtle at the best of times, so robust sample configs are especially helpful. Regards, Brian.

On Oct 20, 2016, at 10:53 AM, Brian Candler <b.candler@pobox.com> wrote:

Exactly. So I'm intentionally demonstrating the case where they are different; a potential attacker can choose whichever outer identity they like.

Yes... that's well known. What can the "attacker" do, in this case? Cause the server to send the "wrong" attributes in an Access-Challenge? And what, exactly, happens as a result of this "attack"? Nothing. Absolutely nothing.

It's what the default config tells it to do. I was surprised. It does not seem like desirable default behaviour: specifically, an unauthorized user being able to generate RADIUS reply attributes which belong to some other user.

Not "reply", Access-Challenge. And definitely not Access-Accept.

I had read that already. It says that "older configurations" had this problem, and it is commented out. The implication is that the current configuration *doesn't* have this problem.

But I see this behaviour with the current sample configuration which is supplied with freeradius 3.0.12.

Then (drum roll) *edit the configuration*. That's what the comments mean.

Do you mean leave the sample config as-is, or leave the behaviour of the rlm_eap module as-is?

I mean I'm not going to change 3.0 unless someone convinces me that a security problem exists.

Would you say rlm_eap is behaving as intended by returning 'updated' for this particular step of the exchange?

It works as written. I'd have to go over the whole module again to see what / why it does things. Since I'm busy with 4.0 re-design... addressing issues in 3.0 is less of a priority.

...

...

... and this does seem to work. But I wonder why it's done this way in the default config. Is this a mistake, or this there some subtle point I am missing? Other peoples systems may behave differently from yours. So I'm inclined to leave the current configuration. Well, obviously people's configs are different, but if they copy-pasted this part of eap config then they should behave the same.

Don't be obtuse. The default configuration is intended to work for the largest amount of people. Changing it *may* mean that it stops working for some people. People who likely won't test it before the change, and won't comment on the change, and likely won't know what happened when it suddenly stops working. *That* is what I meant by "I'm inclined to leave the current configuration".

This seems to be an under-documented area of freeradius. For example: http://wiki.freeradius.org/modules/Rlm_eap /usr/share/doc/freeradius-3.0.11/modules/rlm_eap

If only the Wiki was editable. And if only people could read the source, and update the Wiki.

I couldn't find any reference to return codes, or "ok" or "updated", in either of these.

As always, patches are welcome. Complaining is less useful than making a contribution.

I can't see any tests which exercise the individual modules and check their return codes under different scenarios.

Then submit some.

Anyway, I'm not necessarily saying that the module behaviour is wrong. What I am saying is:

* the module returns different return values at different parts of the EAP exchange, and this appears to be undocumented

Because (a) most modules don't have their return codes documented, (b) no one contributed documentation for that, and (c) it doesn't matter to 99.9% of the people using the module, and (d) it's documented in the source code, for people who really truly *need* to know.

* the sample config doesn't take this into consideration, and ends up sometimes dropping through to later parts of the outer config part-way through the EAP exchange (but not every time)

It's not random. Don't make it sound like it is. The code documents what it does, and why. I suggest reading the EAP module source.

In fact, it explicitly says:

# The EAP module returns "ok" if it is not yet ready to # authenticate the user. The configuration below checks for # that code, and stops processing the "authorize" section if # so. # # Any LDAP and/or SQL servers will not be queried for the # initial set of packets that go back and forth to set up # TTLS or PEAP.

which means I think I was right to expect that eap { ok = return } would catch everything except the end of the exchange.

Or, EAP is more complicated than you think. i.e. your change works for your tests, but will cause other EAP methods to break.

* therefore, the sample config could be improved to match the description (or the rlm_eap behaviour changed to return "ok", but that's a matter for you)

No. Just.... no. I am *not* going to change the code or the documentation because you didn't understand what it's doing, and therefore decided it was broken. That is *entirely* the wrong approach. If you want to change the code, then read the code, and explain why it's broken. If you want to change the documentation, then explain why the documentation doesn't match the behaviour of the server. If you want to change the default configuration in 3.0, then explain why the change doesn't cause problems for people.

IMO freeradius's configurable failover mechanism is subtle at the best of times, so robust sample configs are especially helpful.

I *really* hate passive-aggressive comments like this. I don't need to be told that sample configurations are helpful. I know, I've spent nearly 20 years writing code, documentation, and example configurations that *work*. Perhaps you could explain why the default configurations *don't work*. Explain why they're *broken*. That would be much more productive than making snide comments. You've convinced me (so far) that you've seen an "attack" where none exists, and that without understanding how EAP works you want to change it, and to make that change without understanding how it affects everyone else. Which is just not a helpful approach. Alan DeKok.

On Oct 21, 2016, at 8:12 AM, Brian Candler <b.candler@pobox.com> wrote:

On 20/10/2016 20:01, Alan DeKok wrote:

...

...

I couldn't find any reference to return codes, or "ok" or "updated", in either of these. As always, patches are welcome.

Would this be an acceptable?

I'd rather have the comments there, but the "updated = return" commented out by default. I've gone through the code, and I'm just not convinced that this change will have zero effect on all EAP types. We're planning on fixing this for v4. Not by figuring out the EAP module return codes, but fixing it by design. i.e. the ongoing conversations should be run through their own processing sections / virtual server, and NOT through the normal config. The path for "I received a packet, what do I do?" is just too different from the path taken for "I've received the third packet of an ongoing EAP exchange" Alan DeKok.