Fossil may run temporary files when command settings are empty strings
(1) By Preben Guldberg (preben) on 2023-09-25 13:52:57 [source]
If a command setting like diff-command is accidentally set to an empty string, fossil accepts this, and will execute command lines based on this.
For e.g. diff-command, this means fossil attemps to execute a temporary file.
I'd see two types of events where this might happen:
- Someone clears the string value in the /setup_settings interface.
- Someone thinks an empty value causes the setting to be unset or ignored.
Afaict, there currently is no way to unset the value from /setup_settings.
I have a local patch that addresses this, but no check-in rights (still pending, after I sent waiver in forumpost/a80cbc79ff).
I have not looked into the effect of having empty string values for e.g. log names, some of the email-xxx settings, etc. I imagine having set these in the ui, then clearing the field, could affect other commands in unexpected ways.
If anybody wants to look at this, below is a mangled version that uses no sensible variables and have a couple of small obfuscations and coding style deviations that would probably need adressing if applied. Don't know if that is sufficient for others to work with it easily.
Index: src/db.c
==================================================================
--- src/db.c
+++ src/db.c
@@ -5114,10 +5114,14 @@
}
if( unsetFlag ){
db_unset(pSetting->name/*works-like:"x"*/, globalFlag);
}else{
db_protect_only(PROTECT_NONE);
+ ( (!g.argv[3][0]) &&
+ strrchr(pSetting->name, '-') != NULL &&
+ fossil_stricmp(strrchr(pSetting->name, '-'), "-COMMAND") == 0)
+ ? db_unset(pSetting->name/*works-like:"x"*/, globalFlag) :
db_set(pSetting->name/*works-like:"x"*/, g.argv[3], globalFlag);
db_protect_pop();
}
if( isManifest && g.localOpen ){
manifest_to_disk(db_lget_int("checkout", 0));
Index: src/setup.c
==================================================================
--- src/setup.c
+++ src/setup.c
@@ -240,10 +240,14 @@
const char *zQ = P(zQParm);
if( zQ && fossil_strcmp(zQ,zVal)!=0 && cgi_csrf_safe(2) ){
const int nZQ = (int)strlen(zQ);
setup_incr_cfgcnt();
db_protect_only(PROTECT_NONE);
+ ( (!zQ[0]) &&
+ strrchr(zVar, '-') != NULL &&
+ fossil_stricmp(strrchr(zVar, '-'), "-COMMAND") == 0)
+ ? db_unset(zVar/*works-like:"x"*/, 0) :
db_set(zVar/*works-like:"x"*/, zQ, 0);
db_protect_pop();
admin_log("Set entry_attribute %Q to: %.*s%s",
zVar, 20, zQ, (nZQ>20 ? "..." : ""));
zVal = zQ;
(2) By Stephan Beal (stephan) on 2023-09-25 14:27:49 in reply to 1 [link] [source]
I have a local patch that addresses this, but no check-in rights (still pending, after I sent waiver in forumpost/a80cbc79ff).
It hasn't yet arrived :(.
We're currently discussing solution alternatives and their side-effects. Thanks for reporting this - it's somewhat surprising that it hasn't come up before.
(3) By Stephan Beal (stephan) on 2023-09-25 16:09:09 in reply to 2 [link] [source]
We're currently discussing solution alternatives and their side-effects.
The thinking was that we could add a flag to settings which defines whether an empty string is to be treated as "unset" or a literal value, with only 3 settings permitting a literal empty value.
That logic breaks for the default-csp
setting, where an empty string is currently accepted as "install the default" but would, under this change, use an empty CSP. That's a backwards compatibility risk we can't currently take, as it could change security-relevant behavior in deployed clients.
There may well be other side effects we haven't yet discovered, and will need to do an audit of db_get()/db_set() calls to find out what all may be affected by such a change. With 265 calls to db_get() and 51 calls to db_set(), however, that's not going to happen today ;).
(4) By Stephan Beal (stephan) on 2023-09-26 06:37:00 in reply to 2 [link] [source]
I have a local patch that addresses this, but no check-in rights (still pending, after I sent waiver in forumpost/a80cbc79ff).
It hasn't yet arrived :(.
Please check your email, noting that the mail (from me) might have landed in your spam box.