Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL Injection from API? #15337

Open
wtfbrb opened this issue Sep 6, 2018 · 8 comments
Open

SQL Injection from API? #15337

wtfbrb opened this issue Sep 6, 2018 · 8 comments
Labels
security to-validate Old issues pending validation

Comments

@wtfbrb
Copy link

wtfbrb commented Sep 6, 2018

If you go here: /api/resource/Item?fields=["name","group"] on any erpnext install you get an SQL syntax error which has me worried of what else can be done with some escaping...field names should probably be enclosed in ` either which way if this is not a security issue :)

@umairsy umairsy added the to-validate Old issues pending validation label Oct 18, 2018
@codingCoffee
Copy link
Contributor

I tried replicating he issue, and here's what I found:

  1. I'm only able to use the API if I do have permissions on the DocType in question ('Item' doctype in this case). This rules out the possibility of a security flaw.
  2. Secondly the reason you're getting a error in your SQL syntax, because the row group does not exist in the Item table. The field is named item_group. So it doesn't seem to be a problem in the response either.

If I have missed anything please post the steps to replicate it.

@gbm001
Copy link
Contributor

gbm001 commented Dec 5, 2018

So I haven't had a chance to post it up yet, but I have demonstrated a proof of concept SQL injection from frappe.get_list (which is what the API resource thing goes to). I think you are right, @wtfbrb , that you (in general) shouldn't get SQL programming errors from client input. I can imagine that you could design a secure system that would generate SQL errors but it would be an odd design; in general getting an SQL error suggests you are not properly handling input.

I think the statement 'This rules out the possibility of a security flaw' is clearly untrue; if you can inject SQL then permissions are irrelevant since permissions are handled by the Python and not the SQL. Yes, you might need permissions on one Doctype to access that API, but once you have injected SQL it's all over...

@gbm001
Copy link
Contributor

gbm001 commented Dec 11, 2018

I have now reported the vulnerability to Frappe.
Using the reported attack, a logged-in user with no privileges (such as can usually be created from the sign-up form) can retrieve information from any database table using the JS function frappe.call, calling a particular python routine, and passing it specially-crafted arguments which construct an SQL query of the form:
SELECT fieldname FROM table;
and the results are returned to the frappe.call callback.
Probably this can be trivially extended to multiple fieldnames etc.

@jtarun4625
Copy link

Any update about this issue ?

@gbm001
Copy link
Contributor

gbm001 commented May 8, 2020

They did (eventually) respond. One of the issues I reported has been properly fixed. The other attack I came up with no longer works as it brings up an 'invalid SQL query' error, so I suspect they are still trying to do the 'sanitizing' approach (which is less than ideal) rather than building fully parametrized queries but hey-ho...

@adityahase
Copy link
Member

@gbm001 You're more than welcome to come and fix it.

@silverbacknet
Copy link

@gbm001 You're more than welcome to come and fix it.

Is he, though? Is anyone actually welcome to fix it? In the past, Erpnext has rejected parameterized SQL and enforced a policy of sanitized-and-concatenated-string queries only.

@adityahase
Copy link
Member

@silverbacknet Can you point me to such PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security to-validate Old issues pending validation
Projects
None yet
Development

No branches or pull requests

7 participants