Fossil SCM

Improved documentation of the database write protection logic. Added undocumented SQL command db_protect() and db_protect_pop() to the "sql" command. Panic on a protection stack overflow.

drh 2020-08-21 15:05 sec2020
Commit 75deba73b5e00d6149b5f039b2622629679e5ddab835f80b639155f62585c5ed
Parent c9b9a77d592f031…
3 files changed +73 -16 +3 +40
~ src/db.c ~ src/printf.c ~ src/sqlcmd.c
M src/db.c
+73 -16
--- src/db.c
+++ src/db.c
@@ -355,44 +355,99 @@
355355
 ** Each of these routines pushes the previous protection mask onto
356356
 ** a finite-size stack. Each should be followed by a call to
357357
 ** db_protect_pop() to pop the stack and restore the protections that
358358
 ** existed prior to the call. The protection mask stack has a limited
359359
 ** depth, so take care not to next calls too deeply.
360
+**
361
+** About Database Write Protection
362
+** -------------------------------
363
+**
364
+** This is *not* a primary means of defending the application from
365
+** attack. Fossil should be secure even if this mechanism is disabled.
366
+** The purpose of database write protection is to provide an additional
367
+** layer of defense in case SQL injection bugs somehow slip into other
368
+** parts of the system. In other words, database write protection is
369
+** not primary defense but rather defense in depth.
370
+**
371
+** This mechanism mostly focuses on the USER table, to prevent an
372
+** attacker from giving themselves Admin privilegs, and on the
373
+** CONFIG table and specially "sensitive" settings such as
374
+** "diff-command" or "editor" that if compromised by an attacker
375
+** could lead to an RCE.
376
+**
377
+** By default, the USER and CONFIG tables are read-only. Various
378
+** subsystems that legitimately need to change those tables can
379
+** temporarily do so using:
380
+**
381
+** db_unprotect(PROTECT_xxx);
382
+** // make the legitmate changes here
383
+** db_protect_pop();
384
+**
385
+** Code that runs inside of reduced protections should be carefully
386
+** reviewed to ensure that it is harmless and not subject to SQL
387
+** injection.
388
+**
389
+** Read-only operations (such as many web pages like /timeline)
390
+** can invoke db_protect(PROTECT_ALL) to effectively make the database
391
+** read-only. TEMP tables (which are often used for these kinds of
392
+** pages) are still writable, however.
393
+**
394
+** The PROTECT_SENSITIVE protection is a subset of PROTECT_CONFIG
395
+** that blocks changes to all of the global_config table, but only
396
+** "sensitive" settings in the config table. PROTECT_SENSITIVE
397
+** relies on triggers and the protected_setting() SQL function to
398
+** prevent changes to sensitive settings.
399
+**
400
+** Additional Notes
401
+** ----------------
402
+**
403
+** Calls to routines like db_set() and db_unset() temporarily disable
404
+** the PROTECT_CONFIG protection. The assumption is that these calls
405
+** cannot be invoked by an SQL injection and are thus safe. Make sure
406
+** this is the case by always using a string literal as the name argument
407
+** to db_set() and db_unset() and friend, not a variable that might
408
+** be compromised by an attack.
360409
 */
361410
 void db_protect_only(unsigned flags){
362
- if( db.nProtect>=count(db.aProtect) ){
363
- fossil_fatal("too many db_protect() calls");
411
+ if( db.nProtect>=count(db.aProtect)-2 ){
412
+ fossil_panic("too many db_protect() calls");
364413
 }
365414
 db.aProtect[db.nProtect++] = db.protectMask;
366
- if( (flags & PROTECT_SENSITIVE)!=0
367
- && (db.protectMask & PROTECT_SENSITIVE)==0
368
- && db.bProtectTriggers==0
369
- ){
415
+ if( (flags & PROTECT_SENSITIVE)!=0 && db.bProtectTriggers==0 ){
416
+ /* Create the triggers needed to protect sensitive settings from
417
+ ** being created or modified the first time that PROTECT_SENSITIVE
418
+ ** is enabled. Deleting a sensitive setting is harmless, so there
419
+ ** is not trigger to block deletes. After being created once, the
420
+ ** triggers persist for the life of the database connection. */
370421
 db_multi_exec(
371
- "CREATE TEMP TRIGGER IF NOT EXISTS protect_1"
372
- " BEFORE INSERT ON config WHEN protected_setting(new.name)"
373
- " BEGIN SELECT raise(abort,'not authorized'); END;\n"
374
- "CREATE TEMP TRIGGER IF NOT EXISTS protect_2"
375
- " BEFORE UPDATE ON config WHEN protected_setting(new.name)"
376
- " BEGIN SELECT raise(abort,'not authorized'); END;\n"
422
+ "CREATE TEMP TRIGGER protect_1 BEFORE INSERT ON config"
423
+ " WHEN protected_setting(new.name) BEGIN"
424
+ " SELECT raise(abort,'not authorized');"
425
+ "END;\n"
426
+ "CREATE TEMP TRIGGER protect_2 BEFORE UPDATE ON config"
427
+ " WHEN protected_setting(new.name) BEGIN"
428
+ " SELECT raise(abort,'not authorized');"
429
+ "END;\n"
377430
 );
378431
 db.bProtectTriggers = 1;
379432
 }
380433
 db.protectMask = flags;
381434
 }
382435
 void db_protect(unsigned flags){
383436
 db_protect_only(db.protectMask | flags);
384437
 }
385438
 void db_unprotect(unsigned flags){
386
- if( db.nProtect>=count(db.aProtect) ){
387
- fossil_fatal("too many db_unprotect() calls");
439
+ if( db.nProtect>=count(db.aProtect)-2 ){
440
+ fossil_panic("too many db_unprotect() calls");
388441
 }
389442
 db.aProtect[db.nProtect++] = db.protectMask;
390443
 db.protectMask &= ~flags;
391444
 }
392445
 void db_protect_pop(void){
393
- if( db.nProtect<1 ) fossil_fatal("too many db_protect_pop() calls");
446
+ if( db.nProtect<1 ){
447
+ fossil_panic("too many db_protect_pop() calls");
448
+ }
394449
 db.protectMask = db.aProtect[--db.nProtect];
395450
 }
396451
397452
 /*
398453
 ** Verify that the desired database write pertections are in place.
@@ -409,11 +464,11 @@
409464
 ** Every Fossil database connection automatically registers the following
410465
 ** overarching authenticator callback, and leaves it registered for the
411466
 ** duration of the connection. This authenticator will call any
412467
 ** sub-authenticators that are registered using db_set_authorizer().
413468
 */
414
-static int db_top_authorizer(
469
+int db_top_authorizer(
415470
 void *pNotUsed,
416471
 int eCode,
417472
 const char *z0,
418473
 const char *z1,
419474
 const char *z2,
@@ -439,10 +494,12 @@
439494
 rc = SQLITE_DENY;
440495
 }
441496
 break;
442497
 }
443498
 case SQLITE_DROP_TEMP_TRIGGER: {
499
+ /* Do not allow the triggers that enforce PROTECT_SENSITIVE
500
+ ** to be dropped */
444501
 rc = SQLITE_DENY;
445502
 break;
446503
 }
447504
 }
448505
 if( db.xAuth && rc==SQLITE_OK ){
449506
--- src/db.c
+++ src/db.c
@@ -355,44 +355,99 @@
355 ** Each of these routines pushes the previous protection mask onto
356 ** a finite-size stack. Each should be followed by a call to
357 ** db_protect_pop() to pop the stack and restore the protections that
358 ** existed prior to the call. The protection mask stack has a limited
359 ** depth, so take care not to next calls too deeply.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
360 */
361 void db_protect_only(unsigned flags){
362 if( db.nProtect>=count(db.aProtect) ){
363 fossil_fatal("too many db_protect() calls");
364 }
365 db.aProtect[db.nProtect++] = db.protectMask;
366 if( (flags & PROTECT_SENSITIVE)!=0
367 && (db.protectMask & PROTECT_SENSITIVE)==0
368 && db.bProtectTriggers==0
369 ){
 
 
370 db_multi_exec(
371 "CREATE TEMP TRIGGER IF NOT EXISTS protect_1"
372 " BEFORE INSERT ON config WHEN protected_setting(new.name)"
373 " BEGIN SELECT raise(abort,'not authorized'); END;\n"
374 "CREATE TEMP TRIGGER IF NOT EXISTS protect_2"
375 " BEFORE UPDATE ON config WHEN protected_setting(new.name)"
376 " BEGIN SELECT raise(abort,'not authorized'); END;\n"
 
 
377 );
378 db.bProtectTriggers = 1;
379 }
380 db.protectMask = flags;
381 }
382 void db_protect(unsigned flags){
383 db_protect_only(db.protectMask | flags);
384 }
385 void db_unprotect(unsigned flags){
386 if( db.nProtect>=count(db.aProtect) ){
387 fossil_fatal("too many db_unprotect() calls");
388 }
389 db.aProtect[db.nProtect++] = db.protectMask;
390 db.protectMask &= ~flags;
391 }
392 void db_protect_pop(void){
393 if( db.nProtect<1 ) fossil_fatal("too many db_protect_pop() calls");
 
 
394 db.protectMask = db.aProtect[--db.nProtect];
395 }
396
397 /*
398 ** Verify that the desired database write pertections are in place.
@@ -409,11 +464,11 @@
409 ** Every Fossil database connection automatically registers the following
410 ** overarching authenticator callback, and leaves it registered for the
411 ** duration of the connection. This authenticator will call any
412 ** sub-authenticators that are registered using db_set_authorizer().
413 */
414 static int db_top_authorizer(
415 void *pNotUsed,
416 int eCode,
417 const char *z0,
418 const char *z1,
419 const char *z2,
@@ -439,10 +494,12 @@
439 rc = SQLITE_DENY;
440 }
441 break;
442 }
443 case SQLITE_DROP_TEMP_TRIGGER: {
 
 
444 rc = SQLITE_DENY;
445 break;
446 }
447 }
448 if( db.xAuth && rc==SQLITE_OK ){
449
--- src/db.c
+++ src/db.c
@@ -355,44 +355,99 @@
355 ** Each of these routines pushes the previous protection mask onto
356 ** a finite-size stack. Each should be followed by a call to
357 ** db_protect_pop() to pop the stack and restore the protections that
358 ** existed prior to the call. The protection mask stack has a limited
359 ** depth, so take care not to next calls too deeply.
360 **
361 ** About Database Write Protection
362 ** -------------------------------
363 **
364 ** This is *not* a primary means of defending the application from
365 ** attack. Fossil should be secure even if this mechanism is disabled.
366 ** The purpose of database write protection is to provide an additional
367 ** layer of defense in case SQL injection bugs somehow slip into other
368 ** parts of the system. In other words, database write protection is
369 ** not primary defense but rather defense in depth.
370 **
371 ** This mechanism mostly focuses on the USER table, to prevent an
372 ** attacker from giving themselves Admin privilegs, and on the
373 ** CONFIG table and specially "sensitive" settings such as
374 ** "diff-command" or "editor" that if compromised by an attacker
375 ** could lead to an RCE.
376 **
377 ** By default, the USER and CONFIG tables are read-only. Various
378 ** subsystems that legitimately need to change those tables can
379 ** temporarily do so using:
380 **
381 ** db_unprotect(PROTECT_xxx);
382 ** // make the legitmate changes here
383 ** db_protect_pop();
384 **
385 ** Code that runs inside of reduced protections should be carefully
386 ** reviewed to ensure that it is harmless and not subject to SQL
387 ** injection.
388 **
389 ** Read-only operations (such as many web pages like /timeline)
390 ** can invoke db_protect(PROTECT_ALL) to effectively make the database
391 ** read-only. TEMP tables (which are often used for these kinds of
392 ** pages) are still writable, however.
393 **
394 ** The PROTECT_SENSITIVE protection is a subset of PROTECT_CONFIG
395 ** that blocks changes to all of the global_config table, but only
396 ** "sensitive" settings in the config table. PROTECT_SENSITIVE
397 ** relies on triggers and the protected_setting() SQL function to
398 ** prevent changes to sensitive settings.
399 **
400 ** Additional Notes
401 ** ----------------
402 **
403 ** Calls to routines like db_set() and db_unset() temporarily disable
404 ** the PROTECT_CONFIG protection. The assumption is that these calls
405 ** cannot be invoked by an SQL injection and are thus safe. Make sure
406 ** this is the case by always using a string literal as the name argument
407 ** to db_set() and db_unset() and friend, not a variable that might
408 ** be compromised by an attack.
409 */
410 void db_protect_only(unsigned flags){
411 if( db.nProtect>=count(db.aProtect)-2 ){
412 fossil_panic("too many db_protect() calls");
413 }
414 db.aProtect[db.nProtect++] = db.protectMask;
415 if( (flags & PROTECT_SENSITIVE)!=0 && db.bProtectTriggers==0 ){
416 /* Create the triggers needed to protect sensitive settings from
417 ** being created or modified the first time that PROTECT_SENSITIVE
418 ** is enabled. Deleting a sensitive setting is harmless, so there
419 ** is not trigger to block deletes. After being created once, the
420 ** triggers persist for the life of the database connection. */
421 db_multi_exec(
422 "CREATE TEMP TRIGGER protect_1 BEFORE INSERT ON config"
423 " WHEN protected_setting(new.name) BEGIN"
424 " SELECT raise(abort,'not authorized');"
425 "END;\n"
426 "CREATE TEMP TRIGGER protect_2 BEFORE UPDATE ON config"
427 " WHEN protected_setting(new.name) BEGIN"
428 " SELECT raise(abort,'not authorized');"
429 "END;\n"
430 );
431 db.bProtectTriggers = 1;
432 }
433 db.protectMask = flags;
434 }
435 void db_protect(unsigned flags){
436 db_protect_only(db.protectMask | flags);
437 }
438 void db_unprotect(unsigned flags){
439 if( db.nProtect>=count(db.aProtect)-2 ){
440 fossil_panic("too many db_unprotect() calls");
441 }
442 db.aProtect[db.nProtect++] = db.protectMask;
443 db.protectMask &= ~flags;
444 }
445 void db_protect_pop(void){
446 if( db.nProtect<1 ){
447 fossil_panic("too many db_protect_pop() calls");
448 }
449 db.protectMask = db.aProtect[--db.nProtect];
450 }
451
452 /*
453 ** Verify that the desired database write pertections are in place.
@@ -409,11 +464,11 @@
464 ** Every Fossil database connection automatically registers the following
465 ** overarching authenticator callback, and leaves it registered for the
466 ** duration of the connection. This authenticator will call any
467 ** sub-authenticators that are registered using db_set_authorizer().
468 */
469 int db_top_authorizer(
470 void *pNotUsed,
471 int eCode,
472 const char *z0,
473 const char *z1,
474 const char *z2,
@@ -439,10 +494,12 @@
494 rc = SQLITE_DENY;
495 }
496 break;
497 }
498 case SQLITE_DROP_TEMP_TRIGGER: {
499 /* Do not allow the triggers that enforce PROTECT_SENSITIVE
500 ** to be dropped */
501 rc = SQLITE_DENY;
502 break;
503 }
504 }
505 if( db.xAuth && rc==SQLITE_OK ){
506
M src/printf.c
+3
--- src/printf.c
+++ src/printf.c
@@ -1148,12 +1148,15 @@
11481148
 rc = fossil_print_error(rc, z);
11491149
 abort();
11501150
 exit(rc);
11511151
 }
11521152
 NORETURN void fossil_fatal(const char *zFormat, ...){
1153
+ static int once = 0;
11531154
 char *z;
11541155
 int rc = 1;
1156
+ if( once ) exit(1);
1157
+ once = 1;
11551158
 va_list ap;
11561159
 mainInFatalError = 1;
11571160
 va_start(ap, zFormat);
11581161
 z = vmprintf(zFormat, ap);
11591162
 va_end(ap);
11601163
--- src/printf.c
+++ src/printf.c
@@ -1148,12 +1148,15 @@
1148 rc = fossil_print_error(rc, z);
1149 abort();
1150 exit(rc);
1151 }
1152 NORETURN void fossil_fatal(const char *zFormat, ...){
 
1153 char *z;
1154 int rc = 1;
 
 
1155 va_list ap;
1156 mainInFatalError = 1;
1157 va_start(ap, zFormat);
1158 z = vmprintf(zFormat, ap);
1159 va_end(ap);
1160
--- src/printf.c
+++ src/printf.c
@@ -1148,12 +1148,15 @@
1148 rc = fossil_print_error(rc, z);
1149 abort();
1150 exit(rc);
1151 }
1152 NORETURN void fossil_fatal(const char *zFormat, ...){
1153 static int once = 0;
1154 char *z;
1155 int rc = 1;
1156 if( once ) exit(1);
1157 once = 1;
1158 va_list ap;
1159 mainInFatalError = 1;
1160 va_start(ap, zFormat);
1161 z = vmprintf(zFormat, ap);
1162 va_end(ap);
1163
M src/sqlcmd.c
+40
--- src/sqlcmd.c
+++ src/sqlcmd.c
@@ -153,10 +153,44 @@
153153
 sqlcmd_decompress, 0, 0);
154154
 sqlite3_create_function(db, "gather_artifact_stats", 0, SQLITE_UTF8, 0,
155155
 sqlcmd_gather_artifact_stats, 0, 0);
156156
 return SQLITE_OK;
157157
 }
158
+
159
+/*
160
+** Undocumented test SQL functions:
161
+**
162
+** db_protect(X)
163
+** db_protect_pop(X)
164
+**
165
+** These invoke the corresponding C routines. Misuse may result in
166
+** an assertion fault.
167
+*/
168
+static void sqlcmd_db_protect(
169
+ sqlite3_context *context,
170
+ int argc,
171
+ sqlite3_value **argv
172
+){
173
+ unsigned mask = 0;
174
+ const char *z = (const char*)sqlite3_value_text(argv[0]);
175
+ if( sqlite3_stricmp(z,"user")==0 ) mask |= PROTECT_USER;
176
+ if( sqlite3_stricmp(z,"config")==0 ) mask |= PROTECT_CONFIG;
177
+ if( sqlite3_stricmp(z,"sensitive")==0 ) mask |= PROTECT_SENSITIVE;
178
+ if( sqlite3_stricmp(z,"readonly")==0 ) mask |= PROTECT_READONLY;
179
+ if( sqlite3_stricmp(z,"all")==0 ) mask |= PROTECT_ALL;
180
+ db_protect(mask);
181
+}
182
+static void sqlcmd_db_protect_pop(
183
+ sqlite3_context *context,
184
+ int argc,
185
+ sqlite3_value **argv
186
+){
187
+ db_protect_pop();
188
+}
189
+
190
+
191
+
158192
159193
 /*
160194
 ** This is the "automatic extension" initializer that runs right after
161195
 ** the connection to the repository database is opened. Set up the
162196
 ** database connection to be more useful to the human operator.
@@ -193,10 +227,16 @@
193227
 }
194228
 /* Arrange to trace close operations so that static prepared statements
195229
 ** will get cleaned up when the shell closes the database connection */
196230
 if( g.fSqlTrace ) mTrace |= SQLITE_TRACE_PROFILE;
197231
 sqlite3_trace_v2(db, mTrace, db_sql_trace, 0);
232
+ db_protect_only(PROTECT_NONE);
233
+ sqlite3_set_authorizer(db, db_top_authorizer, db);
234
+ sqlite3_create_function(db, "db_protect", 1, SQLITE_UTF8, 0,
235
+ sqlcmd_db_protect, 0, 0);
236
+ sqlite3_create_function(db, "db_protect_pop", 0, SQLITE_UTF8, 0,
237
+ sqlcmd_db_protect_pop, 0, 0);
198238
 return SQLITE_OK;
199239
 }
200240
201241
 /*
202242
 ** atexit() handler that cleans up global state modified by this module.
203243
--- src/sqlcmd.c
+++ src/sqlcmd.c
@@ -153,10 +153,44 @@
153 sqlcmd_decompress, 0, 0);
154 sqlite3_create_function(db, "gather_artifact_stats", 0, SQLITE_UTF8, 0,
155 sqlcmd_gather_artifact_stats, 0, 0);
156 return SQLITE_OK;
157 }
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
158
159 /*
160 ** This is the "automatic extension" initializer that runs right after
161 ** the connection to the repository database is opened. Set up the
162 ** database connection to be more useful to the human operator.
@@ -193,10 +227,16 @@
193 }
194 /* Arrange to trace close operations so that static prepared statements
195 ** will get cleaned up when the shell closes the database connection */
196 if( g.fSqlTrace ) mTrace |= SQLITE_TRACE_PROFILE;
197 sqlite3_trace_v2(db, mTrace, db_sql_trace, 0);
 
 
 
 
 
 
198 return SQLITE_OK;
199 }
200
201 /*
202 ** atexit() handler that cleans up global state modified by this module.
203
--- src/sqlcmd.c
+++ src/sqlcmd.c
@@ -153,10 +153,44 @@
153 sqlcmd_decompress, 0, 0);
154 sqlite3_create_function(db, "gather_artifact_stats", 0, SQLITE_UTF8, 0,
155 sqlcmd_gather_artifact_stats, 0, 0);
156 return SQLITE_OK;
157 }
158
159 /*
160 ** Undocumented test SQL functions:
161 **
162 ** db_protect(X)
163 ** db_protect_pop(X)
164 **
165 ** These invoke the corresponding C routines. Misuse may result in
166 ** an assertion fault.
167 */
168 static void sqlcmd_db_protect(
169 sqlite3_context *context,
170 int argc,
171 sqlite3_value **argv
172 ){
173 unsigned mask = 0;
174 const char *z = (const char*)sqlite3_value_text(argv[0]);
175 if( sqlite3_stricmp(z,"user")==0 ) mask |= PROTECT_USER;
176 if( sqlite3_stricmp(z,"config")==0 ) mask |= PROTECT_CONFIG;
177 if( sqlite3_stricmp(z,"sensitive")==0 ) mask |= PROTECT_SENSITIVE;
178 if( sqlite3_stricmp(z,"readonly")==0 ) mask |= PROTECT_READONLY;
179 if( sqlite3_stricmp(z,"all")==0 ) mask |= PROTECT_ALL;
180 db_protect(mask);
181 }
182 static void sqlcmd_db_protect_pop(
183 sqlite3_context *context,
184 int argc,
185 sqlite3_value **argv
186 ){
187 db_protect_pop();
188 }
189
190
191
192
193 /*
194 ** This is the "automatic extension" initializer that runs right after
195 ** the connection to the repository database is opened. Set up the
196 ** database connection to be more useful to the human operator.
@@ -193,10 +227,16 @@
227 }
228 /* Arrange to trace close operations so that static prepared statements
229 ** will get cleaned up when the shell closes the database connection */
230 if( g.fSqlTrace ) mTrace |= SQLITE_TRACE_PROFILE;
231 sqlite3_trace_v2(db, mTrace, db_sql_trace, 0);
232 db_protect_only(PROTECT_NONE);
233 sqlite3_set_authorizer(db, db_top_authorizer, db);
234 sqlite3_create_function(db, "db_protect", 1, SQLITE_UTF8, 0,
235 sqlcmd_db_protect, 0, 0);
236 sqlite3_create_function(db, "db_protect_pop", 0, SQLITE_UTF8, 0,
237 sqlcmd_db_protect_pop, 0, 0);
238 return SQLITE_OK;
239 }
240
241 /*
242 ** atexit() handler that cleans up global state modified by this module.
243

Keyboard Shortcuts

Open search /
Next entry (timeline) j
Previous entry (timeline) k
Open focused entry Enter
Show this help ?
Toggle theme Top nav button