Skip to content

Commit

Permalink
sqld: reject ATTACH statements
Browse files Browse the repository at this point in the history
This adds support for rejecting `ATTACH` statements from dumps. `ATTACH`
statements should be considered dangerous in a multi-tenant setting.
With this commit any `ATTACH` statement inside a dump will be rejected
with a 400 status code and a message. In addition, any other sql errors
returned by sqlite will be returned as a 400. All other dump errors
through this code path (conn.with_raw) will return a 500 like before.
  • Loading branch information
LucioFranco committed Nov 25, 2024
1 parent a77f422 commit 3c62bd7
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 3 deletions.
5 changes: 4 additions & 1 deletion libsql-server/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,8 @@ pub enum LoadDumpError {
NoCommit,
#[error("Path is not a file")]
NotAFile,
#[error("The passed dump sql is invalid: {0}")]
InvalidSqlInput(String),
}

impl ResponseError for LoadDumpError {}
Expand All @@ -315,7 +317,8 @@ impl IntoResponse for &LoadDumpError {
| NoTxn
| NoCommit
| NotAFile
| DumpFilePathNotAbsolute => self.format_err(StatusCode::BAD_REQUEST),
| DumpFilePathNotAbsolute
| InvalidSqlInput(_) => self.format_err(StatusCode::BAD_REQUEST),
}
}
}
Expand Down
28 changes: 26 additions & 2 deletions libsql-server/src/namespace/configurator/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use futures::Stream;
use libsql_sys::EncryptionConfig;
use libsql_wal::io::StdIO;
use libsql_wal::registry::WalRegistry;
use rusqlite::hooks::{AuthAction, AuthContext, Authorization};
use tokio::io::AsyncBufReadExt as _;
use tokio::task::JoinSet;
use tokio_util::io::StreamReader;
Expand Down Expand Up @@ -328,8 +329,31 @@ where
line = tokio::task::spawn_blocking({
let conn = conn.clone();
move || -> crate::Result<String, LoadDumpError> {
conn.with_raw(|conn| conn.execute(&line, ())).map_err(|e| {
LoadDumpError::Internal(format!("line: {}, error: {}", line_id, e))
conn.with_raw(|conn| {
conn.authorizer(Some(|auth: AuthContext<'_>| match auth.action {
AuthAction::Attach { filename: _ } => Authorization::Deny,
_ => Authorization::Allow,
}));
conn.execute(&line, ())
})
.map_err(|e| match e {
rusqlite::Error::SqlInputError {
msg, sql, offset, ..
} => {
let msg = if sql.to_lowercase().contains("attach") {
format!(
"attach statements are not allowed in dumps, msg: {}, sql: {}, offset: {}",
msg,
sql,
offset
)
} else {
format!("msg: {}, sql: {}, offset: {}", msg, sql, offset)
};

LoadDumpError::InvalidSqlInput(msg)
}
e => LoadDumpError::Internal(format!("line: {}, error: {}", line_id, e)),
})?;
Ok(line)
}
Expand Down
73 changes: 73 additions & 0 deletions libsql-server/tests/namespaces/dumps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,76 @@ fn export_dump() {

sim.run().unwrap();
}

#[test]
fn load_dump_with_attach_rejected() {
const DUMP: &str = r#"
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE test (x);
INSERT INTO test VALUES(42);
ATTACH foo/bar.sql
COMMIT;"#;

let mut sim = Builder::new()
.simulation_duration(Duration::from_secs(1000))
.build();
let tmp = tempdir().unwrap();
let tmp_path = tmp.path().to_path_buf();

std::fs::write(tmp_path.join("dump.sql"), DUMP).unwrap();

make_primary(&mut sim, tmp.path().to_path_buf());

sim.client("client", async move {
let client = Client::new();

// path is not absolute is an error
let resp = client
.post(
"http://primary:9090/v1/namespaces/foo/create",
json!({ "dump_url": "file:dump.sql"}),
)
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::BAD_REQUEST);

// path doesn't exist is an error
let resp = client
.post(
"http://primary:9090/v1/namespaces/foo/create",
json!({ "dump_url": "file:/dump.sql"}),
)
.await
.unwrap();
assert_eq!(resp.status(), StatusCode::BAD_REQUEST);

let resp = client
.post(
"http://primary:9090/v1/namespaces/foo/create",
json!({ "dump_url": format!("file:{}", tmp_path.join("dump.sql").display())}),
)
.await
.unwrap();
assert_eq!(
resp.status(),
StatusCode::BAD_REQUEST,
"{}",
resp.json::<serde_json::Value>().await.unwrap_or_default()
);

assert_snapshot!(resp.body_string().await?);

let foo =
Database::open_remote_with_connector("http://foo.primary:8080", "", TurmoilConnector)?;
let foo_conn = foo.connect()?;

let res = foo_conn.query("select count(*) from test", ()).await;
// This should error since the dump should have failed!
assert!(res.is_err());

Ok(())
});

sim.run().unwrap();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
source: libsql-server/tests/namespaces/dumps.rs
expression: resp.body_string().await?
snapshot_kind: text
---
{"error":"The passed dump sql is invalid: attach statements are not allowed in dumps, msg: near \"COMMIT\": syntax error, sql: ATTACH foo/bar.sql\n COMMIT;, offset: 28"}

0 comments on commit 3c62bd7

Please sign in to comment.