Hacker News Reader

Comment by mmmooo | Hacker News Reader

Just as a quick look, this thing is riddled w/ potential sql injections..a number of unchecked/unescaped uri controlled variables. In some cases there's validation of numerics w/ +=0, but in many cases (e.g. $sort_col) there's none.

View on HN

The routing script should be taking care of that stuff with regular expressions.

If I'm wrong, please do let me know!

you mean you shouldn't trust user input?

The arguments to controller methods (in CI) are passed through some regular expressions. CI goes as far as to destroy all GET variables (which I highly disagree with).

GET's are all removed (by default), but for uri segments you'll just get some character filters, and some anti-xss attempts (assuming you have that on). Nothing anywhere near sufficient to prevent sql injection. Again, didn't dig too deep, but I don't see any validation that would prevent me from doing some level of at least blindsql..

I think CI rejects anything in the URI which isn't alpha-numeric. Would that solve the issue?

Kind of, anything in permited_uri_chars is allowed. This includes spaces, slashes, commas, %, and a handful of others by default. As I said only skimmed quickly so maybe I'm missing it. Will take a deeper look in a bit once not on mobile.

This is the escaping mechanism I was using.

I would be excited to see an example exploit executed against the app, I've tried plenty of times without success.

Any pull request to fix a vulnerability will be happily accepted!

I live by the communist motto. Trust, but verify.

Skimming the code, it looks like input is being escaped in the SQL queries. Can you link to examples?

Just skimming, and e.g.

in: controllers/invoice.php

$data['invoices'] = $this->invoice_model->select_multiple($this->session->userdata('company_id'), $page, $this->pref_user['per_page'], TRUE, $sort_col);

$sort_col appears to be just uri_segment 2 of list_items.

select_multiple() then calls:

$sql = "SELECT id, name, DATEDIFF(NOW(), duedate) AS past_due FROM invoice WHERE company_id = " . $this->db->escape($company_id) . " ORDER BY $sort_col";

$sort_col is left as-is. It is certainly more difficult, since '()' aren't permitted in the uri, and we're already in the ORDER BY clause, but I think it may still be doable to get some blindsql into there.

Ugh, yup. This code should either be taken down, or come with a HUGE warning that it needs to be audited for security vulnerabilities.

All projects which have been just open sourced are going to contain bugs. That is, until people like you find them and fix them and submit pull requests ;).

CI strips out all funky characters, so while it is possible to cause an erroneous query, I'm not seeing a security issue here.

On top of that we are all hopefully always learning. New kinds of security attacks will come along and we will have to figure out how to address them.

I would encourage you to consider trying hard to build a team around your project. In my experience open source software is hard work and really only can thrive in a community. This doesn't form magically around the software. It takes time and effort to build. If you can get good security folks in your community you can learn a lot from them.

One recommendation I have is to try to get a security guru on your team. LedgerSMB would be nowhere near as secure if it weren't for the efforts of Seneca Cunningham early on in the project. My coding and sensitivity to these issues has greatly improved.

This is indeed a bug:

http://neoinvoice.local/invoice/list_items/1/asdf

Does cause an erroneous query. It should use a case statement to check against column names. CI will strip out any special characters though.

Reply on news.ycombinator.com