eap module returning 'updated' rather than 'ok'

[Alan DeKok]

On Oct 20, 2016, at 10:53 AM, Brian Candler <b.candler at 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.