Fix for #361 don't allow consecutive PASS commands

Cargo.toml

@@ -59,3 +59,4 @@ uuid = { version = "0.8.2", features = ["v4"] }
 pretty_assertions = "0.7.1"
 tokio = { version = "1.5.0", features = ["macros", "rt-multi-thread"]}
 unftp-sbe-fs = { path = "../libunftp/crates/unftp-sbe-fs"}
+unftp-auth-jsonfile = { path = "../libunftp/crates/unftp-auth-jsonfile"}

src/server/controlchan/commands/user.rs

@@ -62,7 +62,7 @@ where
                     Err(_e) => Ok(Reply::new(ReplyCode::NotLoggedIn, "Invalid credentials")),
                 }
             }
-            (SessionState::New, None, _) | (SessionState::WaitPass, None, _) | (SessionState::New, Some(_), false) => {
+            (SessionState::New, None, _) | (SessionState::New, Some(_), false) => {
                 let user = std::str::from_utf8(&self.username)?;
                 session.username = Some(user.to_string());
                 session.state = SessionState::WaitPass;

src/server/controlchan/control_loop.rs

@@ -351,7 +351,11 @@ where
                 session.state = WaitCmd;
                 Ok(Reply::new(ReplyCode::UserLoggedIn, "User logged in, proceed"))
             }
-            AuthFailed => Ok(Reply::new(ReplyCode::NotLoggedIn, "Authentication failed")),
+            AuthFailed => {
+                let mut session = self.session.lock().await;
+                session.state = New; // According to RFC 959, a PASS command MUST precede a USER command
+                Ok(Reply::new(ReplyCode::NotLoggedIn, "Authentication failed"))
+            }
             StorageError(error_type) => match error_type.kind() {
                 ErrorKind::ExceededStorageAllocationError => Ok(Reply::new(ReplyCode::ExceededStorageAllocation, "Exceeded storage allocation")),
                 ErrorKind::FileNameNotAllowedError => Ok(Reply::new(ReplyCode::BadFileName, "File name not allowed")),

tests/common.rs

@@ -0,0 +1,47 @@
+use lazy_static::*;
+use std::sync::Arc;
+use tokio::sync::Mutex;
+use unftp_auth_jsonfile::JsonFileAuthenticator;
+use unftp_sbe_fs::ServerExt;
+
+lazy_static! {
+    static ref CONSUMERS: Arc<Mutex<i32>> = Arc::new(Mutex::new(0));
+}
+
+pub async fn run_with_json_auth() {
+    let addr = "127.0.0.1:2121";
+    let server = libunftp::Server::with_fs(std::env::temp_dir())
+        .authenticator(Arc::new(
+            JsonFileAuthenticator::from_json("[{\"username\":\"test\",\"password\":\"test\"}]").unwrap(),
+        ))
+        .greeting("Welcome test");
+    //    println!("Starting ftp server on {}", addr);
+    server.listen(addr).await.unwrap();
+}
+
+pub async fn initialize() {
+    let count = Arc::clone(&CONSUMERS);
+    let mut lock = count.lock().await;
+    *lock += 1;
+    if *lock == 1 {
+        tokio::spawn(run_with_json_auth());
+    }
+    drop(lock);
+}
+
+pub async fn finalize() {
+    let count = Arc::clone(&CONSUMERS);
+    let mut lock = count.lock().await;
+    *lock -= 1;
+    drop(lock);
+    loop {
+        let lock = count.lock().await;
+        if *lock > 0 {
+            drop(lock);
+            tokio::time::sleep(std::time::Duration::new(1, 0)).await;
+        } else {
+            drop(lock);
+            break;
+        }
+    }
+}

tests/pass_security.rs

@@ -0,0 +1,114 @@
+pub mod common;
+use std::io::Error;
+use tokio::net::TcpStream;
+
+async fn read_from_server<'a>(buffer: &'a mut Vec<u8>, stream: &TcpStream) -> &'a str {
+    loop {
+        stream.readable().await.unwrap();
+        let n = match stream.try_read(buffer) {
+            Ok(n) => n,
+            Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
+                continue;
+            }
+            Err(e) => panic!("{}", e),
+        };
+        return std::str::from_utf8(&buffer[0..n]).unwrap();
+    }
+}
+
+async fn send_to_server(buffer: &str, stream: &TcpStream) {
+    loop {
+        stream.writable().await.unwrap();
+
+        match stream.try_write(buffer.as_bytes()) {
+            Ok(_) => break,
+            Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
+                continue;
+            }
+            Err(e) => panic!("{}", e),
+        };
+    }
+}
+
+async fn tcp_connect() -> Result<TcpStream, Error> {
+    let mut errcount: i32 = 0;
+    loop {
+        match TcpStream::connect("127.0.0.1:2121").await {
+            Ok(s) => return Ok(s),
+            Err(e) => {
+                if errcount > 2 {
+                    return Err(e);
+                }
+                errcount += 1;
+                tokio::time::sleep(std::time::Duration::new(1, 0)).await;
+                continue;
+            }
+        }
+    }
+}
+
+#[tokio::test(flavor = "current_thread")]
+async fn test_pass_command_successful_login() {
+    common::initialize().await;
+
+    let stream = tcp_connect().await.unwrap();
+
+    let mut buffer = vec![0_u8; 1024];
+
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "220 Welcome test\r\n");
+
+    send_to_server("USER test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "331 Password Required\r\n");
+
+    send_to_server("PASS test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "230 User logged in, proceed\r\n");
+
+    common::finalize().await;
+}
+
+#[tokio::test(flavor = "current_thread")]
+async fn test_pass_followed_by_pass_invalid() {
+    common::initialize().await;
+
+    let stream = tcp_connect().await.unwrap();
+
+    let mut buffer = vec![0_u8; 1024];
+
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "220 Welcome test\r\n");
+
+    send_to_server("USER test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "331 Password Required\r\n");
+
+    send_to_server("PASS wrong_password\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "530 Authentication failed\r\n");
+
+    send_to_server("PASS test\r\n", &stream).await;
+    assert!(read_from_server(&mut buffer, &stream).await.starts_with("503"));
+
+    common::finalize().await;
+}
+
+#[tokio::test(flavor = "current_thread")]
+async fn test_pass_preceeds_user_valid() {
+    common::initialize().await;
+
+    let stream = tcp_connect().await.unwrap();
+
+    let mut buffer = vec![0_u8; 1024];
+
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "220 Welcome test\r\n");
+
+    send_to_server("USER test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "331 Password Required\r\n");
+
+    send_to_server("PASS wrong_password\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "530 Authentication failed\r\n");
+
+    send_to_server("USER test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "331 Password Required\r\n");
+
+    send_to_server("PASS test\r\n", &stream).await;
+    assert_eq!(read_from_server(&mut buffer, &stream).await, "230 User logged in, proceed\r\n");
+
+    common::finalize().await;
+}