Security Strategy

[objecttothis]

During the conversion to CI4 we need to make sure that our code security strategy is rock solid. This will lay the groundwork for 3rd-party integrations so that we can have a foundational set of code standards for security.

Escaping outputs:

• Many of the built-in CodeIgniter 4 library functions automatically escape outputs. Examples of this are form_input(), form_password(), form_upload(), form_textarea(), form_dropdown(), form_multiselect(), form_checkbox() and form_radio(). anchor() also is automatically escaped, so no need to do anything with those.
• Output outside of this should always be wrapped in esc() and using the correct context as a 2nd parameter.
• If done correctly, we shouldn't experience problems of data being garbled.

Validation of Inputs:

• CodeIgniter has validation mechanisms that allow us to return errors to the user when invalid data is sent.
• We need to be validating textboxes and other form input for correct data, and required data.

Query Security:

• CodeIgniter4's Query Builder should be used when possible. This is because QueryBuilder has automatic escaping of queries and creates parameterized queries.
• Any time queries are not written with QueryBuilder or QueryBuilder->rawSql() is used, $this->db->escape() should be wrapped around variables being inserted into the database and $this->db->protect_identifiers() should be wrapped around variables representing table or column names. protect_identifiers fails in many cases, so input must be validated using allow list.

Database Security:

• Encryption of all information which can be used to identify an individual including names, addresses, phone numbers, email and addresses.
• Passwords need to be hashed and salted.
• Encryption of API keys used in API calls.

Now on to what I am less certain about:

• CSRF is needed, but we need to do everything in such a way that it doesn't hinder APIs since those will be needed for third party integrations. If it's interfering, potentially using token authentication and APIKEYs in place of CSRF is warranted?
• General best practice, I'm told, is to store html and tags in the database as they are entered (i.e., not encoded). Filtering inputs practices though seems to frequently suggestion wrapping $_POST (getPost() in CI4) in htmlspecialchars() since getPost($foo, FILTER_SANITIZE_STRING) is deprecated. Those functions encode special characters though before they get sent to the model for storing in the database. So the big question is what does Filtering Inputs need to look like?
• One option is simply stripping unwanted data. For example with phone numbers we may wrap that in a preg_replace() that filters anything that is not '(', ')', ' ', '-', '+','[0-9]' and maybe 'x' or '/' for indicating extension numbers. We only have certain kinds of data, so it seems reasonable to have a handful of functions in the security_helper to properly filter these kinds of values without mangling the text. It also makes sense to use the 2nd parameter of getGet() and getPost() where it doesn't mangle text since those are already written. In places we want some HTML tags to be allowed (such as descriptions and attribute text) perhaps using something like https://github.com/ezyang/htmlpurifier rather than strip_tags() would be a good approach.

What's wrong? What's missing?
@jekkos , et. al. thoughts?

[jekkos]

Escaping output

• I would also use bootstrap tables builtin escape functionality so that we can just echo the raw data in json objects

Validation of inputs

• Server side validation is mandatory. Clientside or javascript validation is optional and will make the UI more user friendly. The latter can trigger faster as we do not always need to do a full server roundtrip

Query security

• Recently I reviewed this ticket on blind SQL injection. I wonder if this issue is still present in CI4. The issue was that the sort in the builder accepts a field name that could be manipulated to run custom SQL. The fix was never merged but it should be revisited and could be a good sanity check on the CI4 protections

Database security

• Encrypting user data might be tricky, how will we query it if the database contains encrypted values? Then all query input needs to be encrypted as well

CSRF

• If your application is attaching the credentials via an Authorization header then the browser can't automatically authenticate the requests, and CSRF isn't possible. I believe I never saw CSRF mitigation and probably because browser do not send this header automatically

• This point I agree and tried to explain already few times. In my opinion we should not try to do escaping on the input. We should escape the data right before rendering it instead. The reason is that wee do not know in what context an input will need to be shown. It can be HTML, XML, javascript, or something else. The way escaping is done is different in every context and it won't work when it the text that needs to be rendered was already escaped in some other way

• This last comment is in my opinion a validation step. Basically we try to normalize certain fields before it goes to the database.

[objecttothis]

@jekkos I looked into the vulnerability a little and here is what Github Copilot came back with.

It appears we are using QueryBuilder in most places, but we should search the code for places where we have sorting in the query but we aren't using QueryBuilder.

I had also added filters into the getGet() methods to filter the inputs and this should add to the protection provided by QueryBuilder.

It wouldn't hurt to also add some validation as well, but I'd be curious to see if the original attack vector was closed properly. Where is the original vulnerability report?

[jekkos]

I retrieved all the bug reports we had on this project here : https://huntr.com/users/jekkos/dashboard

[jekkos]

This was a concrete example https://huntr.com/bounties/eccf7762-efb4-4db6-a1de-1030331f34d7

[objecttothis]

This was a concrete example https://huntr.com/bounties/eccf7762-efb4-4db6-a1de-1030331f34d7

OK if someone else doesn't get to it then I'll try it on Monday when I'm working on this project again.

[objecttothis]

@jekkos it doesn't give me access to your dashboard. Are you able to either email it to me or send the report and example to the devroom in glitter? I'm not sure if that site allows sharing of a dashboard with multiple maintainers but that could be an option as well.

[objecttothis]

@jekkos I think I'm just too much of a sucker for a good vulnerability hunt. I couldn't wait until Monday. Unfortunately here is the SQL being generated:

SELECT *
FROM `ospos_giftcards`
LEFT JOIN `ospos_people` AS `person` ON `ospos_giftcards`.`person_id` = `person`.`person_id`
WHERE   (
`person`.`first_name` LIKE '%%' ESCAPE '!'
OR  `person`.`last_name` LIKE '%%' ESCAPE '!'
OR  CONCAT(person.first_name, " ", person.last_name) LIKE '%%' ESCAPE '!'
OR  `ospos_giftcards`.`giftcard_number` LIKE '%%' ESCAPE '!'
OR  `ospos_giftcards`.`person_id` LIKE '%%' ESCAPE '!'
 )
AND `ospos_giftcards`.`deleted` = 0
ORDER BY (SELECT (CASE WHEN (5937=5937) THEN 1 ELSE (SELECT 4996 UNION SELECT 4231) END)) ASC

So as you can see the ORDER BY clause is vulnerable to injection even in CI 4.5.1 using $builder->orderBy(). This particular injection is only returning '1', but it's proof of concept that the field is vulnerable.

The solution I think is to filter and/or validate the inputs

		$search = $this->request->getGet('search', FILTER_SANITIZE_FULL_SPECIAL_CHARS);
		$limit  = $this->request->getGet('limit', FILTER_SANITIZE_NUMBER_INT);
		$offset = $this->request->getGet('offset', FILTER_SANITIZE_NUMBER_INT);
		$sort   = $this->request->getGet('sort', FILTER_SANITIZE_FULL_SPECIAL_CHARS);
		$order  = $this->request->getGet('order', FILTER_SANITIZE_FULL_SPECIAL_CHARS);

		if(!in_array($sort, $giftcard->get_sortable_columns()))
		{
			// Invalid sort column, handle the error
		}

		if(!in_array($order, ['ASC','DESC']))
		{
			// Invalid order direction, handle the error
		}

		$giftcards = $this->giftcard->search($search, $limit, $offset, $sort, $order);

Maybe do this or just silently filter out anything that is not a valid input. I think it would involve creating a field in each model we do sorting and a getter function to return an array of sortable columns, since we may not want sorting on every column. thoughts?

[objecttothis]

If we want to use CodeIgniter's validation engine, it wouldn't be too difficult to write a custom rule. You can even pass the acceptable sort columns to the rule. It's pretty clean that way. Then if it doesn't pass validation we can do what we want with it.

[jekkos]

@objecttothis are you sure that it is vulnerable? I just tried this url
https://dev.opensourcepos.org/giftcards/search?search=&offset=0&limit=25&sort=(SELECT%20(CASE%20WHEN%20(5937=5937)%20THEN%201%20ELSE%20(SELECT%204996%20UNION%20SELECT%204231)%20END))

it does not seem to misbehave at first sight

[jekkos]

No you are right, it's still vulnerable. I tried this one sort=1 AND (SELECT 3335 FROM (SELECT(SLEEP(5)))uafX) and the response is delayed with a couple of seconds, meaning that the sleep is indeed executed.

The PR that I did was indeed about to validate the parameter passed in to the sort column. In this case it would check whether the param has a column name that is present in the db, if not then it would fall back to a default sorting order and ignore the input.

[objecttothis]

Take a look at the query I posted. That's the result of $builder->getCompiledSelect(). It runs the injected select which just produces '1' on my database rather than enumerating tables, however it's enough for me to know that SQL isn't being escaped going into queryBuilder->orderBy(). So, if more malicious SQL is injected we need to be doing something. Maybe I'm not reading this right. Am I missing something?

[objecttothis]

Take a look at the query I posted. That's the result of $builder->getCompiledSelect(). It runs the injected select which just produces '1' on my database rather than enumerating tables, however it's enough for me to know that SQL isn't being escaped going into queryBuilder->orderBy(). So, if more malicious SQL is injected we need to be doing something. Maybe I'm not reading this right. Am I missing something?

(SELECT (CASE WHEN (5937=5937) THEN 1 ELSE (SELECT 4996 UNION SELECT 4231) END))

[jekkos]

then you could also try to run a drop tables or some try to execute some system command, it's quite a serious vulnerability

[objecttothis]

Good ole' Johnny DROP TABLES