Subject: Re: [BEHAVE] STUN fingerprint and integrity tests

From: Benny Prijono <[email-masked]>
Date: Mon, 17 Sep 2007 19:26:45 +0100

Jonathan Rosenberg wrote:
> An important point of clarification here.
> 
> Though FINGERPRINT is currently the only attribute beyond the MI, there 
> could potentially be others in the future so you can't depend on it 
> being the only one. So inserting a dummy FINGERPRINT won't help.

Jonathan,

sorry for joining late on the discussion, but your comment concerns 
me. Allowing any other attribute, except FINGERPRINT, beyond M-I, 
not only violates rfc3489bis-10 but could also break existing 
implementation as well.

The part where it violates rfc3489bis-10 is in Section 14.5:

"When present, the FINGERPRINT attribute MUST be the last attribute 
in the message, and thus will appear after MESSAGE-INTEGRITY."

And the reason why it may break existing implementation is because, 
in order to avoid linear searching for M-I throughout all 
attributes, currently we could probe M-I at position 
(20+hdr.length-24), if the M-I is the last attribute, or at position 
(20+hdr.length-24-8), if the M-I appears as the attribute before 
FINGERPRINT.

Allowing some other attributes to exist between M-I and FINGERPRINT 
will break above algorithm, and it will mean linear search for the 
M-I. So I propose we disallow this.

  -benny

_______________________________________________
Behave mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/behave

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Mon, 17 Sep 2007 13:44:24 -0700

Benny Prijono wrote:
>> Though FINGERPRINT is currently the only attribute beyond the MI,
>> there could potentially be others in the future so you can't depend on
>> it being the only one. So inserting a dummy FINGERPRINT won't help.
> 
> Jonathan,
> 
> sorry for joining late on the discussion, but your comment concerns me.
> Allowing any other attribute, except FINGERPRINT, beyond M-I, not only
> violates rfc3489bis-10 but could also break existing implementation as
> well.
> 
> "When present, the FINGERPRINT attribute MUST be the last attribute in
> the message, and thus will appear after MESSAGE-INTEGRITY."
> 
> And the reason why it may break existing implementation is because, in
> order to avoid linear searching for M-I throughout all attributes,
> currently we could probe M-I at position (20+hdr.length-24), if the M-I
> is the last attribute, or at position (20+hdr.length-24-8), if the M-I
> appears as the attribute before FINGERPRINT.
> 
> Allowing some other attributes to exist between M-I and FINGERPRINT will
> break above algorithm, and it will mean linear search for the M-I. So I
> propose we disallow this.
> 

I think that changing the MUST in rfc3489bis-10 to a SHOULD is a better
solution.  Who knows if another attribute after the FINGERPRINT
attribute will not be needed in the future.

-- 
Marc Petit-Huguenin           [                                 ]
Home: [email-masked] [RFC1855-compliant space for rent ]
Work: [email-masked]            [                                 ]
[                                                               ]

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Mon, 17 Sep 2007 17:13:45 -0700

Brian Korver wrote:
> On Sep 17, 2007, at 1:44 PM, Marc Petit-Huguenin wrote:
> Making this a SHOULD means we can't count on it being last, that we must
> always do a sequential search for it.  I vote for sticking with MUST and
> revisiting the issue if/when a good reason presents itself.  

Well, the problem is that it will not be possible to change this
decision in a future RFC, because it will be incompatible with the
current version.

> The
> proposed change seems like premature future-proofing.

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Tue, 18 Sep 2007 12:57:58 +0300

> not only violates rfc3489bis-10 but could also break existing

If there is a known attribute after FINGERPRINT, someone is trying to screw 
you and you MUST ignore the attribute.

If there is an unknown optional attribute after FINGERPRINT, you MUST ignore 
it anyway, regardless of where FINGERPRINT is.

If there is an unknown mandatory attribute after FINGERPRINT, you MUST drop 
the entire message regardless of where FINGERPRINT is.

As such I do not see much of an issue, except the likeliness that someone will 
implement the spec wrong.

> (20+hdr.length-24-8), if the M-I appears as the attribute before

You MUST do at least one linear pass over the entire STUN message, because you 
MUST look for unknown mandatory attributes. Hence, that is a non-issue.

> M-I. So I propose we disallow this.

IMHO, that algorithm is already broken.

-- 
Rémi Denis-Courmont

---

From: Benny Prijono <[email-masked]>
Date: Tue, 18 Sep 2007 12:37:32 +0100

> implement the spec wrong.

The issue that I was asking was for attributes between M-I and
FINGERPRINT, not the ones after FINGERPRINT.

Interestingly though, the processing for attributes beyond
FINGERPRINT like above is not mentioned anywhere in the spec. What I
found is:

Section 14.5
   When present, the FINGERPRINT attribute MUST be the last attribute
   in the message, and thus will appear after MESSAGE-INTEGRITY.

So I think we should drop the entire message if attributes are found
beyond FINGERPRINT, regardless whether its comprehension is required
or not.

> MUST look for unknown mandatory attributes. Hence, that is a non-issue.

True that linear searching will have to be done in parsing, and
checking for invalid attributes is done in parsing stage. But not
everybody does everything in a single function. Some will have
authentication in separate layer after parsing is done.

> IMHO, that algorithm is already broken.

Not if we only allow FINGERPRINT after M-I. And certainly that was 
valid in bis-06, which I based my implementation on. :-(

  -benny

---

From: Rémi Denis-Courmont <[email-masked]>
Date: Tue, 18 Sep 2007 15:10:58 +0300

Le Tuesday 18 September 2007 14:37:32 ext Benny Prijono, vous avez écrit :
> > If there is a known attribute after FINGERPRINT, someone is trying to
> > screw you and you MUST ignore the attribute.
> >
> > If there is an unknown optional attribute after FINGERPRINT, you MUST
> > ignore it anyway, regardless of where FINGERPRINT is.
> >
> > If there is an unknown mandatory attribute after FINGERPRINT, you MUST
> > drop the entire message regardless of where FINGERPRINT is.
> >
> > As such I do not see much of an issue, except the likeliness that someone
> > will implement the spec wrong.
>

My mistake. Simply s/FINGERPRINT/MESSAGE-INTEGRITY/g in the previous 
sentences, and you get my point.

> beyond FINGERPRINT, regardless whether its comprehension is required

Right. I meant MESSAGE-INTEGRITY.

> > You MUST do at least one linear pass over the entire STUN message,
> > because you MUST look for unknown mandatory attributes. Hence, that is a
> > non-issue.
>

You can always flag attributes that are after M-I during the parsing phase. 
Then it only adds O(1) time per attribute (and O(1) memory).

> valid in bis-06, which I based my implementation on. :-(

STUN NAT control needs to be able to add stuff after M-I already. This change 
has been introduced very much on-purpose, as far as I understand.

-- 
Rémi Denis-Courmont

---

From: Benny Prijono <[email-masked]>
Date: Tue, 18 Sep 2007 13:56:46 +0100

> sentences, and you get my point.

In that case, the attributes must be ignored instead, as per 14.4:

  "With the exception of the FINGERPRINT attribute, which appears 
after MESSAGE-INTEGRITY, agents MUST ignore all other attributes 
that follow MESSAGE-INTEGRITY."

> Then it only adds O(1) time per attribute (and O(1) memory).

True, software can always be changed (unfortunately!).

> has been introduced very much on-purpose, as far as I understand.

Okay, I guess I'll have to accept that.

I was just updating my software from 06 to 10, and it seems that I 
missed reading the part where non-FINGERPRINT attribute is now 
allowed past M-I, thus I was quite surprised to hear Jonathan's comment.

Back to coding now..

thanks,
  -benny

---

From: Brian Korver <[email-masked]>
Date: Mon, 17 Sep 2007 16:38:37 -0700

> attribute will not be needed in the future.

Making this a SHOULD means we can't count on it being last, that we  
must always do a sequential search for it.  I vote for sticking with  
MUST and revisiting the issue if/when a good reason presents itself.   
The proposed change seems like premature future-proofing.

-brian
[email-masked]

---

From: Brian Korver <[email-masked]>
Date: Mon, 17 Sep 2007 19:04:27 -0700

> decision in a future RFC, because it will be incompatible with the

Sure, but you could say that of any MUST.

-brian
[email-masked]

---

From: Brian Korver <[email-masked]>
Date: Mon, 17 Sep 2007 16:38:37 -0700

-brian
[email-masked]

---

From: Brian Korver <[email-masked]>
Date: Mon, 17 Sep 2007 19:04:27 -0700

-brian
[email-masked]