From 9c4d31f5489c7f7e45113c31f032c43f640df4b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 1 Sep 2016 10:16:04 +0200 Subject: [PATCH] Signer errors --- Cargo.lock | 12 +++-- dapps/Cargo.toml | 1 + dapps/src/error_tpl.html | 2 +- dapps/src/lib.rs | 2 + dapps/src/router/mod.rs | 13 +++-- dapps/src/tests/authorization.rs | 2 +- dapps/src/tests/helpers.rs | 50 ++---------------- dapps/src/tests/validation.rs | 23 +++++++- devtools/src/http_client.rs | 64 +++++++++++++++++++++++ devtools/src/lib.rs | 1 + signer/Cargo.toml | 1 + signer/src/lib.rs | 10 ++-- signer/src/tests/mod.rs | 81 +++++++++++++++++++++++++++++ signer/src/ws_server/error_tpl.html | 21 ++++++++ signer/src/ws_server/mod.rs | 9 +++- signer/src/ws_server/session.rs | 14 +++-- 16 files changed, 236 insertions(+), 70 deletions(-) create mode 100644 devtools/src/http_client.rs create mode 100644 signer/src/tests/mod.rs create mode 100644 signer/src/ws_server/error_tpl.html diff --git a/Cargo.lock b/Cargo.lock index 79bb6dba9..b9ad5d6bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -290,6 +290,7 @@ version = "1.4.0" dependencies = [ "clippy 0.0.85 (registry+https://github.com/rust-lang/crates.io-index)", "ethabi 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "ethcore-devtools 1.4.0", "ethcore-rpc 1.4.0", "ethcore-util 1.4.0", "hyper 0.9.4 (git+https://github.com/ethcore/hyper)", @@ -455,6 +456,7 @@ version = "1.4.0" dependencies = [ "clippy 0.0.85 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", + "ethcore-devtools 1.4.0", "ethcore-io 1.4.0", "ethcore-rpc 1.4.0", "ethcore-util 1.4.0", @@ -1068,7 +1070,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "parity-dapps" version = "1.4.0" -source = "git+https://github.com/ethcore/parity-ui.git#e4dddf36e7c9fa5c6e746790119c71f67438784a" +source = "git+https://github.com/ethcore/parity-ui.git#be8754c3ccd142c43a20b82e9b1fba416b5b332a" dependencies = [ "aster 0.17.0 (registry+https://github.com/rust-lang/crates.io-index)", "glob 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1082,7 +1084,7 @@ dependencies = [ [[package]] name = "parity-dapps-home" version = "1.4.0" -source = "git+https://github.com/ethcore/parity-ui.git#e4dddf36e7c9fa5c6e746790119c71f67438784a" +source = "git+https://github.com/ethcore/parity-ui.git#be8754c3ccd142c43a20b82e9b1fba416b5b332a" dependencies = [ "parity-dapps 1.4.0 (git+https://github.com/ethcore/parity-ui.git)", ] @@ -1090,7 +1092,7 @@ dependencies = [ [[package]] name = "parity-dapps-signer" version = "1.4.0" -source = "git+https://github.com/ethcore/parity-ui.git#e4dddf36e7c9fa5c6e746790119c71f67438784a" +source = "git+https://github.com/ethcore/parity-ui.git#be8754c3ccd142c43a20b82e9b1fba416b5b332a" dependencies = [ "parity-dapps 1.4.0 (git+https://github.com/ethcore/parity-ui.git)", ] @@ -1098,7 +1100,7 @@ dependencies = [ [[package]] name = "parity-dapps-status" version = "1.4.0" -source = "git+https://github.com/ethcore/parity-ui.git#e4dddf36e7c9fa5c6e746790119c71f67438784a" +source = "git+https://github.com/ethcore/parity-ui.git#be8754c3ccd142c43a20b82e9b1fba416b5b332a" dependencies = [ "parity-dapps 1.4.0 (git+https://github.com/ethcore/parity-ui.git)", ] @@ -1106,7 +1108,7 @@ dependencies = [ [[package]] name = "parity-dapps-wallet" version = "1.4.0" -source = "git+https://github.com/ethcore/parity-ui.git#e4dddf36e7c9fa5c6e746790119c71f67438784a" +source = "git+https://github.com/ethcore/parity-ui.git#be8754c3ccd142c43a20b82e9b1fba416b5b332a" dependencies = [ "parity-dapps 1.4.0 (git+https://github.com/ethcore/parity-ui.git)", ] diff --git a/dapps/Cargo.toml b/dapps/Cargo.toml index 1f5a0c491..f2604f07f 100644 --- a/dapps/Cargo.toml +++ b/dapps/Cargo.toml @@ -23,6 +23,7 @@ serde_macros = { version = "0.7.0", optional = true } zip = { version = "0.1", default-features = false } ethabi = "0.2.1" linked-hash-map = "0.3" +ethcore-devtools = { path = "../devtools" } ethcore-rpc = { path = "../rpc" } ethcore-util = { path = "../util" } parity-dapps = { git = "https://github.com/ethcore/parity-ui.git", version = "1.4" } diff --git a/dapps/src/error_tpl.html b/dapps/src/error_tpl.html index 227764b9c..6551431a6 100644 --- a/dapps/src/error_tpl.html +++ b/dapps/src/error_tpl.html @@ -16,7 +16,7 @@

{details}

- {version} + {version}
diff --git a/dapps/src/lib.rs b/dapps/src/lib.rs index bc54b0f37..e24def0a1 100644 --- a/dapps/src/lib.rs +++ b/dapps/src/lib.rs @@ -61,6 +61,8 @@ extern crate parity_dapps; extern crate ethcore_rpc; extern crate ethcore_util as util; extern crate linked_hash_map; +#[cfg(test)] +extern crate ethcore_devtools as devtools; mod endpoint; mod apps; diff --git a/dapps/src/router/mod.rs b/dapps/src/router/mod.rs index 94d0a0fc0..9326917bf 100644 --- a/dapps/src/router/mod.rs +++ b/dapps/src/router/mod.rs @@ -55,9 +55,16 @@ pub struct Router { impl server::Handler for Router { fn on_request(&mut self, req: server::Request) -> Next { + + // Choose proper handler depending on path / domain + let url = extract_url(&req); + let endpoint = extract_endpoint(&url); + let is_utils = endpoint.1 == SpecialEndpoint::Utils; + // Validate Host header if let Some(ref hosts) = self.allowed_hosts { - if !host_validation::is_valid(&req, hosts, self.endpoints.keys().cloned().collect()) { + let is_valid = is_utils || host_validation::is_valid(&req, hosts, self.endpoints.keys().cloned().collect()); + if !is_valid { self.handler = host_validation::host_invalid_response(); return self.handler.on_request(req); } @@ -70,10 +77,6 @@ impl server::Handler for Router { return self.handler.on_request(req); } - // Choose proper handler depending on path / domain - let url = extract_url(&req); - let endpoint = extract_endpoint(&url); - self.handler = match endpoint { // First check special endpoints (ref path, ref endpoint) if self.special.contains_key(endpoint) => { diff --git a/dapps/src/tests/authorization.rs b/dapps/src/tests/authorization.rs index dceb194b7..e7a70c9cd 100644 --- a/dapps/src/tests/authorization.rs +++ b/dapps/src/tests/authorization.rs @@ -54,7 +54,7 @@ fn should_reject_on_invalid_auth() { // then assert_eq!(response.status, "HTTP/1.1 401 Unauthorized".to_owned()); - assert_eq!(response.body, "15\n

Unauthorized

\n0\n\n".to_owned()); + assert!(response.body.contains("Unauthorized")); assert_eq!(response.headers_raw.contains("WWW-Authenticate"), false); } diff --git a/dapps/src/tests/helpers.rs b/dapps/src/tests/helpers.rs index 84f638b34..1296162d2 100644 --- a/dapps/src/tests/helpers.rs +++ b/dapps/src/tests/helpers.rs @@ -15,16 +15,15 @@ // along with Parity. If not, see . use std::env; -use std::io::{Read, Write}; -use std::str::{self, Lines}; +use std::str; use std::sync::Arc; -use std::net::TcpStream; use rustc_serialize::hex::{ToHex, FromHex}; use ServerBuilder; use Server; use apps::urlhint::ContractClient; use util::{Bytes, Address, Mutex, ToPretty}; +use devtools::http_client; const REGISTRAR: &'static str = "8e4e9b13d4b45cb0befc93c3061b1408f67316b2"; const URLHINT: &'static str = "deadbeefcafe0000000000000000000000000000"; @@ -77,47 +76,6 @@ pub fn serve() -> Server { serve_hosts(None) } -pub struct Response { - pub status: String, - pub headers: Vec, - pub headers_raw: String, - pub body: String, +pub fn request(server: Server, request: &str) -> http_client::Response { + http_client::request(server.addr(), request) } - -pub fn read_block(lines: &mut Lines, all: bool) -> String { - let mut block = String::new(); - loop { - let line = lines.next(); - match line { - None => break, - Some("") if !all => break, - Some(v) => { - block.push_str(v); - block.push_str("\n"); - }, - } - } - block -} - -pub fn request(server: Server, request: &str) -> Response { - let mut req = TcpStream::connect(server.addr()).unwrap(); - req.write_all(request.as_bytes()).unwrap(); - - let mut response = String::new(); - req.read_to_string(&mut response).unwrap(); - - let mut lines = response.lines(); - let status = lines.next().unwrap().to_owned(); - let headers_raw = read_block(&mut lines, false); - let headers = headers_raw.split('\n').map(|v| v.to_owned()).collect(); - let body = read_block(&mut lines, true); - - Response { - status: status, - headers: headers, - headers_raw: headers_raw, - body: body, - } -} - diff --git a/dapps/src/tests/validation.rs b/dapps/src/tests/validation.rs index b233a07d8..b6a8c825c 100644 --- a/dapps/src/tests/validation.rs +++ b/dapps/src/tests/validation.rs @@ -34,7 +34,7 @@ fn should_reject_invalid_host() { // then assert_eq!(response.status, "HTTP/1.1 403 Forbidden".to_owned()); - assert_eq!(response.body, "85\n\n\t\t

Request with disallowed Host header has been blocked.

\n\t\t

Check the URL in your browser address bar.

\n\t\t\n0\n\n".to_owned()); + assert!(response.body.contains("Current host is disallowed")); } #[test] @@ -77,3 +77,24 @@ fn should_serve_dapps_domains() { assert_eq!(response.status, "HTTP/1.1 200 OK".to_owned()); } +#[test] +// NOTE [todr] This is required for error pages to be styled properly. +fn should_allow_parity_utils_even_on_invalid_domain() { + // given + let server = serve_hosts(Some(vec!["localhost:8080".into()])); + + // when + let response = request(server, + "\ + GET /parity-utils/styles.css HTTP/1.1\r\n\ + Host: 127.0.0.1:8080\r\n\ + Connection: close\r\n\ + \r\n\ + {} + " + ); + + // then + assert_eq!(response.status, "HTTP/1.1 200 OK".to_owned()); +} + diff --git a/devtools/src/http_client.rs b/devtools/src/http_client.rs new file mode 100644 index 000000000..27fa6ec50 --- /dev/null +++ b/devtools/src/http_client.rs @@ -0,0 +1,64 @@ +// Copyright 2015, 2016 Ethcore (UK) Ltd. +// This file is part of Parity. + +// Parity is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity. If not, see . + +use std::io::{Read, Write}; +use std::str::{self, Lines}; +use std::net::{TcpStream, SocketAddr}; + +pub struct Response { + pub status: String, + pub headers: Vec, + pub headers_raw: String, + pub body: String, +} + +pub fn read_block(lines: &mut Lines, all: bool) -> String { + let mut block = String::new(); + loop { + let line = lines.next(); + match line { + None => break, + Some("") if !all => break, + Some(v) => { + block.push_str(v); + block.push_str("\n"); + }, + } + } + block +} + +pub fn request(address: &SocketAddr, request: &str) -> Response { + let mut req = TcpStream::connect(address).unwrap(); + req.write_all(request.as_bytes()).unwrap(); + + let mut response = String::new(); + req.read_to_string(&mut response).unwrap(); + + let mut lines = response.lines(); + let status = lines.next().unwrap().to_owned(); + let headers_raw = read_block(&mut lines, false); + let headers = headers_raw.split('\n').map(|v| v.to_owned()).collect(); + let body = read_block(&mut lines, true); + + Response { + status: status, + headers: headers, + headers_raw: headers_raw, + body: body, + } +} + diff --git a/devtools/src/lib.rs b/devtools/src/lib.rs index 831e4315e..87457f7f3 100644 --- a/devtools/src/lib.rs +++ b/devtools/src/lib.rs @@ -22,6 +22,7 @@ extern crate rand; mod random_path; mod test_socket; mod stop_guard; +pub mod http_client; pub use random_path::*; pub use test_socket::*; diff --git a/signer/Cargo.toml b/signer/Cargo.toml index 5bb6325f1..cfcbedff8 100644 --- a/signer/Cargo.toml +++ b/signer/Cargo.toml @@ -19,6 +19,7 @@ ws = { git = "https://github.com/ethcore/ws-rs.git", branch = "mio-upstream-stab ethcore-util = { path = "../util" } ethcore-io = { path = "../util/io" } ethcore-rpc = { path = "../rpc" } +ethcore-devtools = { path = "../devtools" } parity-dapps-signer = { git = "https://github.com/ethcore/parity-ui.git", version = "1.4", optional = true} clippy = { version = "0.0.85", optional = true} diff --git a/signer/src/lib.rs b/signer/src/lib.rs index 45383c7a1..abe84a03e 100644 --- a/signer/src/lib.rs +++ b/signer/src/lib.rs @@ -54,15 +54,13 @@ extern crate jsonrpc_core; extern crate ws; #[cfg(feature = "ui")] extern crate parity_dapps_signer as signer; +#[cfg(test)] +extern crate ethcore_devtools as devtools; mod authcode_store; mod ws_server; +#[cfg(test)] +mod tests; pub use authcode_store::*; pub use ws_server::*; - -#[cfg(test)] -mod tests { - #[test] - fn should_work() {} -} diff --git a/signer/src/tests/mod.rs b/signer/src/tests/mod.rs new file mode 100644 index 000000000..eaed49de8 --- /dev/null +++ b/signer/src/tests/mod.rs @@ -0,0 +1,81 @@ +// Copyright 2015, 2016 Ethcore (UK) Ltd. +// This file is part of Parity. + +// Parity is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity. If not, see . + +use std::env; +use std::thread; +use std::time::Duration; +use std::sync::Arc; +use devtools::http_client; +use rpc::ConfirmationsQueue; +use rand; + +use ServerBuilder; +use Server; + +pub fn serve() -> Server { + let queue = Arc::new(ConfirmationsQueue::default()); + let builder = ServerBuilder::new(queue, env::temp_dir()); + let port = 35000 + rand::random::() % 10000; + let res = builder.start(format!("127.0.0.1:{}", port).parse().unwrap()).unwrap(); + thread::sleep(Duration::from_millis(25)); + res +} + +pub fn request(server: Server, request: &str) -> http_client::Response { + http_client::request(server.addr(), request) +} + +#[test] +fn should_reject_invalid_host() { + // given + let server = serve(); + + // when + let response = request(server, + "\ + GET / HTTP/1.1\r\n\ + Host: test:8180\r\n\ + Connection: close\r\n\ + \r\n\ + {} + " + ); + + // then + assert_eq!(response.status, "HTTP/1.1 403 FORBIDDEN".to_owned()); + assert!(response.body.contains("URL Blocked")); +} + +#[test] +fn should_serve_styles_even_on_disallowed_domain() { + // given + let server = serve(); + + // when + let response = request(server, + "\ + GET /styles.css HTTP/1.1\r\n\ + Host: test:8180\r\n\ + Connection: close\r\n\ + \r\n\ + {} + " + ); + + // then + assert_eq!(response.status, "HTTP/1.1 200 OK".to_owned()); +} + diff --git a/signer/src/ws_server/error_tpl.html b/signer/src/ws_server/error_tpl.html new file mode 100644 index 000000000..04a0f3c30 --- /dev/null +++ b/signer/src/ws_server/error_tpl.html @@ -0,0 +1,21 @@ + + + + + + {meta} + {title} + + + +
+
+

{title}

+

{message}

+

{details}

+
+
+ {version} +
+ + diff --git a/signer/src/ws_server/mod.rs b/signer/src/ws_server/mod.rs index 4ebe96bfa..6d332adbe 100644 --- a/signer/src/ws_server/mod.rs +++ b/signer/src/ws_server/mod.rs @@ -93,9 +93,15 @@ pub struct Server { broadcaster_handle: Option>, queue: Arc, panic_handler: Arc, + addr: SocketAddr, } impl Server { + /// Returns the address this server is listening on + pub fn addr(&self) -> &SocketAddr { + &self.addr + } + /// Starts a new `WebSocket` server in separate thread. /// Returns a `Server` handle which closes the server when droped. fn start(addr: SocketAddr, handler: Arc, queue: Arc, authcodes_path: PathBuf, skip_origin_validation: bool) -> Result { @@ -121,7 +127,7 @@ impl Server { // Spawn a thread with event loop let handle = thread::spawn(move || { ph.catch_panic(move || { - match ws.listen(addr).map_err(ServerError::from) { + match ws.listen(addr.clone()).map_err(ServerError::from) { Err(ServerError::IoError(io)) => die(format!( "Signer: Could not start listening on specified address. Make sure that no other instance is running on Signer's port. Details: {:?}", io @@ -158,6 +164,7 @@ impl Server { broadcaster_handle: Some(broadcaster_handle), queue: queue, panic_handler: panic_handler, + addr: addr, }) } } diff --git a/signer/src/ws_server/session.rs b/signer/src/ws_server/session.rs index 34097c24d..b53f47db8 100644 --- a/signer/src/ws_server/session.rs +++ b/signer/src/ws_server/session.rs @@ -107,10 +107,15 @@ impl ws::Handler for Session { fn on_request(&mut self, req: &ws::Request) -> ws::Result<(ws::Response)> { let origin = req.header("origin").or_else(|| req.header("Origin")).map(|x| &x[..]); let host = req.header("host").or_else(|| req.header("Host")).map(|x| &x[..]); + // Styles file is allowed for error pages to display nicely. + let is_styles_file = req.resource() == "/styles.css"; // Check request origin and host header. if !self.skip_origin_validation { - if !origin_is_allowed(&self.self_origin, origin) && !(origin.is_none() && origin_is_allowed(&self.self_origin, host)) { + let is_valid = origin_is_allowed(&self.self_origin, origin) || (origin.is_none() && origin_is_allowed(&self.self_origin, host)); + let is_valid = is_styles_file || is_valid; + + if !is_valid { warn!(target: "signer", "Blocked connection to Signer API from untrusted origin."); return Ok(error( ErrorType::Forbidden, @@ -121,8 +126,9 @@ impl ws::Handler for Session { } } - // Detect if it's a websocket request. - if req.header("sec-websocket-key").is_some() { + // Detect if it's a websocket request + // (styles file skips origin validation, so make sure to prevent WS connections on this resource) + if req.header("sec-websocket-key").is_some() && !is_styles_file { // Check authorization if !auth_is_valid(&self.authcodes_path, req.protocols()) { info!(target: "signer", "Unauthorized connection to Signer API blocked."); @@ -198,7 +204,7 @@ enum ErrorType { fn error(error: ErrorType, title: &str, message: &str, details: Option<&str>) -> ws::Response { let content = format!( - include_str!("../../../dapps/src/error_tpl.html"), + include_str!("./error_tpl.html"), title=title, meta="", message=message,