Skip to content

Conversation

hirasawayuki
Copy link
Contributor

@hirasawayuki hirasawayuki commented Jan 2, 2022

Fixes issue: #617

Added the following two fixes

  • Check that Sec-websocket-key is encoded as base64
  • Check that Sec-Websocket-Key is encoded from 16 bytes long

RFC6455 states the following:

A |Sec-WebSocket-Key| header field with a base64-encoded value that, when decoded, is 16 bytes in length.

@hirasawayuki hirasawayuki force-pushed the check-sec-websocket-key branch from c0eac79 to 4a6464d Compare January 3, 2022 07:16
@hirasawayuki
Copy link
Contributor Author

hirasawayuki commented Jan 3, 2022

@garyburd
Thanks for the review! I checked the ws(nodejs library), and it seems to return HTTP Status 400 if it doesn't match regular expression of encoded 16-byte string. (/^[+/0-9A-Za-z]{22}==$/)
So I saw no problem in rejecting an invalid key.

https://github.com/websockets/ws/blob/5edf1f4a1b1750109c1bb56eff7ad78902eee7dc/lib/websocket-server.js#L18
https://github.com/websockets/ws/blob/5edf1f4a1b1750109c1bb56eff7ad78902eee7dc/lib/websocket-server.js#L236-L245

@hirasawayuki hirasawayuki changed the title [bug] Add check for Sec-WebSocket-Key header Add check for Sec-WebSocket-Key header Jan 3, 2022
@hirasawayuki hirasawayuki force-pushed the check-sec-websocket-key branch from 4a6464d to 187c686 Compare January 4, 2022 03:17
@hirasawayuki
Copy link
Contributor Author

hirasawayuki commented Jan 4, 2022

@garyburd
I'll let you be the judge! 😃

@ghost ghost merged commit 69d0eb9 into gorilla:master Feb 16, 2022
@hirasawayuki hirasawayuki deleted the check-sec-websocket-key branch February 23, 2022 13:36
@andrewhodel
Copy link

There is a maximum limit of the incoming data per the base64 limit.

You should add an error from the upgrader if the size is exceeded for two reasons:

  1. The response isn't unknowingly handled in the module code like go's http 301 when there is a .. in the url path.

  2. The developer gets to decide how strict to be instead of a lower level structure that doesn't participate in these cost decisions.

@andrewhodel
Copy link

The base64 alphabet has a limit per byte with regards to representation size.

nono referenced this pull request in cozy/cozy-stack Nov 6, 2023
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gorilla/websocket](https://togithub.com/gorilla/websocket) | require | patch | `v1.5.0` -> `v1.5.1` | --- ### Release Notes <details> <summary>gorilla/websocket (github.com/gorilla/websocket)</summary> ### [`v1.5.1`](https://togithub.com/gorilla/websocket/releases/tag/v1.5.1) [Compare Source](https://togithub.com/gorilla/websocket/compare/v1.5.0...v1.5.1) #### What's Changed - Add check for Sec-WebSocket-Key header by [@&#8203;hirasawayuki](https://togithub.com/hirasawayuki) in [https://github.com/gorilla/websocket/pull/752](https://togithub.com/gorilla/websocket/pull/752) - Changed the method name UnderlyingConn to NetConn by [@&#8203;JWSong](https://togithub.com/JWSong) in [https://github.com/gorilla/websocket/pull/773](https://togithub.com/gorilla/websocket/pull/773) - remove all versions < 1.16 and add 1.18 by [@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) in [https://github.com/gorilla/websocket/pull/793](https://togithub.com/gorilla/websocket/pull/793) - Check for and report bad protocol in TLSClientConfig.NextProtos by [@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) in [https://github.com/gorilla/websocket/pull/788](https://togithub.com/gorilla/websocket/pull/788) - check err before GotConn for trace by [@&#8203;junnplus](https://togithub.com/junnplus) in [https://github.com/gorilla/websocket/pull/798](https://togithub.com/gorilla/websocket/pull/798) - Update README.md by [@&#8203;coreydaley](https://togithub.com/coreydaley) in [https://github.com/gorilla/websocket/pull/839](https://togithub.com/gorilla/websocket/pull/839) - Correct way to save memory using write buffer pool and freeing net.http default buffers by [@&#8203;FMLS](https://togithub.com/FMLS) in [https://github.com/gorilla/websocket/pull/761](https://togithub.com/gorilla/websocket/pull/761) - Update go version & add verification/testing tools by [@&#8203;coreydaley](https://togithub.com/coreydaley) in [https://github.com/gorilla/websocket/pull/840](https://togithub.com/gorilla/websocket/pull/840) - update golang.org/x/net by [@&#8203;coreydaley](https://togithub.com/coreydaley) in [https://github.com/gorilla/websocket/pull/856](https://togithub.com/gorilla/websocket/pull/856) - update GitHub workflows by [@&#8203;coreydaley](https://togithub.com/coreydaley) in [https://github.com/gorilla/websocket/pull/857](https://togithub.com/gorilla/websocket/pull/857) #### New Contributors - [@&#8203;hirasawayuki](https://togithub.com/hirasawayuki) made their first contribution in [https://github.com/gorilla/websocket/pull/752](https://togithub.com/gorilla/websocket/pull/752) - [@&#8203;JWSong](https://togithub.com/JWSong) made their first contribution in [https://github.com/gorilla/websocket/pull/773](https://togithub.com/gorilla/websocket/pull/773) - [@&#8203;ChannyClaus](https://togithub.com/ChannyClaus) made their first contribution in [https://github.com/gorilla/websocket/pull/793](https://togithub.com/gorilla/websocket/pull/793) - [@&#8203;junnplus](https://togithub.com/junnplus) made their first contribution in [https://github.com/gorilla/websocket/pull/798](https://togithub.com/gorilla/websocket/pull/798) - [@&#8203;coreydaley](https://togithub.com/coreydaley) made their first contribution in [https://github.com/gorilla/websocket/pull/839](https://togithub.com/gorilla/websocket/pull/839) - [@&#8203;FMLS](https://togithub.com/FMLS) made their first contribution in [https://github.com/gorilla/websocket/pull/761](https://togithub.com/gorilla/websocket/pull/761) **Full Changelog**: gorilla/websocket@v1.5.0...v1.5.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone Europe/Paris, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/cozy/cozy-stack). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
algitbot pushed a commit to alpinelinux/build-server-status that referenced this pull request May 5, 2024
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/gorilla/websocket](https://github.com/gorilla/websocket) | require | patch | `v1.5.0` -> `v1.5.1` | --- ### Release Notes <details> <summary>gorilla/websocket (github.com/gorilla/websocket)</summary> ### [`v1.5.1`](https://github.com/gorilla/websocket/releases/tag/v1.5.1) [Compare Source](gorilla/websocket@v1.5.0...v1.5.1) #### What's Changed - Add check for Sec-WebSocket-Key header by [@&#8203;hirasawayuki](https://github.com/hirasawayuki) in gorilla/websocket#752 - Changed the method name UnderlyingConn to NetConn by [@&#8203;JWSong](https://github.com/JWSong) in gorilla/websocket#773 - remove all versions < 1.16 and add 1.18 by [@&#8203;ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#793 - Check for and report bad protocol in TLSClientConfig.NextProtos by [@&#8203;ChannyClaus](https://github.com/ChannyClaus) in gorilla/websocket#788 - check err before GotConn for trace by [@&#8203;junnplus](https://github.com/junnplus) in gorilla/websocket#798 - Update README.md by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#839 - Correct way to save memory using write buffer pool and freeing net.http default buffers by [@&#8203;FMLS](https://github.com/FMLS) in gorilla/websocket#761 - Update go version & add verification/testing tools by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#840 - update golang.org/x/net by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#856 - update GitHub workflows by [@&#8203;coreydaley](https://github.com/coreydaley) in gorilla/websocket#857 #### New Contributors - [@&#8203;hirasawayuki](https://github.com/hirasawayuki) made their first contribution in gorilla/websocket#752 - [@&#8203;JWSong](https://github.com/JWSong) made their first contribution in gorilla/websocket#773 - [@&#8203;ChannyClaus](https://github.com/ChannyClaus) made their first contribution in gorilla/websocket#793 - [@&#8203;junnplus](https://github.com/junnplus) made their first contribution in gorilla/websocket#798 - [@&#8203;coreydaley](https://github.com/coreydaley) made their first contribution in gorilla/websocket#839 - [@&#8203;FMLS](https://github.com/FMLS) made their first contribution in gorilla/websocket#761 **Full Changelog**: gorilla/websocket@v1.5.0...v1.5.1 </details> --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- &nbsp; <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yODYuMSIsInVwZGF0ZWRJblZlciI6IjM3LjI4Ni4xIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbXX0=--> See merge request alpine/infra/build-server-status!9
@ghost ghost mentioned this pull request Aug 4, 2024
1 task
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants