Repository: pion/sctp
Pull request #298 (2024-02-08 20:39:48)

Support Unknown Parameters in INIT Chunk
Start to implement section-3.2.1 and respect chunks with a action of Skip. We don't properly report the values back yet.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Attention: `11 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`3a6b5ef`)](https://app.codecov.io/gh/pion/sctp/commit/3a6b5ef0642705e5d7dd9583e7e03f3e6264f7f7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) 80.48% compared to head [(`890670a`)](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) 67.30%.

| [Files](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Patch % | Lines |
|---|---|---|
| [chunk\_init.go](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y2h1bmtfaW5pdC5nbw==) | 0.00% | [9 Missing and 1 partial :warning: ](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) |
| [chunk\_init\_common.go](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y2h1bmtfaW5pdF9jb21tb24uZ28=) | 88.88% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           master     #298       +/-   ##
===========================================
- Coverage   80.48%   67.30%   -13.18%     
===========================================
  Files          48       48               
  Lines        4053     4056        +3     
===========================================
- Hits         3262     2730      -532     
- Misses        648     1172      +524     
- Partials      143      154       +11     
```

| [Flag](https://app.codecov.io/gh/pion/sctp/pull/298/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/sctp/pull/298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/sctp/pull/298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `67.30% <45.00%> (+0.12%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/sctp/pull/298?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

Sean-Der: If someone has the availability mind looking at this? Soon Chrome is going to roll out with this new param (that we don't support yet)

Pion is going to fail to negotiate SCTP then. I want to get this out so we don't see DataChannels failing. 
Sean-Der: Test failure is ok/expected. We are breaking public API by not having unmarshal errors anymore

Patch:
diff --git a/chunk_init.go b/chunk_init.go
index fb89844..a6a3be3 100644
--- a/chunk_init.go
+++ b/chunk_init.go
@@ -38,6 +38,7 @@ var (
 	ErrInitInboundStreamRequestZero  = errors.New("INIT ACK inbound stream request must be > 0")
 	ErrInitOutboundStreamRequestZero = errors.New("INIT ACK outbound stream request must be > 0")
 	ErrInitAdvertisedReceiver1500    = errors.New("INIT ACK Advertised Receiver Window Credit (a_rwnd) must be >= 1500")
+	ErrInitUnknownParam              = errors.New("INIT with unknown param")
 )
 
 func (i *chunkInit) unmarshal(raw []byte) error {
@@ -89,8 +90,7 @@ func (i *chunkInit) check() (abort bool, err error) {
 	// to be 0, the receiver MUST treat it as an error and close the
 	// association by transmitting an ABORT.
 	if i.initiateTag == 0 {
-		abort = true
-		return abort, ErrChunkTypeInitInitateTagZero
+		return true, ErrChunkTypeInitInitateTagZero
 	}
 
 	// Defines the maximum number of streams the sender of this INIT
@@ -104,8 +104,7 @@ func (i *chunkInit) check() (abort bool, err error) {
 	// Note: A receiver of an INIT with the MIS value of 0 SHOULD abort
 	// the association.
 	if i.numInboundStreams == 0 {
-		abort = true
-		return abort, ErrInitInboundStreamRequestZero
+		return true, ErrInitInboundStreamRequestZero
 	}
 
 	// Defines the number of outbound streams the sender of this INIT
@@ -116,8 +115,7 @@ func (i *chunkInit) check() (abort bool, err error) {
 	// abort the association.
 
 	if i.numOutboundStreams == 0 {
-		abort = true
-		return abort, ErrInitOutboundStreamRequestZero
+		return true, ErrInitOutboundStreamRequestZero
 	}
 
 	// An SCTP receiver MUST be able to receive a minimum of 1500 bytes in
@@ -125,8 +123,14 @@ func (i *chunkInit) check() (abort bool, err error) {
 	// less than 1500 bytes in its initial a_rwnd sent in the INIT or INIT
 	// ACK.
 	if i.advertisedReceiverWindowCredit < 1500 {
-		abort = true
-		return abort, ErrInitAdvertisedReceiver1500
+		return true, ErrInitAdvertisedReceiver1500
+	}
+
+	for _, p := range i.unrecognizedParams {
+		if p.unrecognizedAction == paramHeaderUnrecognizedActionStop ||
+			p.unrecognizedAction == paramHeaderUnrecognizedActionStopAndReport {
+			return true, ErrInitUnknownParam
+		}
 	}
 
 	return false, nil
diff --git a/chunk_init_common.go b/chunk_init_common.go
index 63c70c9..b3f8b84 100644
--- a/chunk_init_common.go
+++ b/chunk_init_common.go
@@ -49,6 +49,7 @@ type chunkInitCommon struct {
 	numInboundStreams              uint16
 	initialTSN                     uint32
 	params                         []param
+	unrecognizedParams             []paramHeader
 }
 
 const (
@@ -59,7 +60,6 @@ const (
 // Init chunk errors
 var (
 	ErrInitChunkParseParamTypeFailed = errors.New("failed to parse param type")
-	ErrInitChunkUnmarshalParam       = errors.New("failed unmarshalling param in Init Chunk")
 	ErrInitAckMarshalParam           = errors.New("unable to marshal parameter for INIT/INITACK")
 )
 
@@ -91,18 +91,21 @@ func (i *chunkInitCommon) unmarshal(raw []byte) error {
 	remaining := len(raw) - offset
 	for remaining > 0 {
 		if remaining > initOptionalVarHeaderLength {
-			pType, err := parseParamType(raw[offset:])
-			if err != nil {
+			var pHeader paramHeader
+			if err := pHeader.unmarshal(raw[offset:]); err != nil {
 				return fmt.Errorf("%w: %v", ErrInitChunkParseParamTypeFailed, err) //nolint:errorlint
 			}
-			p, err := buildParam(pType, raw[offset:])
+
+			p, err := buildParam(pHeader.typ, raw[offset:])
 			if err != nil {
-				return fmt.Errorf("%w: %v", ErrInitChunkUnmarshalParam, err) //nolint:errorlint
+				i.unrecognizedParams = append(i.unrecognizedParams, pHeader)
+			} else {
+				i.params = append(i.params, p)
 			}
-			i.params = append(i.params, p)
-			padding := getPadding(p.length())
-			offset += p.length() + padding
-			remaining -= p.length() + padding
+
+			padding := getPadding(pHeader.length())
+			offset += pHeader.length() + padding
+			remaining -= pHeader.length() + padding
 		} else {
 			break
 		}
diff --git a/paramheader.go b/paramheader.go
index 5cef07d..5b69442 100644
--- a/paramheader.go
+++ b/paramheader.go
@@ -10,13 +10,40 @@ import (
 	"fmt"
 )
 
+type paramHeaderUnrecognizedAction byte
+
 type paramHeader struct {
-	typ paramType
-	len int
-	raw []byte
+	typ                paramType
+	unrecognizedAction paramHeaderUnrecognizedAction
+	len                int
+	raw                []byte
 }
 
+/*
+	 The Parameter Types are encoded such that the highest-order 2 bits specify
+	 the action that is taken if the processing endpoint does not recognize the
+	 Parameter Type.
+
+	 00 - Stop processing this parameter and do not process any further parameters within this chunk.
+
+	 01 - Stop processing this parameter, do not process any further parameters within this chunk, and
+		  report the unrecognized parameter, as described in Section 3.2.2.
+
+	 10 - Skip this parameter and continue processing.
+
+	 11 - Skip this parameter and continue processing, but report the unrecognized
+		  parameter, as described in Section 3.2.2.
+
+	 https://www.rfc-editor.org/rfc/rfc9260.html#section-3.2.1
+*/
+
 const (
+	paramHeaderUnrecognizedActionMask                                        = 0b11000000
+	paramHeaderUnrecognizedActionStop          paramHeaderUnrecognizedAction = 0b00000000
+	paramHeaderUnrecognizedActionStopAndReport paramHeaderUnrecognizedAction = 0b01000000
+	paramHeaderUnrecognizedActionSkip          paramHeaderUnrecognizedAction = 0b10000000
+	paramHeaderUnrecognizedActionSkipAndReport paramHeaderUnrecognizedAction = 0b11000000
+
 	paramHeaderLength = 4
 )
 
@@ -57,6 +84,7 @@ func (p *paramHeader) unmarshal(raw []byte) error {
 		return fmt.Errorf("%w: %v", ErrParamHeaderParseFailed, err) //nolint:errorlint
 	}
 	p.typ = typ
+	p.unrecognizedAction = paramHeaderUnrecognizedAction(raw[0] & paramHeaderUnrecognizedActionMask)
 	p.raw = raw[paramHeaderLength:paramLengthPlusHeader]
 	p.len = int(paramLengthPlusHeader)
 


Test patch:
diff --git a/chunk_init_test.go b/chunk_init_test.go
new file mode 100644
index 0000000..e235e3f
--- /dev/null
+++ b/chunk_init_test.go
@@ -0,0 +1,35 @@
+// SPDX-FileCopyrightText: 2023 The Pion community <https://pion.ly>
+// SPDX-License-Identifier: MIT
+
+package sctp
+
+import (
+	"testing"
+)
+
+func TestChunkInit_UnrecognizedParameters(t *testing.T) {
+	initChunkHeader := []byte{
+		0x55, 0xb9, 0x64, 0xa5, 0x00, 0x02, 0x00, 0x00,
+		0x04, 0x00, 0x08, 0x00, 0xe8, 0x6d, 0x10, 0x30,
+	}
+
+	unrecognizedSkip := append([]byte{}, initChunkHeader...)
+	unrecognizedSkip = append(unrecognizedSkip, byte(paramHeaderUnrecognizedActionSkip), 0xFF, 0x00, 0x04, 0x00)
+
+	i := &chunkInitCommon{}
+	if err := i.unmarshal(unrecognizedSkip); err != nil {
+		t.Errorf("Unmarshal init Chunk failed: %v", err)
+	} else if len(i.unrecognizedParams) != 1 || i.unrecognizedParams[0].unrecognizedAction != paramHeaderUnrecognizedActionSkip {
+		t.Errorf("Unrecognized Param parsed incorrectly")
+	}
+
+	unrecognizedStop := append([]byte{}, initChunkHeader...)
+	unrecognizedStop = append(unrecognizedStop, byte(paramHeaderUnrecognizedActionStop), 0xFF, 0x00, 0x04, 0x00)
+
+	i = &chunkInitCommon{}
+	if err := i.unmarshal(unrecognizedStop); err != nil {
+		t.Errorf("Unmarshal init Chunk failed: %v", err)
+	} else if len(i.unrecognizedParams) != 1 || i.unrecognizedParams[0].unrecognizedAction != paramHeaderUnrecognizedActionStop {
+		t.Errorf("Unrecognized Param parsed incorrectly")
+	}
+}
diff --git a/param_ecn_capable_test.go b/param_ecn_capable_test.go
index b62cd5b..92d3ae6 100644
--- a/param_ecn_capable_test.go
+++ b/param_ecn_capable_test.go
@@ -22,9 +22,10 @@ func TestParamECNCapabale_Success(t *testing.T) {
 			testParamECNCapabale(),
 			&paramECNCapable{
 				paramHeader: paramHeader{
-					typ: ecnCapable,
-					len: 4,
-					raw: []byte{},
+					typ:                ecnCapable,
+					unrecognizedAction: paramHeaderUnrecognizedActionSkip,
+					len:                4,
+					raw:                []byte{},
 				},
 			},
 		},
diff --git a/param_forward_tsn_supported_test.go b/param_forward_tsn_supported_test.go
index a64c91e..79048b3 100644
--- a/param_forward_tsn_supported_test.go
+++ b/param_forward_tsn_supported_test.go
@@ -22,9 +22,10 @@ func TestParamForwardTSNSupported_Success(t *testing.T) {
 			testParamForwardTSNSupported(),
 			&paramForwardTSNSupported{
 				paramHeader: paramHeader{
-					typ: forwardTSNSupp,
-					len: 4,
-					raw: []byte{},
+					typ:                forwardTSNSupp,
+					len:                4,
+					unrecognizedAction: paramHeaderUnrecognizedActionSkipAndReport,
+					raw:                []byte{},
 				},
 			},
 		},