From c3de71d9361d22f2513c6ee9af0e9558b9dcdc56 Mon Sep 17 00:00:00 2001 From: James Gilles Date: Tue, 31 Dec 2024 18:35:04 -0500 Subject: [PATCH] Fix critical security vulnerability in automigration code --- crates/schema/src/auto_migrate.rs | 52 +++++++++++++------ .../schema/src/auto_migrate/pretty_print.rs | 21 +++++++- crates/schema/src/def.rs | 1 + 3 files changed, 57 insertions(+), 17 deletions(-) diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index fffcff24c57..7ec28446e5a 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -43,7 +43,7 @@ pub struct AutoMigratePlan<'def> { /// There is also an implied check: that the schema in the database is compatible with the old ModuleDef. pub prechecks: Vec>, /// The migration steps to perform. - /// Order should not matter, as the steps are independent. + /// Order matters: `Remove`s of a particular `Def` must be ordered before `Add`s. pub steps: Vec>, } @@ -59,29 +59,46 @@ pub enum AutoMigratePrecheck<'def> { /// A step in an automatic migration. #[derive(PartialEq, Eq, Debug, PartialOrd, Ord)] pub enum AutoMigrateStep<'def> { + // It is important FOR CORRECTNESS that `Remove` variants are declared before `Add` variants in this enum! + // + // The ordering is used to sort the steps of an auto-migration. + // If adds go before removes, the following can occur: + // + // 1. `AddIndex("my_special_boy)` + // 2. `RemoveIndex("my_special_boy)` + // + // You see the problem. + // + // For now, we just ensure that we declare all `Remove` variants before `Add` variants + // and let `#[derive(PartialOrd)]` take care of the rest. + // TODO: when this enum is made serializable, a more durable fix will be needed here. + // Probably we will want to have separate arrays of add and remove steps. + // + /// Remove an index. + RemoveIndex(::Key<'def>), + /// Remove a constraint. + RemoveConstraint(::Key<'def>), + /// Remove a sequence. + RemoveSequence(::Key<'def>), + /// Remove a schedule annotation from a table. + RemoveSchedule(::Key<'def>), + /// Remove a row-level security query. + RemoveRowLevelSecurity(::Key<'def>), + /// Add a table, including all indexes, constraints, and sequences. /// There will NOT be separate steps in the plan for adding indexes, constraints, and sequences. AddTable(::Key<'def>), /// Add an index. AddIndex(::Key<'def>), - /// Remove an index. - RemoveIndex(::Key<'def>), - /// Remove a constraint. - RemoveConstraint(::Key<'def>), /// Add a sequence. AddSequence(::Key<'def>), - /// Remove a sequence. - RemoveSequence(::Key<'def>), - /// Change the access of a table. - ChangeAccess(::Key<'def>), /// Add a schedule annotation to a table. AddSchedule(::Key<'def>), - /// Remove a schedule annotation from a table. - RemoveSchedule(::Key<'def>), /// Add a row-level security query. AddRowLevelSecurity(::Key<'def>), - /// Remove a row-level security query. - RemoveRowLevelSecurity(::Key<'def>), + + /// Change the access of a table. + ChangeAccess(::Key<'def>), } /// Something that might prevent an automatic migration. @@ -412,9 +429,14 @@ fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Id .collect_all_errors() } -// Because we can refer to many tables and fields on the row level-security query, we need to remove all of them, -// then add the new ones, instead of trying to track the graph of dependencies. fn auto_migrate_row_level_security(plan: &mut AutoMigratePlan) -> Result<()> { + // Because we can refer to many tables and fields on the row level-security query, we need to remove all of them, + // then add the new ones, instead of trying to track the graph of dependencies. + // When pretty-printing, steps to remove and re-add a row-level-security policy are not shown, + // since they are an implementation detail that will be surprising to users. + // TODO: change this to not add-and-remove unchanged policies, and hand the responsibility for reinitializing + // queries to the `core` crate instead. + // When you do this, you will need to update `pretty_print.rs`. for rls in plan.old.row_level_security() { plan.steps.push(AutoMigrateStep::RemoveRowLevelSecurity(rls.key())); } diff --git a/crates/schema/src/auto_migrate/pretty_print.rs b/crates/schema/src/auto_migrate/pretty_print.rs index 81b6821f728..650845189ad 100644 --- a/crates/schema/src/auto_migrate/pretty_print.rs +++ b/crates/schema/src/auto_migrate/pretty_print.rs @@ -6,7 +6,7 @@ use colored::{self, Colorize}; use lazy_static::lazy_static; use regex::Regex; use spacetimedb_lib::{ - db::raw_def::v9::{TableAccess, TableType}, + db::raw_def::v9::{RawRowLevelSecurityDefV9, TableAccess, TableType}, AlgebraicType, }; use spacetimedb_primitives::{ColId, ColList}; @@ -188,8 +188,9 @@ pub fn pretty_print(plan: &MigratePlan) -> Result { write!( outr, - "- {} auto-increment constraint on column {} of table {}", + "- {} auto-increment constraint {} on column {} of table {}", removed, + constraint_name(*sequence), column_name_from_id(table_def, sequence_def.column), table_name(&*table_def.name), )?; @@ -232,10 +233,26 @@ pub fn pretty_print(plan: &MigratePlan) -> Result { )?; } AutoMigrateStep::AddRowLevelSecurity(rls) => { + // Implementation detail: Row-level-security policies are always removed and re-added + // because the `core` crate needs to recompile some stuff. + // We hide this from the user. + if plan.old.lookup::(*rls) + == plan.new.lookup::(*rls) + { + continue; + } writeln!(outr, "- {} row level security policy:", created)?; writeln!(outr, " `{}`", rls.blue())?; } AutoMigrateStep::RemoveRowLevelSecurity(rls) => { + // Implementation detail: Row-level-security policies are always removed and re-added + // because the `core` crate needs to recompile some stuff. + // We hide this from the user. + if plan.old.lookup::(*rls) + == plan.new.lookup::(*rls) + { + continue; + } writeln!(outr, "- {} row level security policy:", removed)?; writeln!(outr, " `{}`", rls.blue())?; } diff --git a/crates/schema/src/def.rs b/crates/schema/src/def.rs index c43df56f361..f4020b9bb6c 100644 --- a/crates/schema/src/def.rs +++ b/crates/schema/src/def.rs @@ -122,6 +122,7 @@ pub struct ModuleDef { /// The row-level security policies. /// /// **Note**: Are only validated syntax-wise. + // TODO: for consistency, this should be changed to store a `RowLevelSecurityDef` instead. row_level_security_raw: HashMap, }