1531904 - RTCPeerConnection.createDataChannel doesn't do a very good job of validating stream ids

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Description

[Byron Campen [:bwc]]

https://jsfiddle.net/jhbLk2on/

We seem to be relying entirely on webidl to do checking for us, but webidl does not seem to be upset when -1 is used to init an unsigned short. We are also not checking for 65535 as the spec requires.

Comment 1

[Byron Campen [:bwc]]

It seems to me that our hands are a little bit tied here; either this is a bug in our implementation of webidl, or the webidl spec makes it impossible to detect integer overflow when initting a webidl dictionary.

jib, what's your take?

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

AFAICT WebIDL converts -1 to an unsigned short of 63335:

• Let x be ? ConvertToInt(V, 16, "unsigned"), which says:
  • Let lowerBound be 0. Let upperBound be 2^16 − 1.
  • Let x be ? ToNumber(V) // here -1
  • Set x to ! IntegerPart(x). // still -1
  • Set x to x modulo 2^16 // here 65535

So we just need to implement this prose:

18. If [[DataChannelId]] is equal to 65535, which is greater than the maximum allowed ID of 65534 but still qualifies as an unsigned short, throw a TypeError.

This means, according to the spec, that -1 should TypeError, whereas e.g. -2 should not.

We could file a spec issue for stricter checking, e.g. using [EnforceRange]. That might be a good idea.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Wait, looks like this has already been done. RTCDataChannelInit says:

dictionary RTCDataChannelInit {
             boolean         ordered = true;
             [EnforceRange]
             unsigned short  maxPacketLifeTime;
             [EnforceRange]
             unsigned short  maxRetransmits;
             USVString       protocol = "";
             boolean         negotiated = false;
             [EnforceRange]
             unsigned short  id;
             RTCPriorityType priority = "low";
};

We just need to update our webidl to match.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This changes comment 2 to

• If the conversion is to an IDL type associated with the [EnforceRange] extended attribute, then:
  • If x is NaN, +∞, or −∞, then throw a TypeError.
  • Set x to ! IntegerPart(x).
  • If x < lowerBound or x > upperBound, then throw a TypeError.

👍

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We are also not checking for 65535 as the spec requires.

We still need to test for that manually, even after we add [EnforceRange].

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I also think the -1 test should be changed to test for -2.

Comment 7

[Byron Campen [:bwc]]

Alright, looks like we can fix this.

Comment 8

[Byron Campen [:bwc]]

Attachment: Bug 1531904 - Part 0: Re-enable some test-cases. r?jib

Comment 9

[Byron Campen [:bwc]]

Attachment: Bug 1531904 - Part 1: webidl updates. r?jib,smaug

Depends on D26769

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1531904 - Part 2: Keep up with webidl changes in part 1. r?smaug

Depends on D26770

Comment 11

[Byron Campen [:bwc]]

Attachment: Bug 1531904 - Part 3: Fix some validation logic in createDataChannel. r?jib

Depends on D26771

Comment 12

[Byron Campen [:bwc]]

Attachment: Bug 1531904 - Part 4: Fix some test-cases, and add some others. r?jib

Depends on D26772

Comment 13

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85ef447e4e154aeec7162d8559b66b621293543

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb1553deea2f
Part 0: Re-enable some test-cases. r=jib
https://hg.mozilla.org/integration/autoland/rev/86d62da0f2a8
Part 1: webidl updates. r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/08102fb37153
Part 2: Keep up with webidl changes in part 1. r=smaug,jib
https://hg.mozilla.org/integration/autoland/rev/3e57d6dc64b2
Part 3: Fix some validation logic in createDataChannel. r=jib
https://hg.mozilla.org/integration/autoland/rev/85cc0827d8d3
Part 4: Fix some test-cases, and add some others. r=jib

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/eb1553deea2f
https://hg.mozilla.org/mozilla-central/rev/86d62da0f2a8
https://hg.mozilla.org/mozilla-central/rev/08102fb37153
https://hg.mozilla.org/mozilla-central/rev/3e57d6dc64b2
https://hg.mozilla.org/mozilla-central/rev/85cc0827d8d3

Comment 16

[James Graham [:jgraham]]

The test changes here conflict with the upstream changes from https://github.com/web-platform-tests/wpt/pull/16038 and I'm not confident I can come up with the desired resolution. Can you suggest a resolved version of the two files (webrtc/RTCPeerConnection-ondatachannel.html webrtc/RTCPeerConnection-createDataChannel.html)

Comment 17

[Byron Campen [:bwc]]

Sent mail.