From 1a344a4c1349d0947dcb6346ea2d35523625a265 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Dec 2022 14:16:01 -0600 Subject: [PATCH 1/5] Reject invalid "Sec-WebSocket-Key" headers from clients Client "Sec-WebSocket-Key" should be a valid 16 byte base64 encoded nonce. If the header is not valid, the server should reject the client. --- accept.go | 7 ++++++- accept_test.go | 33 ++++++++++++++++++++++++++------- internal/test/xrand/xrand.go | 5 +++++ 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/accept.go b/accept.go index b90e15eb..2f4fb2eb 100644 --- a/accept.go +++ b/accept.go @@ -185,10 +185,15 @@ func verifyClientRequest(w http.ResponseWriter, r *http.Request) (errCode int, _ return http.StatusBadRequest, fmt.Errorf("unsupported WebSocket protocol version (only 13 is supported): %q", r.Header.Get("Sec-WebSocket-Version")) } - if r.Header.Get("Sec-WebSocket-Key") == "" { + websocketSecKey := r.Header.Get("Sec-WebSocket-Key") + if websocketSecKey == "" { return http.StatusBadRequest, errors.New("WebSocket protocol violation: missing Sec-WebSocket-Key") } + if v, err := base64.StdEncoding.DecodeString(websocketSecKey); err != nil || len(v) != 16 { + return http.StatusBadRequest, fmt.Errorf("WebSocket protocol violation: invalid Sec-WebSocket-Key %q, must be a 16 byte base64 encoded string", websocketSecKey) + } + return 0, nil } diff --git a/accept_test.go b/accept_test.go index c554bdaf..d0cc4878 100644 --- a/accept_test.go +++ b/accept_test.go @@ -9,6 +9,7 @@ import ( "net" "net/http" "net/http/httptest" + "nhooyr.io/websocket/internal/test/xrand" "strings" "testing" @@ -36,7 +37,7 @@ func TestAccept(t *testing.T) { r.Header.Set("Connection", "Upgrade") r.Header.Set("Upgrade", "websocket") r.Header.Set("Sec-WebSocket-Version", "13") - r.Header.Set("Sec-WebSocket-Key", "meow123") + r.Header.Set("Sec-WebSocket-Key", xrand.Base64(16)) r.Header.Set("Origin", "harhar.com") _, err := Accept(w, r, nil) @@ -52,7 +53,7 @@ func TestAccept(t *testing.T) { r.Header.Set("Connection", "Upgrade") r.Header.Set("Upgrade", "websocket") r.Header.Set("Sec-WebSocket-Version", "13") - r.Header.Set("Sec-WebSocket-Key", "meow123") + r.Header.Set("Sec-WebSocket-Key", xrand.Base64(16)) r.Header.Set("Origin", "https://harhar.com") _, err := Accept(w, r, nil) @@ -116,7 +117,7 @@ func TestAccept(t *testing.T) { r.Header.Set("Connection", "Upgrade") r.Header.Set("Upgrade", "websocket") r.Header.Set("Sec-WebSocket-Version", "13") - r.Header.Set("Sec-WebSocket-Key", "meow123") + r.Header.Set("Sec-WebSocket-Key", xrand.Base64(16)) _, err := Accept(w, r, nil) assert.Contains(t, err, `http.ResponseWriter does not implement http.Hijacker`) @@ -136,7 +137,7 @@ func TestAccept(t *testing.T) { r.Header.Set("Connection", "Upgrade") r.Header.Set("Upgrade", "websocket") r.Header.Set("Sec-WebSocket-Version", "13") - r.Header.Set("Sec-WebSocket-Key", "meow123") + r.Header.Set("Sec-WebSocket-Key", xrand.Base64(16)) _, err := Accept(w, r, nil) assert.Contains(t, err, `failed to hijack connection`) @@ -183,7 +184,7 @@ func Test_verifyClientHandshake(t *testing.T) { }, }, { - name: "badWebSocketKey", + name: "missingWebSocketKey", h: map[string]string{ "Connection": "Upgrade", "Upgrade": "websocket", @@ -191,13 +192,31 @@ func Test_verifyClientHandshake(t *testing.T) { "Sec-WebSocket-Key": "", }, }, + { + name: "shortWebSocketKey", + h: map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": xrand.Base64(15), + }, + }, + { + name: "invalidWebSocketKey", + h: map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": "notbase64", + }, + }, { name: "badHTTPVersion", h: map[string]string{ "Connection": "Upgrade", "Upgrade": "websocket", "Sec-WebSocket-Version": "13", - "Sec-WebSocket-Key": "meow123", + "Sec-WebSocket-Key": xrand.Base64(16), }, http1: true, }, @@ -207,7 +226,7 @@ func Test_verifyClientHandshake(t *testing.T) { "Connection": "keep-alive, Upgrade", "Upgrade": "websocket", "Sec-WebSocket-Version": "13", - "Sec-WebSocket-Key": "meow123", + "Sec-WebSocket-Key": xrand.Base64(16), }, success: true, }, diff --git a/internal/test/xrand/xrand.go b/internal/test/xrand/xrand.go index 8de1ede8..82064d5c 100644 --- a/internal/test/xrand/xrand.go +++ b/internal/test/xrand/xrand.go @@ -2,6 +2,7 @@ package xrand import ( "crypto/rand" + "encoding/base64" "fmt" "math/big" "strings" @@ -45,3 +46,7 @@ func Int(max int) int { } return int(x.Int64()) } + +func Base64(n int) string { + return base64.StdEncoding.EncodeToString(Bytes(n)) +} From f46da9af6d363b6a5fe10d09705acd9d933af644 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Dec 2022 14:18:41 -0600 Subject: [PATCH 2/5] Remove build tag at top of files --- accept.go | 1 - accept_test.go | 1 - 2 files changed, 2 deletions(-) diff --git a/accept.go b/accept.go index 2f4fb2eb..24c5dca3 100644 --- a/accept.go +++ b/accept.go @@ -1,4 +1,3 @@ -//go:build !js // +build !js package websocket diff --git a/accept_test.go b/accept_test.go index d0cc4878..270f62da 100644 --- a/accept_test.go +++ b/accept_test.go @@ -1,4 +1,3 @@ -//go:build !js // +build !js package websocket From 3233cb5a0622a6eba869e3d169e98d11e1f2688f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Dec 2022 14:27:27 -0600 Subject: [PATCH 3/5] Remove all leading and trailing whitespace --- accept.go | 3 +++ accept_test.go | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/accept.go b/accept.go index 24c5dca3..11b312d1 100644 --- a/accept.go +++ b/accept.go @@ -1,3 +1,4 @@ +//go:build !js // +build !js package websocket @@ -185,6 +186,8 @@ func verifyClientRequest(w http.ResponseWriter, r *http.Request) (errCode int, _ } websocketSecKey := r.Header.Get("Sec-WebSocket-Key") + // The RFC states to remove any leading or trailing whitespace. + websocketSecKey = strings.TrimSpace(websocketSecKey) if websocketSecKey == "" { return http.StatusBadRequest, errors.New("WebSocket protocol violation: missing Sec-WebSocket-Key") } diff --git a/accept_test.go b/accept_test.go index 270f62da..ba245c47 100644 --- a/accept_test.go +++ b/accept_test.go @@ -1,3 +1,4 @@ +//go:build !js // +build !js package websocket @@ -229,6 +230,16 @@ func Test_verifyClientHandshake(t *testing.T) { }, success: true, }, + { + name: "successSecKeyExtraSpace", + h: map[string]string{ + "Connection": "keep-alive, Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": " " + xrand.Base64(16) + " ", + }, + success: true, + }, } for _, tc := range testCases { From 309e088c4f56983e20ae732aa59669f85144818f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Dec 2022 14:46:50 -0600 Subject: [PATCH 4/5] Handle multiple sec-websocket-keys --- accept.go | 12 ++++++++---- accept_test.go | 22 +++++++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/accept.go b/accept.go index 11b312d1..285b3103 100644 --- a/accept.go +++ b/accept.go @@ -185,13 +185,17 @@ func verifyClientRequest(w http.ResponseWriter, r *http.Request) (errCode int, _ return http.StatusBadRequest, fmt.Errorf("unsupported WebSocket protocol version (only 13 is supported): %q", r.Header.Get("Sec-WebSocket-Version")) } - websocketSecKey := r.Header.Get("Sec-WebSocket-Key") - // The RFC states to remove any leading or trailing whitespace. - websocketSecKey = strings.TrimSpace(websocketSecKey) - if websocketSecKey == "" { + websocketSecKeys := r.Header.Values("Sec-WebSocket-Key") + if len(websocketSecKeys) == 0 { return http.StatusBadRequest, errors.New("WebSocket protocol violation: missing Sec-WebSocket-Key") } + if len(websocketSecKeys) > 1 { + return http.StatusBadRequest, errors.New("WebSocket protocol violation: multiple Sec-WebSocket-Key headers") + } + + // The RFC states to remove any leading or trailing whitespace. + websocketSecKey := strings.TrimSpace(websocketSecKeys[0]) if v, err := base64.StdEncoding.DecodeString(websocketSecKey); err != nil || len(v) != 16 { return http.StatusBadRequest, fmt.Errorf("WebSocket protocol violation: invalid Sec-WebSocket-Key %q, must be a 16 byte base64 encoded string", websocketSecKey) } diff --git a/accept_test.go b/accept_test.go index ba245c47..5b37dfc8 100644 --- a/accept_test.go +++ b/accept_test.go @@ -185,6 +185,14 @@ func Test_verifyClientHandshake(t *testing.T) { }, { name: "missingWebSocketKey", + h: map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + }, + }, + { + name: "emptyWebSocketKey", h: map[string]string{ "Connection": "Upgrade", "Upgrade": "websocket", @@ -210,6 +218,18 @@ func Test_verifyClientHandshake(t *testing.T) { "Sec-WebSocket-Key": "notbase64", }, }, + { + name: "extraWebSocketKey", + h: map[string]string{ + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + // Kinda cheeky, but http headers are case-insensitive. + // If 2 sec keys are present, this is a failure condition. + "Sec-WebSocket-Key": xrand.Base64(16), + "sec-webSocket-key": xrand.Base64(16), + }, + }, { name: "badHTTPVersion", h: map[string]string{ @@ -256,7 +276,7 @@ func Test_verifyClientHandshake(t *testing.T) { } for k, v := range tc.h { - r.Header.Set(k, v) + r.Header.Add(k, v) } _, err := verifyClientRequest(httptest.NewRecorder(), r) From 305eab9a519e2d563636ea1ea7b0b82377acf2fb Mon Sep 17 00:00:00 2001 From: Anmol Sethi Date: Wed, 18 Oct 2023 21:31:59 -0700 Subject: [PATCH 5/5] misc: Format and compile #360 --- accept_test.go | 4 ++-- internal/test/xrand/xrand.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/accept_test.go b/accept_test.go index 5b37dfc8..7cb85d0f 100644 --- a/accept_test.go +++ b/accept_test.go @@ -9,11 +9,11 @@ import ( "net" "net/http" "net/http/httptest" - "nhooyr.io/websocket/internal/test/xrand" "strings" "testing" "nhooyr.io/websocket/internal/test/assert" + "nhooyr.io/websocket/internal/test/xrand" ) func TestAccept(t *testing.T) { @@ -68,7 +68,7 @@ func TestAccept(t *testing.T) { r.Header.Set("Connection", "Upgrade") r.Header.Set("Upgrade", "websocket") r.Header.Set("Sec-WebSocket-Version", "13") - r.Header.Set("Sec-WebSocket-Key", "meow123") + r.Header.Set("Sec-WebSocket-Key", xrand.Base64(16)) r.Header.Set("Sec-WebSocket-Extensions", extensions) return r } diff --git a/internal/test/xrand/xrand.go b/internal/test/xrand/xrand.go index 82064d5c..9bfb39ce 100644 --- a/internal/test/xrand/xrand.go +++ b/internal/test/xrand/xrand.go @@ -47,6 +47,7 @@ func Int(max int) int { return int(x.Int64()) } +// Base64 returns a randomly generated base64 string of length n. func Base64(n int) string { return base64.StdEncoding.EncodeToString(Bytes(n)) }