Don't reject offers/replies that contain a malformed candidate #1315
Description
Your environment.
- Version: Sorry don't know how to get the SHA for a Go package? I ran
go get github.com/pion/webrtc/v3today (13th of Jul 2020). - Browser: No browser. The offer SDP was generated using a custom webrtc stack.
aaron@pcdodo:~/go/src/github.com/pion/webrtc/examples/data-channels$ go version
go version go1.13.8 linux/amd64
aaron@pcdodo:~/go/src/github.com/pion/webrtc/examples/data-channels$ lsb_release -a
No LSB modules are available.
Distributor ID:
Ubuntu
Description: Ubuntu 20.04
LTS
Release: 20.04
Codename: focal
aaron@pcdodo:~/go/src/github.com/pion/webrtc/examples/data-channels$ uname -a
Linux pcdodo 4.4.0-19041-Microsoft #1-Microsoft Fri Dec 06 14:06:00 PST 2019 x86_64 x86_64 x86_64 GNU/Linux
aaron@pcdodo:~/go/src/github.com/pion/webrtc/examples/data-channels$
What did you do?
Attempted to run the data-channels example using:
echo <sdp offer> | go run main.go
What happened?
aaron@pcdodo:~/go/src/github.com/pion/webrtc/examples/data-channels$ echo eyJ0eXBlIjoib2ZmZXIiLCJzZHAiOiJ2PTBcclxubz0tIDY5MjkwIDAgSU4gSVA0IDEyNy4wLjAuMVxyXG5zPS1cclxudD0wIDBcclxuYT1maW5nZXJwcmludDpzaGEtMjU2IEM2OkVEOjhDOjlEOjA2OjUwOjc3OjIzOjBBOjRBOkQ4OjQyOjY4OjI5OkQwOjcwOjJGOkJCOkM3OjcyOkVDOjk4OjVDOjYyOjA3OjFCOjBDOjVEOkNCOkNFOkJFOkNEXHJcbmE9Z3JvdXA6QlVORExFIDBcclxubT1hcHBsaWNhdGlvbiA5IFVEUC9EVExTL1NDVFAgd2VicnRjLWRhdGFjaGFubmVsXHJcbmM9SU4gSVA0IDAuMC4wLjBcclxuYT1pY2UtdWZyYWc6V05HUVxyXG5hPWljZS1wd2Q6SU9aUEdWSFdaTkZXRlJORVNJVEVGSVZOXHJcbmE9ZmluZ2VycHJpbnQ6c2hhLTI1NiBDNjpFRDo4Qzo5RDowNjo1MDo3NzoyMzowQTo0QTpEODo0Mjo2ODoyOTpEMDo3MDoyRjpCQjpDNzo3MjpFQzo5ODo1Qzo2MjowNzoxQjowQzo1RDpDQjpDRTpCRTpDRFxyXG5hPWNhbmRpZGF0ZToyNDQxIDEgdWRwIDY1OTEzNiBmZTgwOjo1NGE5OmQyMzg6YjJlZTpjZWIlMjEgNjA3MjAgdHlwIGhvc3QgZ2VuZXJhdGlvbiAwXHJcbmE9Y2FuZGlkYXRlOjI5NDQgMSB1ZHAgNjU5MTM2IDE5Mi4xNjguMTEuNTAgNjA3MjAgdHlwIGhvc3QgZ2VuZXJhdGlvbiAwXHJcbmE9aWNlLW9wdGlvbnM6aWNlMix0cmlja2xlXHJcbmE9bWlkOjBcclxuYT1lbmQtb2YtY2FuZGlkYXRlc1xyXG5hPXNldHVwOmFjdHBhc3NcclxuYT1zY3RwLXBvcnQ6NDAwMFxyXG5hPW1heC1tZXNzYWdlLXNpemU6MjYyMTQ0XHJcbiJ9 | go run main.go
panic: failed to parse address
goroutine 1 [running]:
main.main()
/home/aaron/go/src/github.com/pion/webrtc/examples/data-channels/main.go:68 +0x431
exit status 2
The deserialised SDP is as below. If the ICE candidate with the IPv6 line is removed then the error does not occur.
v=0
o=- 69290 0 IN IP4 127.0.0.1
s=-
t=0 0
a=fingerprint:sha-256 C6:ED:8C:9D:06:50:77:23:0A:4A:D8:42:68:29:D0:70:2F:BB:C7:72:EC:98:5C:62:07:1B:0C:5D:CB:CE:BE:CD
a=group:BUNDLE 0
m=application 9 UDP/DTLS/SCTP webrtc-datachannel
c=IN IP4 0.0.0.0
a=ice-ufrag:WNGQ
a=ice-pwd:IOZPGVHWZNFWFRNESITEFIVN
a=fingerprint:sha-256 C6:ED:8C:9D:06:50:77:23:0A:4A:D8:42:68:29:D0:70:2F:BB:C7:72:EC:98:5C:62:07:1B:0C:5D:CB:CE:BE:CD
a=candidate:2441 1 udp 659136 fe80::54a9:d238:b2ee:ceb%21 60720 typ host generation 0
a=candidate:2944 1 udp 659136 192.168.11.50 60720 typ host generation 0
a=ice-options:ice2,trickle
a=mid:0
a=end-of-candidates
a=setup:actpass
a=sctp-port:4000
a=max-message-size:262144
A separate related issue is the SDP answer received from the data-channels example isn't setting the ICE candidate's `foundation value. An example SDP answer is below:
eyJ0eXBlIjoiYW5zd2VyIiwic2RwIjoidj0wXHJcbm89LSAzNTA1NTg0OTUgMTU5NDY3NjE0NCBJTiBJUDQgMC4wLjAuMFxyXG5zPS1cclxudD0wIDBcclxuYT1maW5nZXJwcmludDpzaGEtMjU2IDQzOjEyOjIwOjlFOjYwOkQxOjBBOkM2OjZCOkMxOkU5OkZGOjY0OkQ2OkYxOjEyOkQwOjA3OjY0OjE5OkMxOkI3OjMxOjI1OjQ5OkQzOjIzOkIxOkI1OkFBOkI5OjI5XHJcbmE9Z3JvdXA6QlVORExFIDBcclxubT1hcHBsaWNhdGlvbiA5IERUTFMvU0NUUCA1MDAwXHJcbmM9SU4gSVA0IDAuMC4wLjBcclxuYT1zZXR1cDphY3RpdmVcclxuYT1taWQ6MFxyXG5hPXNlbmRyZWN2XHJcbmE9c2N0cG1hcDo1MDAwIHdlYnJ0Yy1kYXRhY2hhbm5lbCAxMDI0XHJcbmE9aWNlLXVmcmFnOkFkaWNRZWdaSVBsWnByaUNcclxuYT1pY2UtcHdkOkxpWWJISXd6c05FT0hJUnFxWW1vUWpwZFh0bFNUcGpGXHJcbmE9Y2FuZGlkYXRlOmZvdW5kYXRpb24gMSB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjExLjUwIDUxNzk5IHR5cCBob3N0IGdlbmVyYXRpb24gMFxyXG5hPWNhbmRpZGF0ZTpmb3VuZGF0aW9uIDIgdWRwIDIxMzA3MDY0MzEgMTkyLjE2OC4xMS41MCA1MTc5OSB0eXAgaG9zdCBnZW5lcmF0aW9uIDBcclxuYT1jYW5kaWRhdGU6Zm91bmRhdGlvbiAxIHVkcCAyMTMwNzA2NDMxIDE3Mi4yOS4xNjAuMSA1MTgwMCB0eXAgaG9zdCBnZW5lcmF0aW9uIDBcclxuYT1jYW5kaWRhdGU6Zm91bmRhdGlvbiAyIHVkcCAyMTMwNzA2NDMxIDE3Mi4yOS4xNjAuMSA1MTgwMCB0eXAgaG9zdCBnZW5lcmF0aW9uIDBcclxuYT1jYW5kaWRhdGU6Zm91bmRhdGlvbiAxIHVkcCAyMTMwNzA2NDMxIDE5Mi4xNjguNjQuMSA1MTgwMSB0eXAgaG9zdCBnZW5lcmF0aW9uIDBcclxuYT1jYW5kaWRhdGU6Zm91bmRhdGlvbiAyIHVkcCAyMTMwNzA2NDMxIDE5Mi4xNjguNjQuMSA1MTgwMSB0eXAgaG9zdCBnZW5lcmF0aW9uIDBcclxuYT1jYW5kaWRhdGU6Zm91bmRhdGlvbiAxIHVkcCAyMTMwNzA2NDMxIDE5Mi4xNjguMTM3LjEgNTE4MDYgdHlwIGhvc3QgZ2VuZXJhdGlvbiAwXHJcbmE9Y2FuZGlkYXRlOmZvdW5kYXRpb24gMiB1ZHAgMjEzMDcwNjQzMSAxOTIuMTY4LjEzNy4xIDUxODA2IHR5cCBob3N0IGdlbmVyYXRpb24gMFxyXG5hPWNhbmRpZGF0ZTpmb3VuZGF0aW9uIDEgdWRwIDE2OTQ0OTg4MTUgMzcuMjI4LjI0My4xMDcgMjQ0MTMgdHlwIHNyZmx4IHJhZGRyIDAuMC4wLjAgcnBvcnQgNTE4MTIgZ2VuZXJhdGlvbiAwXHJcbmE9Y2FuZGlkYXRlOmZvdW5kYXRpb24gMiB1ZHAgMTY5NDQ5ODgxNSAzNy4yMjguMjQzLjEwNyAyNDQxMyB0eXAgc3JmbHggcmFkZHIgMC4wLjAuMCBycG9ydCA1MTgxMiBnZW5lcmF0aW9uIDBcclxuYT1lbmQtb2YtY2FuZGlkYXRlc1xyXG4ifQ==
Deserialised:
v=0
o=- 350558495 1594676144 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 43:12:20:9E:60:D1:0A:C6:6B:C1:E9:FF:64:D6:F1:12:D0:07:64:19:C1:B7:31:25:49:D3:23:B1:B5:AA:B9:29
a=group:BUNDLE 0
m=application 9 DTLS/SCTP 5000
c=IN IP4 0.0.0.0
a=setup:active
a=mid:0
a=sendrecv
a=sctpmap:5000 webrtc-datachannel 1024
a=ice-ufrag:AdicQegZIPlZpriC
a=ice-pwd:LiYbHIwzsNEOHIRqqYmoQjpdXtlSTpjF
a=candidate:foundation 1 udp 2130706431 192.168.11.50 51799 typ host generation 0
a=candidate:foundation 2 udp 2130706431 192.168.11.50 51799 typ host generation 0
a=candidate:foundation 1 udp 2130706431 172.29.160.1 51800 typ host generation 0
a=candidate:foundation 2 udp 2130706431 172.29.160.1 51800 typ host generation 0
a=candidate:foundation 1 udp 2130706431 192.168.64.1 51801 typ host generation 0
a=candidate:foundation 2 udp 2130706431 192.168.64.1 51801 typ host generation 0
a=candidate:foundation 1 udp 2130706431 192.168.137.1 51806 typ host generation 0
a=candidate:foundation 2 udp 2130706431 192.168.137.1 51806 typ host generation 0
a=candidate:foundation 1 udp 1694498815 37.228.243.107 24413 typ srflx raddr 0.0.0.0 rport 51812 generation 0
a=candidate:foundation 2 udp 1694498815 37.228.243.107 24413 typ srflx raddr 0.0.0.0 rport 51812 generation 0
a=end-of-candidates
Activity
Sean-Der commentedon Jul 16, 2020
Thanks for the report @sipsorcery ! I will check this out this weekend.
I haven't put any time myself in getting IPv6 working. Will drop an estimate or a PR if things go really well :)
jech commentedon Aug 13, 2020
That's a link-local IPv6 address with an interface identifier. The interface identifier is local to the sending host, and therefore doesn't make any sense in this context. It is not legal according to the grammar given in Section 15.1 of RFC 5245 (which ultimately refers to the IP6-address production in Section 9 of RFC 4566).
@Sean-Der, it's a matter of opinion, but I think that in the interest of robustness Pion should ignore malformed candidate lines rather than failing the whole SDP.
sipsorcery commentedon Aug 13, 2020
An alternative would be to snip the
zone IDprefix off the IPv6 address.RFC 4007: IPv6 Scoped Address Architecture, which deals with the
zone ID(Windows) orscope ID(Linux), does indicate it can be ignored if not understood.jech commentedon Aug 13, 2020
sipsorcery commentedon Aug 13, 2020
On Windows 10 connection attempts to IPv6 link local addresses work fine.
The telnet connection attempt from a Windows 10 machine to an Ubuntu machine works.
> telnet fe80::bcc:137b:3bc1:b926 8081Ubuntu running:
nc -6 -l 8081On Ubuntu the IPv6 link local addresses don't seem very usable to be even able to ping the local address on the machine the interface name is required. And I wasn't able to find any permutation where Ubuntu could ping a Windows link local address.
ping -6 fe80::bcc:137b:3bc1:b926%eno1So I guess in short the IPv6 link local address ICE candidates are useful on Windows and not so much on Linux. Supporting them is probably not that critical in either case.
jech commentedon Aug 13, 2020
[-]Error parsing IPv6 addresses on ICE candidates[/-][+]IPv6 Support[/+]jech commentedon Dec 18, 2021
Probably related to 2699584 and 6160e80.
Add IPv6 Test
Add IPv6 Test
Add IPv6 Test
Add IPv6 Test
Add IPv6 Test
7 remaining items
[-]IPv6 Support[/-][+]Don't reject offers/replies that contain a malformed candidate[/+]Sean-Der commentedon Apr 17, 2025
Oh wow, good catch @jech I am very lucky/grateful to have you around. I am so impressed you are reading the activity on these.
Let me check what libwebrtc does. I can see it going both ways.
@jech What do you think is better? RFC 9413 has been pushing me lately to go with point #2
If we go with #2 I will add tests to ensure we do throw an error properly/give a useful error message.
JoTurk commentedon Apr 17, 2025
@Sean-Der i was going to make an issue to discuss this, personally I think we should go with #2 but not sure how we would log / pass errors, stderr is easy to ignore, and we don't have a clear error collector pattern, But idk.
nils-ohlmeier commentedon Apr 17, 2025
@Sean-Der maybe my two cents are making thing more complicated, but I do think that these topics are a little bit more nuanced.
From my point of view ICE RFCs have rules in them for how to parse
a=candidatelines in SDP. If a candidate is properly formed, but for example contains an IPv7 address the SDP or ICE parser should fail because it encountered an unknown transport protocol. It should parse it as best as it can and then pass that unknown address to the ICE stack, which then can decided if this is a fatal error, or if it want to ignore this unknown, unsupported transport.Same goes for candidate lines can have for example
myawesomeextension=1, which the parser doesn't need to understand. But it should not fail when encountering a new, unknown extension.I think where the parser should return an error with something which doesn't conform to the formatting rules from the ICE RFCs, for example
a=candidate:foobar my very own thingshould result in the SDP getting rejected.jech commentedon Apr 17, 2025
As Nils said, SDP is an extensible format, so we should be accepting candidate lines that are syntactically valid but contain information that we don't understand (yet). The expedient way to implement this is to simply drop any candidate line that we don't understand, perhaps after logging a warning, and continue on the rest of the SDP.
If we want to do anything more ambitious, then we'll need to carefully check the relevant RFCs in order to find exactly which candidate lines are syntactically invalid and justify aborting the parsing process. I'm not sure we have the manpower for that.
JoTurk commentedon Apr 18, 2025
The candidates parser in ICE is now close to the spec, and it would reject candidates with even invalid extensions or just a single invalid character, pion/ice#754 it even caught a bug in someone implementation few weeks ago pion/ice#768 (we added
candidate:prefix after that pion/ice#769) I wanted to make it follow the RFCs more, and same for how we handle SDP candidates.If everyone agrees I can do this in no time!
jech commentedon Apr 18, 2025
I don't have strong feelings on this, but I have a slight preference for being more permissive than what you suggest, if I understand you correctly. In particular, I suggest just ignoring the candidate if an address is malformed, since this is what this issue was initially about.