Subject: [eap] RE: mapping between the state machine and 2284bis

From: [email-masked] ([email-masked])
Date: Wed, 14 May 2003 17:11:43 +0300

Jari,

Big thanks for reviewing the state machine document!=20
We've spent so much time on those diagrams what we're=20
probably blind to any problems in it by now :-)

> DISCARD: OK (with a nit) -- 2284bis has more detail
>   - Section 4 has a silent discard for other Code values
>   - I also recently sent e-mail about whether Length >=3D
>     received PDU should
>     cause a silent discard. This too might be something
>     that parseEapReq does, though it isn't specified in
>     detail.

Yes, parseEapReq is supposed to do this: if the PDU has=20
invalid length or code, it doesn't set rxReq (which results=20
in silenty discard).

>   - Perhaps there should be a "logEvent" call, as is
>     required in 2284bis in the silent discard definition?

We discussed this, but decided not to show any logging calls
here. Real implementations will probably log many events not
explicitly mentioned for logging in 2284bis (such as success=20
and failure!), and logging does not change the behavior of the
change machine.

(For the same reason, we didn't include any "statistics"
variables that would not change the operation of the state
machine, but could be interesting to show in some authenticator
management interface)

> GET_METHOD: NOT OK -- same level of detail, one nit and one problem
>    - Incoming arrow valid according to Section 2.1 and Section 4
>    - First if branch valid according to Section 2.1
>    - Second if branch valid, but note that it would be imho
>      clearer if setting the methodState to NORMAL in METHOD
>      would also result in setting currentMethod to NONE.
>      Then we could remove "resetCurrentMethod"? On the other
>      hand, you appear to rely on currentMethod to stay intact
>      for forbidding sequences... right?

There's a new version of the peer diagram that has simplified
this state and METHOD state considerably. You may want to review
that one instead (although we don't have yet the explanatory
text for it).

It's available at:
http://www.cs.hut.fi/~peronen/eap/eap_peer_13052003.png

Regarding your question: Setting methodState to NORMAL can't
reset currentMethod, because NORMAL is used for two different
purposes: when the method has ended, and when the method can
either continue or end (this is one of the things handled
differently in 13052003 version).

> - The third if branch appears unclear or perhaps incorrect. Section =
2.1:
>    "A peer MUST NOT send a Nak (legacy or expanded) in reply to a
>     Request, after an initial non-Nak Response has been sent. Since
>     spoofed EAP Request packets may be sent by an attacker, an an
>     authenticator receiving an unexpected Nak SHOULD silently discard
>     it and log the event."
>
>     And here we send a Nak (even if we were in the middle of
>     another method). The spec talks about it being invalid for
>     the authenticator to send other types while already doing
>     one authentication method. We also talk about the strict
>     mode, where such requests would be silently discarded.
>     But I didn't find a place that says what we should do
>     outside the strict mode for such requests. So should we
>     respond with a Nak, as the sm does? Or silent discard?
>     If silent discard, we need to split the third if branch
>     into two, one dealing with currentmethod =3D NONE and
>     one with the other case.

Again, this is handled quite differently in 13052003 version,
but the basic principle is correct in sm-02 version as well: the
third branch (that sends the Nak) is reached only if
currentMethod is NONE or IDENTITY (assuming we're not inside a
tunnel, something removed in 13052003 version completely).=20
So we are not in the middle of a method, and haven't sent any
responses for Types >=3D4 yet.

> METHOD: NOT OK -- one nit and one small error
>     - Aligned with A.1 in general, but:
>     - Perhaps there should be a call to logEvent, to show the
>      SHOULD requirement in A.1?
>    - A.1 allows MIC failure to be a fatal error. This does
>      not appear to be handled by the sm.

As an unintended consequence of other changes, the last=20
item seems to be better handled in 13052003 version :-)

Best regards,
Pasi

---

From: [email-masked] (Jari Arkko)
Date: Wed, 21 May 2003 16:53:47 +0300

[email-masked] wrote:
> Jari,
> 
> Big thanks for reviewing the state machine document! 
> We've spent so much time on those diagrams what we're 
>>  - I also recently sent e-mail about whether Length >=
>>    received PDU should
> Yes, parseEapReq is supposed to do this: if the PDU has 
> invalid length or code, it doesn't set rxReq (which results 
> in silenty discard).

OK.

> explicitly mentioned for logging in 2284bis (such as success 

Sounds good. The picture almost doesn't fit to a page
now, it makes sense to restrict it. (Did you say that
you consider statistics and logging out of scope
somewhere in the document?)

> differently in 13052003 version).

The new version looks very good, much clearer. Thanks...

>>- The third if branch appears unclear or perhaps incorrect. Section 2.1:
>>    into two, one dealing with currentmethod = NONE and
>>    one with the other case.
> 
> 
> tunnel, something removed in 13052003 version completely). 
> responses for Types >=4 yet.

Yes, at least the new version appears to work correctly
for this.

(FYI: I was confused for a while trying to search the condition
which handles the reception of a new non-NOTIF method while
already doing something else. It turns out that it was the
"else" branch from RECEIVED to DISCARD. I'm not sure whether
this should be made more explicit in the picture, but should
at least be pointed out by the text.)

> As an unintended consequence of other changes, the last 

I'm not sure why this works in the new version. Can you
explain? If intCheck == FALSE then you just discard
and continue as-is.  On the other hand, if "decision"
is set to FAIL, then you have a fatal error. So you'll
have to interpret fatal MIC errors as something where
intCheck is set to TRUE? You can do this, but this
should probably be explained somewhere. Another way
is to transition non-deterministically to either DISCARD
or FAILURE when intCheck == FALSE ;-)

--Jari