Skip to content

Commit d3be42e

Browse files
straight-shootasdogruyol
straight-shoota
authored and
sdogruyol
committedApr 27, 2018
Improve WebSocket handshake validation (#5327)
·
1.16.10.25.0
1 parent 6717e0b commit d3be42e

File tree

4 files changed

+163
-21
lines changed

4 files changed

+163
-21
lines changed
 

Diff for: ‎spec/std/http/server/handlers/websocket_handler_spec.cr

+72-8
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,10 @@ describe HTTP::WebSocketHandler do
2222
io = IO::Memory.new
2323

2424
headers = HTTP::Headers{
25-
"Upgrade" => "WS",
26-
"Connection" => "Upgrade",
27-
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
25+
"Upgrade" => "WS",
26+
"Connection" => "Upgrade",
27+
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
28+
"Sec-WebSocket-Version" => "13",
2829
}
2930
request = HTTP::Request.new("GET", "/", headers: headers)
3031
response = HTTP::Server::Response.new(io)
@@ -47,6 +48,7 @@ describe HTTP::WebSocketHandler do
4748
"Upgrade" => "websocket",
4849
"Connection" => {{connection}},
4950
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
51+
"Sec-WebSocket-Version" => "13",
5052
}
5153
request = HTTP::Request.new("GET", "/", headers: headers)
5254
response = HTTP::Server::Response.new(io)
@@ -63,16 +65,17 @@ describe HTTP::WebSocketHandler do
6365

6466
response.close
6567

66-
io.to_s.should eq("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-Websocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")
68+
io.to_s.should eq("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")
6769
end
6870
{% end %}
6971

7072
it "gives upgrade response for case-insensitive 'WebSocket' upgrade request" do
7173
io = IO::Memory.new
7274
headers = HTTP::Headers{
73-
"Upgrade" => "WebSocket",
74-
"Connection" => "Upgrade",
75-
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
75+
"Upgrade" => "WebSocket",
76+
"Connection" => "Upgrade",
77+
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
78+
"Sec-WebSocket-Version" => "13",
7679
}
7780
request = HTTP::Request.new("GET", "/", headers: headers)
7881
response = HTTP::Server::Response.new(io)
@@ -89,6 +92,67 @@ describe HTTP::WebSocketHandler do
8992

9093
response.close
9194

92-
io.to_s.should eq("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-Websocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")
95+
io.to_s.should eq("HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")
96+
end
97+
98+
it "returns bad request if Sec-WebSocket-Key is missing" do
99+
io = IO::Memory.new
100+
101+
headers = HTTP::Headers{
102+
"Upgrade" => "websocket",
103+
"Connection" => "Upgrade",
104+
"Sec-WebSocket-Version" => "13",
105+
}
106+
request = HTTP::Request.new("GET", "/", headers: headers)
107+
response = HTTP::Server::Response.new(io)
108+
context = HTTP::Server::Context.new(request, response)
109+
110+
handler = HTTP::WebSocketHandler.new { }
111+
handler.call context
112+
113+
response.close
114+
115+
io.to_s.should eq("HTTP/1.1 400 Bad Request\r\nContent-Length: 0\r\n\r\n")
116+
end
117+
118+
it "returns upgrade required if Sec-WebSocket-Version is missing" do
119+
io = IO::Memory.new
120+
121+
headers = HTTP::Headers{
122+
"Upgrade" => "websocket",
123+
"Connection" => "Upgrade",
124+
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
125+
}
126+
request = HTTP::Request.new("GET", "/", headers: headers)
127+
response = HTTP::Server::Response.new(io)
128+
context = HTTP::Server::Context.new(request, response)
129+
130+
handler = HTTP::WebSocketHandler.new { }
131+
handler.call context
132+
133+
response.close
134+
135+
io.to_s.should eq("HTTP/1.1 426 Upgrade Required\r\nSec-WebSocket-Version: 13\r\nContent-Length: 0\r\n\r\n")
136+
end
137+
138+
it "returns upgrade required if Sec-WebSocket-Version is invalid" do
139+
io = IO::Memory.new
140+
141+
headers = HTTP::Headers{
142+
"Upgrade" => "websocket",
143+
"Connection" => "Upgrade",
144+
"Sec-WebSocket-Key" => "dGhlIHNhbXBsZSBub25jZQ==",
145+
"Sec-WebSocket-Version" => "12",
146+
}
147+
request = HTTP::Request.new("GET", "/", headers: headers)
148+
response = HTTP::Server::Response.new(io)
149+
context = HTTP::Server::Context.new(request, response)
150+
151+
handler = HTTP::WebSocketHandler.new { }
152+
handler.call context
153+
154+
response.close
155+
156+
io.to_s.should eq("HTTP/1.1 426 Upgrade Required\r\nSec-WebSocket-Version: 13\r\nContent-Length: 0\r\n\r\n")
93157
end
94158
end

Diff for: ‎spec/std/http/web_socket_spec.cr

+55
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,61 @@ describe HTTP::WebSocket do
374374
ws2.run
375375
end
376376

377+
it "handshake fails if server does not switch protocols" do
378+
port_chan = Channel(Int32).new
379+
spawn do
380+
http_ref = nil
381+
http_server = http_ref = HTTP::Server.new(0) do |context|
382+
context.response.status_code = 200
383+
end
384+
385+
http_server.bind
386+
port_chan.send(http_server.port)
387+
http_server.listen
388+
389+
http_ref.not_nil!.close
390+
end
391+
392+
listen_port = port_chan.receive
393+
394+
expect_raises(Socket::Error, "Handshake got denied. Status code was 200.") do
395+
HTTP::WebSocket::Protocol.new("127.0.0.1", port: listen_port, path: "/")
396+
end
397+
end
398+
399+
it "handshake fails if server does not verify Sec-WebSocket-Key" do
400+
port_chan = Channel(Int32).new
401+
spawn do
402+
http_ref = nil
403+
has_been_called = false
404+
405+
http_server = http_ref = HTTP::Server.new(0) do |context|
406+
response = context.response
407+
response.status_code = 101
408+
response.headers["Upgrade"] = "websocket"
409+
response.headers["Connection"] = "Upgrade"
410+
if has_been_called
411+
response.headers["Sec-WebSocket-Accept"] = "foobar"
412+
http_ref.not_nil!.close
413+
else
414+
has_been_called = true
415+
end
416+
end
417+
418+
http_server.bind
419+
port_chan.send(http_server.port)
420+
http_server.listen
421+
end
422+
423+
listen_port = port_chan.receive
424+
425+
2.times do
426+
expect_raises(Socket::Error, "Handshake got denied. Server did not verify WebSocket challenge.") do
427+
HTTP::WebSocket::Protocol.new("127.0.0.1", port: listen_port, path: "/")
428+
end
429+
end
430+
end
431+
377432
typeof(HTTP::WebSocket.new(URI.parse("ws://localhost")))
378433
typeof(HTTP::WebSocket.new("localhost", "/"))
379434
typeof(HTTP::WebSocket.new("ws://localhost"))

Diff for: ‎src/http/server/handlers/websocket_handler.cr

+17-9
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,28 @@ class HTTP::WebSocketHandler
1515

1616
def call(context)
1717
if websocket_upgrade_request? context.request
18-
key = context.request.headers["Sec-Websocket-Key"]
18+
response = context.response
19+
20+
version = context.request.headers["Sec-WebSocket-Version"]?
21+
unless version == WebSocket::Protocol::VERSION
22+
response.status_code = 426
23+
response.headers["Sec-WebSocket-Version"] = WebSocket::Protocol::VERSION
24+
return
25+
end
1926

20-
accept_code =
21-
{% if flag?(:without_openssl) %}
22-
Digest::SHA1.base64digest("#{key}258EAFA5-E914-47DA-95CA-C5AB0DC85B11")
23-
{% else %}
24-
Base64.strict_encode(OpenSSL::SHA1.hash("#{key}258EAFA5-E914-47DA-95CA-C5AB0DC85B11"))
25-
{% end %}
27+
key = context.request.headers["Sec-WebSocket-Key"]?
28+
29+
unless key
30+
response.status_code = 400
31+
return
32+
end
33+
34+
accept_code = WebSocket::Protocol.key_challenge(key)
2635

27-
response = context.response
2836
response.status_code = 101
2937
response.headers["Upgrade"] = "websocket"
3038
response.headers["Connection"] = "Upgrade"
31-
response.headers["Sec-Websocket-Accept"] = accept_code
39+
response.headers["Sec-WebSocket-Accept"] = accept_code
3240
response.upgrade do |io|
3341
ws_session = WebSocket.new(io)
3442
@proc.call(ws_session, context)

Diff for: ‎src/http/web_socket/protocol.cr

+19-4
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ class HTTP::WebSocket::Protocol
2626
end
2727

2828
MASK_BIT = 128_u8
29-
VERSION = 13
29+
VERSION = "13"
3030

3131
record PacketInfo,
3232
opcode : Opcode,
@@ -257,19 +257,26 @@ class HTTP::WebSocket::Protocol
257257
end
258258
{% end %}
259259

260+
random_key = Base64.strict_encode(StaticArray(UInt8, 16).new { rand(256).to_u8 })
261+
260262
headers["Host"] = "#{host}:#{port}"
261263
headers["Connection"] = "Upgrade"
262264
headers["Upgrade"] = "websocket"
263-
headers["Sec-WebSocket-Version"] = VERSION.to_s
264-
headers["Sec-WebSocket-Key"] = Base64.strict_encode(StaticArray(UInt8, 16).new { rand(256).to_u8 })
265+
headers["Sec-WebSocket-Version"] = VERSION
266+
headers["Sec-WebSocket-Key"] = random_key
265267

266268
path = "/" if path.empty?
267269
handshake = HTTP::Request.new("GET", path, headers)
268270
handshake.to_io(socket)
269271
socket.flush
270272
handshake_response = HTTP::Client::Response.from_io(socket)
271273
unless handshake_response.status_code == 101
272-
raise Socket::Error.new("Handshake got denied. Status code was #{handshake_response.status_code}")
274+
raise Socket::Error.new("Handshake got denied. Status code was #{handshake_response.status_code}.")
275+
end
276+
277+
challenge_response = Protocol.key_challenge(random_key)
278+
unless handshake_response.headers["Sec-WebSocket-Accept"]? == challenge_response
279+
raise Socket::Error.new("Handshake got denied. Server did not verify WebSocket challenge.")
273280
end
274281

275282
new(socket, masked: true)
@@ -285,4 +292,12 @@ class HTTP::WebSocket::Protocol
285292

286293
raise ArgumentError.new("No host or path specified which are required.")
287294
end
295+
296+
def self.key_challenge(key)
297+
{% if flag?(:without_openssl) %}
298+
Digest::SHA1.base64digest("#{key}258EAFA5-E914-47DA-95CA-C5AB0DC85B11")
299+
{% else %}
300+
Base64.strict_encode(OpenSSL::SHA1.hash("#{key}258EAFA5-E914-47DA-95CA-C5AB0DC85B11"))
301+
{% end %}
302+
end
288303
end

0 commit comments

Comments
 (0)
Please sign in to comment.