Fix:concatenation prohibition #33612
Conversation
Unuseful concatenation function prohibition in formulas
| @@ -11082,14 +11082,6 @@ function dol_eval($s, $returnvalue = 1, $hideerrors = 1, $onlysimplestring = '1' | |||
| return ''; | |||
| } | |||
| } | |||
| if (preg_match('/[^0-9]+\.[^0-9]+/', $s)) { // We refuse . if not between 2 numbers | |||
There was a problem hiding this comment.
Why removing this rule ?
Which use cas do you want to solve ?
There was a problem hiding this comment.
A simple concatenation expression such as $a.$b is matched by this rule and is rejected. So, it prevents the use of the php concatenation operator in calculated extrafields formulas.
There was a problem hiding this comment.
Yes, this is a security feature. concat is used to obfuscated evil instruction.
We want for the moment to keep the possible things available into dol_eval as simple as possible.
So to allow concatenation, can you provide an example of instruction you want to introduce into dol_eval (with value) so we can see how wa can allow concatenation without breaking security?
There was a problem hiding this comment.
@bafbes,
I needed to have an extrafield to see the date of the last paiement of factures (when partially paid) in list view.
Impossible to do it without database request, and need concatenation of ID with the SQL request.
I've managed to do it with dol_concatdesc :
dol_concatdesc('SELECT datep FROM llx_facture_paiement WHERE f_rowid = ' , $objectoffield->id)
I hope that it will help you.
@eldy it seems to me that dol_eval should not be limited but protected. If an attacker can make dol_eval execute string, it is not preventing concatenation that will prevent anything, am i wrong ? But maybe there are usages that i'm not seeing.
It seems to me that it is as usefull as trying to forbid usage of JOIN inside the $db->sql() function. We need it and if an attacker is allowed to execute sql requests, that is not preventing the JOIN that will enhance security.
But as i've said, you have a far greater view on large-scale use cases.
There was a problem hiding this comment.
I use an extrafield for contract objects called path in Sellyoursaas module. It is calculated by the formula '/home/jail/home/'.$object->array_options['options_username_os'].'/'.$object->array_options['options_database_db']. This formula is no longer acceptable by this test.
There was a problem hiding this comment.
On the forum, we can find users that are using implode( ' ', array('myString1', 'myString2')) to circumvent this dot restriction.
There was a problem hiding this comment.
@bafbes
I pushed a better fix.
The concate is disallowed now only if MAIN_DISALLOW_STRING_OBFUSCATION_IN_DOL_EVAL is set so we can restore the same level of security if we need.
eldy
added
the
Discussion
Erroneous concatenation function prohibition in formulas