search

pion / stun

A Go implementation of STUN
https://pion.ly/
MIT License
644 stars 95 forks source link

Ignore comprehension bits in the STUN attribute type field #21

Closed enobufs closed 5 years ago

enobufs commented 5 years ago

Your environment.

What did you do?

STUN binding transaction failed with the following public STUN servers:

What did you expect?

STUN binding should succeed.

What happened?

These servers (older implementation) returns XOR-MAPPED-ADDRESS with comprehension-required bit set, the type value being 0x8020.. Pion.stun only expects XOR-MAPPED-ADDRESS to be 0x0020, and discards the attribute.

These servers are implemented with older version of STUN, when XOR-MAPPED-ADDRESS was assigned as 'comprehension-optional', therefore, it should be optional for the client to use, but there's no way to retrieve the attribute because it is dropped, and 0x08020 is not defined. Both 0x0020 and 0x8020 must be treated as XOR-MAPPED-ADDRESS.

We should ignore the first bit of attribute field so that we can parse the field correctly regardless of the comprehension bits. Instead, we should introduce a new field to RawAttribute struct, for example, ComprehensionOptional bool so that the application can tell whether the field was required or not. This way, we can use older version of STUN server, and also the pion would be compatible with the future version of STUN messages, meaning, the following attributes are currently optional, but it may become a requirement in the future. At that point, current pion implementation would break!

enobufs commented 5 years ago

Hmmm... I thought the attribute type values masked with 0x7fff would be all unique, but there's one exception.

RFC 5389

   0x8022: SOFTWARE

RFC 5766

   0x0022: RESERVATION-TOKEN

All other values are uniquely assigned tho... :(

Seeing the change of value for XOR-MAPPED-ADRESS from 0x8020 to 0x0020 in the past, I believe IETF (or IANA) must had made a mistake assigning values for SOFTWARE and RESERVATION-TOKEN...