CiviCRM

Eileen McNaughton added a comment - 06/Nov/11 9:53 PM Here - see what you think

Eileen McNaughton added a comment - 06/Nov/11 10:08 PM Note, I haven't made it clever enough to add the ' - because it doesn't apply to all operators (e.g IN)

Eileen McNaughton added a comment - 22/Nov/11 2:45 AM Just a note on this ticket that it needs some filtering / security before closing (Tim suggested a simple solution).

xavier dutoit added a comment - 11/Dec/11 12:11 PM Hi, What's the status on that? Did you implement the security check as per Tim's suggestion? (re-assign to me or erik so we look at it if needed during the sprint)

Eileen McNaughton added a comment - 11/Dec/11 8:20 PM Nope, still needs to be done

Eileen McNaughton added a comment - 12/Jan/12 1:19 AM Just adding this email thread in here Summary = we should put this in place & test it:: $dao->whereAdd(sprintf('%s %s "%s"', $field, $operator, DAO::escapeString($criteria)));  (#1 below) From Tim I'm not sure which existing functionality you mean. My impression is that the existing functionality (pre-r37268) is based on building queries like: $dao = new DAO_Address(); $dao->street_name = '123 Some St'; $dao->find(); This enforces some restrictions similar to #1 (eg when composing SQL, the DAO will escape any values which are assigned in the above manner), but it only works with the "=" operator. To add any kind of support for other operators, we would need an enhancement like r37268. #1 would tweak r37268 to make it more secure by changing the whereAdd to something like: $dao->whereAdd(sprintf('%s %s "%s"', $field, $operator, DAO::escapeString($criteria))); #2 or #3 would be alternative approaches which would allow developers to use different operators -- and also allow developers to securely mix-in more advanced SQL functionality (like WHERE conditions based on SQL functions). On Nov 8, 2011, at 4:40 PM, Eileen McNaughton wrote: Totten - do you think this issue is broader than just the filters? The filters broaden the risk but is the risk already there?   What's your thoughts on #1 - doing more filtering based on the field type? It's a side issue but I'd like to see more consistency in how this is done (it tends to flush out the greebles when we push consistency). Are you thinking #1 for existing functionality & #2 for the extra filters functionality?   Eileen   From: Tim Otten [] Sent: Wednesday, 9 November 2011 10:17 a.m. To: Xavier Dutoit Cc: Eileen McNaughton Subject: Re: I stuck in the array handling for queries but - it is a security risk?   Those are good examples of possible injections. The ';' trick is a good one -- other tricks include calling SQL functions with side-effects, embedding sub-queries, and (in some cases... but maybe not here) performing inline modifications to SQL variables (like @civicrm_user_id).   My general feeling is that the current implementation isn't safe. Some safer approaches:   1. Tighten the set of acceptable filter options -- i.e. validate $field (it should appear in the DAO's fields() and always escape $criteria with CRM_Core_DAO::escapeString(). This prohibits complex expressions, functions, etc.   2. Allow arbitrary SQL from local PHP-based clients -- but not remote clients. If a remote client needs some advanced query ability, then the developer will have to implement their own API call which securely wraps around civicrm_api.   // Secure API wrapper function civicrm_api3_address_myget(&$params) {   $params['options.is_secure'] = TRUE;   $params['wheres'] = array();   $params['wheres'][] = sprintf("street_address like "%s",     CRM_Core_DAO::escapeString($params['street_address']);   );   unset($params['street_address']);   return civicrm_api('address', 'get', $params); }   3. Allow for "registered" or "named" SQL filters -- in PHP, a developer declares a filter named "byDayOfWeek". Then any client (local or remote) can reference "byDayOfWeek" as part of their API call, e.g.   // Client usage civicrm_api('activity','get',array(   'filters' => array(     array('byDayOfWeek', 'activity_date_time', 'Mon'),   ) ));   // Filter implementation function mymodule_filter_byDayOfWeek(&$dao, $spec) {   // validate $spec[1] is a valid field name   // validate $spec[2] is a valid day-of-week   $dao->whereAdd(sprintf("dayofweek(%s) = %d", $spec[1], $spec[2])); }   4. Redesign CiviCRM's entire access-control infrastructure to use SQL-based security mechanisms (views, stored-procedures, etc). Then we can be promiscuous about executing arbitrary SQL. (NON-STARTER)   My personal favorite is #3 -- it allows developers the full power of SQL (including multiple WHEREs, JOINs, HAVINGs, etc), it works the same way for local and remote clients, and it doesn't suffer the combinatorial problem. (With #2, you have work harder to support different combinations of filters,entities, and actions.)   On Nov 7, 2011, at 2:10 PM, Xavier Dutoit wrote: Hi, Starts looking good ;) https://fisheye2.atlassian.com/viewrep/CiviCRM/trunk/api/v3/utils.php?r1=37242&r2=37268 I'm not sure if/where the sql query is escaped (eg is whereAdd injection safe? or is this escaped elsewhere?) eg. what if $criteria contains " 'empty'; DELETE FROM civicrm_contact;" eg sort_name=array ('like' => " 'empty'; DELETE FROM civicrm_contact;" ); Tim, do you have examples for phpunit tests for sql injections? Think we should add a few, for this at least. X+ On 7 November 2011 19:18, Eileen McNaughton wrote: http://issues.civicrm.org/jira/browse/CRM-9150   I realised that the way I did it would probably allow people to add their own 'OR blah blah' - as far as I know you can't embed a nested update or anything in a where clause but not sure if there is a risk   Eileen      

xavier dutoit added a comment - 16/Jan/12 9:11 AM Hi, Am I supposed to do something on that one? Lost track, reassign to me if you want me to do something X+

Eileen McNaughton added a comment - 07/Feb/12 8:27 PM Tim - I've committed your suggestion & tests pass - can you double check you think it's all OK?

Eileen McNaughton added a comment - 07/Feb/12 8:29 PM Would be good if we could create a test that tested the security aspect of it

Tim Otten added a comment - 07/Feb/12 9:37 PM 38735 looks right for the security issue, but there's some functional issues:  * In $acceptedSQLOperators, the "NOT IN" operator has an extra space  * The "<>", "!=", "IS NULL", "IS NOT NULL", and "NOT LIKE" operators might be nice.  * I think this breaks non-binary operators (IN, NOT IN, BETWEEN). Each argument needs to be escaped separately. This may require a clarification/change to the contract. e.g. // Client Code $params['country_id'] = array('IN' => array(1013,1154,1228)); $params['activity_date_time'] = array('BETWEEN' => array($lowDatetime, $highDateTime)); // Query Building Code (untested) if(in_array($operator,$acceptedSQLOperators)){     switch ($operator) {         // unary operators         case 'IS NULL':         case 'IS NOT NULL':             $dao->whereAdd(sprintf('%s %s', $field, $operator));             break;                      // ternary operators         case 'BETWEEN':         case 'NOT BETWEEN'             if (empty($criteria[0]) || empty($criteria[1])) error();             $dao->whereAdd(sprintf('%s BETWEEN "%s" AND "%s"', $field, DAO::escapeString($criteria[0]), DAO::escapeString($criteria[1])));             break;                  // n-ary operators         case 'IN':         case 'NOT IN':             if (empty($criteria)) error();             $escapedCriteria = array_map(array('CRM_Core_DAO','escapeString'), $criteria);             $dao->whereAdd(sprintf('%s %s ("%s")', $field, $operator, implode('", "', $escapedCriteria)));             break;                  // binary operators         default:             $dao->whereAdd(sprintf('%s %s "%s"', $field, $operator, DAO::escapeString($criteria)));     } }

Eileen McNaughton added a comment - 07/Feb/12 10:10 PM Hmm - I'm tempted then to only support Unary operators for 4.1 & do the rest for 4.2 So, if I do     $acceptedSQLOperators = array( '=', '<=', '>=', '>', '<', 'LIKE',"<>", "!=", "IS NULL", "IS NOT NULL", "NOT LIKE" ); For 4.1 - it would seem OK (& them maybe I'll raise a separate issue to extend it to Between / In etc for 4.2 - sound right?

Tim Otten added a comment - 08/Feb/12 7:41 PM That makes sense to me. Note that the unary ("x IS NULL", "x IS NOT NULL") and binary ("x=y","x!=y") operators do require different whereAdd() statements. If the goal is to keep it simple (with the current whereAdd() statement), then one would also omit the "IS NULL" and "IS NOT NULL".

Eileen McNaughton added a comment - 08/Feb/12 9:58 PM OK - have restricted. When someone next gets keen they can open a new issue & extend