3 ms·

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 valida

by mmmooo 14y ago

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.

renownedmedia 14y ago
The routing script should be taking care of that stuff with regular expressions. If I'm wrong, please do let me know!
jumby 14y ago
you mean you shouldn't trust user input?
- renownedmedia 14y ago
  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).
  - mmmooo 14y ago
    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..
    - rokhayakebe 14y ago
      I think CI rejects anything in the URI which isn't alpha-numeric. Would that solve the issue?
      - mmmooo 14y ago
        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.
      - renownedmedia 14y ago
        This is the escaping mechanism I was using.
    - renownedmedia 14y ago
      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!
- mmmooo 14y ago
  I live by the communist motto. Trust, but verify.
notJim 14y ago
Skimming the code, it looks like input is being escaped in the SQL queries. Can you link to examples?
- mmmooo 14y ago
  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.
  - notJim 14y ago
    Ugh, yup. This code should either be taken down, or come with a HUGE warning that it needs to be audited for security vulnerabilities.
    - renownedmedia 14y ago
      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.
      - einhverfr 14y ago
        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.

by mmmooo 14y ago

renownedmedia 14y ago

The routing script should be taking care of that stuff with regular expressions. If I'm wrong, please do let me know!

jumby 14y ago

you mean you shouldn't trust user input?

renownedmedia 14y ago
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).
- mmmooo 14y ago
  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..
  - rokhayakebe 14y ago
    I think CI rejects anything in the URI which isn't alpha-numeric. Would that solve the issue?
    - mmmooo 14y ago
      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.
    - renownedmedia 14y ago
      This is the escaping mechanism I was using.
  - renownedmedia 14y ago
    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!
mmmooo 14y ago
I live by the communist motto. Trust, but verify.

notJim 14y ago

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

mmmooo 14y ago
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.
- notJim 14y ago
  Ugh, yup. This code should either be taken down, or come with a HUGE warning that it needs to be audited for security vulnerabilities.
  - renownedmedia 14y ago
    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.
    - einhverfr 14y ago
      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.