Skip to content

Commit 09ab8cb

Browse files
philipch07philipch07
committed
Ignore malformed candidates
1 parent 9488911 commit 09ab8cb

File tree

2 files changed

+196
-10
lines changed

2 files changed

+196
-10
lines changed

‎unmarshal.go‎

Lines changed: 47 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package sdp
66
import (
77
"errors"
88
"fmt"
9+
"net"
910
"net/url"
1011
"strconv"
1112
"strings"
@@ -134,6 +135,24 @@ func (s *SessionDescription) UnmarshalString(value string) error {
134135
return nil
135136
}
136137

138+
// isBadCandidateAddress returns true if the candidate line has an invalid connection-address.
139+
// It allows mDNS hostnames (".local") per ICE-mDNS.
140+
func isBadCandidateAddress(candidateValue string) bool {
141+
fields := strings.Fields(candidateValue)
142+
143+
if len(fields) < 6 {
144+
return true // clearly malformed candidate line
145+
}
146+
147+
addr := fields[4]
148+
149+
if strings.HasSuffix(addr, ".local") {
150+
return false // always allow mDNS host candidates
151+
}
152+
153+
return net.ParseIP(addr) == nil
154+
}
155+
137156
// Unmarshal converts the value into a []byte and then calls UnmarshalString.
138157
// Callers should use the more performant UnmarshalString.
139158
func (s *SessionDescription) Unmarshal(value []byte) error {
@@ -813,18 +832,27 @@ func unmarshalSessionEncryptionKey(l *lexer) (stateFn, error) {
813832
return s11, nil
814833
}
815834

816-
func unmarshalSessionAttribute(l *lexer) (stateFn, error) {
817-
value, err := l.readLine()
835+
func unmarshalSessionAttribute(lex *lexer) (stateFn, error) {
836+
value, err := lex.readLine()
818837
if err != nil {
819838
return nil, err
820839
}
821840

822841
i := strings.IndexRune(value, ':')
823-
a := l.cache.getSessionAttribute()
824842
if i > 0 {
825-
a.Key = value[:i]
826-
a.Value = value[i+1:]
843+
key := value[:i]
844+
val := value[i+1:]
845+
846+
if key == AttrKeyCandidate && isBadCandidateAddress(val) {
847+
// Ignore malformed candidate; keep processing rest of SDP
848+
return s11, nil
849+
}
850+
851+
a := lex.cache.getSessionAttribute()
852+
a.Key = key
853+
a.Value = val
827854
} else {
855+
a := lex.cache.getSessionAttribute()
828856
a.Key = value
829857
}
830858

@@ -975,18 +1003,27 @@ func unmarshalMediaEncryptionKey(l *lexer) (stateFn, error) {
9751003
return s14, nil
9761004
}
9771005

978-
func unmarshalMediaAttribute(l *lexer) (stateFn, error) {
979-
value, err := l.readLine()
1006+
func unmarshalMediaAttribute(lex *lexer) (stateFn, error) {
1007+
value, err := lex.readLine()
9801008
if err != nil {
9811009
return nil, err
9821010
}
9831011

9841012
i := strings.IndexRune(value, ':')
985-
a := l.cache.getMediaAttribute()
9861013
if i > 0 {
987-
a.Key = value[:i]
988-
a.Value = value[i+1:]
1014+
key := value[:i]
1015+
val := value[i+1:]
1016+
1017+
if key == AttrKeyCandidate && isBadCandidateAddress(val) {
1018+
// Ignore malformed candidate; keep processing rest of SDP
1019+
return s14, nil
1020+
}
1021+
1022+
a := lex.cache.getMediaAttribute()
1023+
a.Key = key
1024+
a.Value = val
9891025
} else {
1026+
a := lex.cache.getMediaAttribute()
9901027
a.Key = value
9911028
}
9921029

‎unmarshal_test.go‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44
package sdp
55

66
import (
7+
"net"
8+
"strings"
79
"testing"
10+
"unicode"
811

912
"github.com/stretchr/testify/assert"
1013
)
@@ -173,6 +176,11 @@ const (
173176
MediaTCPTLSMRCPv2 = TimingSDP +
174177
"m=application 1544 TCP/TLS/MRCPv2 1\r\n"
175178

179+
minimalBaseSDP = "v=0\r\n" +
180+
"o=- 1 1 IN IP4 0.0.0.0\r\n" +
181+
"s=-\r\n" +
182+
"t=0 0\r\n"
183+
176184
CanonicalUnmarshalSDP = "v=0\r\n" +
177185
"o=jdoe 2890844526 2890842807 IN IP4 10.47.16.5\r\n" +
178186
"s=SDP Seminar\r\n" +
@@ -579,3 +587,144 @@ func TestUnmarshalOriginEdgeCases(t *testing.T) {
579587
})
580588
}
581589
}
590+
591+
func TestIgnoreMalformedSessionCandidate(t *testing.T) {
592+
in := minimalBaseSDP +
593+
"a=recvonly\r\n" +
594+
// invalid IPv4 address should be ignored
595+
"a=candidate:1 1 UDP 12345 999.999.999.999 1234 typ host\r\n"
596+
597+
var sd SessionDescription
598+
err := sd.UnmarshalString(in)
599+
assert.NoError(t, err)
600+
601+
out, err := sd.Marshal()
602+
assert.NoError(t, err)
603+
604+
// candidate line should be dropped, everything else is preserved
605+
want := minimalBaseSDP + "a=recvonly\r\n"
606+
assert.Equal(t, want, string(out))
607+
}
608+
609+
func TestIgnoreMalformedMediaCandidate(t *testing.T) {
610+
in := minimalBaseSDP +
611+
"m=audio 9 RTP/AVP 0\r\n" +
612+
"a=rtpmap:0 PCMU/8000\r\n" +
613+
// malformed, should be ignored
614+
"a=candidate:1 1 UDP 12345 not_an_ip 1234 typ host\r\n" +
615+
// valid candidate remains
616+
"a=candidate:2 1 UDP 12345 203.0.113.1 2345 typ host\r\n"
617+
618+
var sd SessionDescription
619+
err := sd.UnmarshalString(in)
620+
assert.NoError(t, err)
621+
622+
out, err := sd.Marshal()
623+
assert.NoError(t, err)
624+
625+
want := minimalBaseSDP +
626+
"m=audio 9 RTP/AVP 0\r\n" +
627+
"a=rtpmap:0 PCMU/8000\r\n" +
628+
"a=candidate:2 1 UDP 12345 203.0.113.1 2345 typ host\r\n"
629+
assert.Equal(t, want, string(out))
630+
}
631+
632+
func TestAllowMDNSCandidate(t *testing.T) {
633+
in := minimalBaseSDP +
634+
// mDNS hostnames are valid
635+
"a=candidate:1 1 UDP 12345 myhost.local 1234 typ host\r\n"
636+
637+
var sd SessionDescription
638+
err := sd.UnmarshalString(in)
639+
assert.NoError(t, err)
640+
641+
out, err := sd.Marshal()
642+
assert.NoError(t, err)
643+
assert.Equal(t, in, string(out))
644+
}
645+
646+
func TestAllowIPv6Candidate(t *testing.T) {
647+
in := minimalBaseSDP +
648+
// IPv6 literal is valid
649+
"a=candidate:1 1 UDP 12345 2001:db8::1 5555 typ host\r\n"
650+
651+
var sd SessionDescription
652+
err := sd.UnmarshalString(in)
653+
assert.NoError(t, err)
654+
655+
out, err := sd.Marshal()
656+
assert.NoError(t, err)
657+
assert.Equal(t, in, string(out))
658+
}
659+
660+
func TestIgnoreTruncatedCandidate(t *testing.T) {
661+
in := minimalBaseSDP +
662+
// missing port + fewer than 6 fields after split should be ignored
663+
"a=candidate:1 1 UDP 2113667327 203.0.113.1\r\n"
664+
665+
var sd SessionDescription
666+
err := sd.UnmarshalString(in)
667+
assert.NoError(t, err)
668+
669+
out, err := sd.Marshal()
670+
assert.NoError(t, err)
671+
// attribute should be dropped
672+
assert.Equal(t, minimalBaseSDP, string(out))
673+
}
674+
675+
func FuzzUnmarshalIgnoreMalformedCandidates(f *testing.F) {
676+
// Seeds: valid IPv4, valid IPv6, mDNS, and some bad cases
677+
for _, seed := range []string{
678+
"203.0.113.1",
679+
"2001:db8::1",
680+
"router.local",
681+
"999.999.999.999",
682+
"bad space", // should be sanitized
683+
"", // should be coerced
684+
"::::", // invalid IPv6
685+
"host_name", // underscore is invalid for DNS but we just treat as string
686+
"-.-", // sample punctuation
687+
} {
688+
f.Add(seed)
689+
}
690+
691+
f.Fuzz(func(t *testing.T, addr string) {
692+
// sanitize fuzzed addr to avoid breaking SDP tokenization/lines
693+
addr = strings.Map(func(r rune) rune {
694+
switch {
695+
case unicode.IsLetter(r), unicode.IsDigit(r):
696+
return r
697+
}
698+
// allow a few safe punctuation characters
699+
switch r {
700+
case '.', ':', '-':
701+
return r
702+
default:
703+
return -1
704+
}
705+
}, addr)
706+
if addr == "" {
707+
addr = "bad"
708+
}
709+
710+
line := "a=candidate:1 1 UDP 12345 " + addr + " 1 typ host\r\n"
711+
in := minimalBaseSDP + line
712+
713+
var sd SessionDescription
714+
err := sd.UnmarshalString(in)
715+
// unmarshal should never error if a candidate address is malformed.
716+
assert.NoError(t, err)
717+
718+
outBytes, err := sd.Marshal()
719+
assert.NoError(t, err)
720+
out := string(outBytes)
721+
722+
// Determine if candidate should be kept
723+
keep := strings.HasSuffix(addr, ".local") || net.ParseIP(addr) != nil
724+
if keep {
725+
assert.Contains(t, out, line)
726+
} else {
727+
assert.NotContains(t, out, line)
728+
}
729+
})
730+
}

0 commit comments

Comments
 (0)