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

Fixing #5884 #5885

Closed
wants to merge 3 commits into from
Closed

Fixing #5884 #5885

wants to merge 3 commits into from

Conversation

PMunch
Copy link
Contributor

@PMunch PMunch commented May 25, 2017

Added more robust MySQL escaping.

Also changed from simple strings to more types that should be supported when inserting into SQL statements.

This fixes nim-lang/db_connector#13

## Database sanitizes the SQLInput
case s.kind:
of SQLStringKind:
result = "'"
Copy link
Contributor

@euantorano euantorano May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it might be worth using newStringOfCap with the length of s.str + 2 initially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah that would probably be good.

@Araq
Copy link
Member

Araq commented Jun 1, 2017

This is a breaking change and inconsistent with the other db_ modules. I agree the reliance on strings everywhere is pretty bad but we need to

a) move db_ modules into its own Nimble package.
b) come up with some super efficient "DB value" abstraction that we'll get wrong across the range of supported databases -.-

@PMunch
Copy link
Contributor Author

PMunch commented Jun 1, 2017

Well, breaking, but breaking broken things. I wrote my server implementation first, then rewrote the SQL escaping afterwards making sure that my server didn't break in the process. Sure it won't work for anything that can be converted to a string, but it really shouldn't be able to. This makes it more explicit if you try to do something that's really not a good idea to do, and makes it hard for un-escaped things to be added to the SQL statement. As for inconsistency, it's all based on SQL but the problem is that SQL isn't one consistent system. So modelling it as such might not be appropriate. I can see why the current system is used, but it simply isn't safe. As for the "DB value" abstraction the type I created here should be serviceable for most common cases, however there are some special cases like timestamps and such which could be added (but those might different between platforms). Another thing to add to make sure we don't miss anything for a certain database implementation is an "Unsafe" or "Unescaped" kind which will allow the user to add a string of unescaped data to their query with the express warning that this should be done at their own risk.

The reason why I started messing around with this was also because I couldn't do what I needed to do for my query. I had a simple SELECT * FROM table LIMIT 100 OFFSET ? query where I wanted to replace the ? with a number to offset by for pagination. The current string based implementation turns the number into a string and then escapes that which doesn't work for the OFFSET parameter. So not only is it not safe but it's lacking in functionality.

@Araq
Copy link
Member

Araq commented Jun 1, 2017

I had a simple SELECT * FROM table LIMIT 100 OFFSET ? query where I wanted to replace the ? with a number to offset by for pagination.

The real thing you're supposed to use is sql("select * from table limit 100 offset " & $offset)

@PMunch
Copy link
Contributor Author

PMunch commented Jun 1, 2017

But that pattern is horrible. You shouldn't concatenate variables with a SQL pattern without passing it through an escape system. Sure if offset is an int (and I would hope it is and not a @"parameter" from a Jester path) you would be fine, but the pattern shouldn't be encouraged.

@Araq
Copy link
Member

Araq commented Jun 1, 2017

Well no, what is broken here is SQL's overly complicated escape system full of special cases that you need to work around sometimes and I showed you the workaround.

@PMunch
Copy link
Contributor Author

PMunch commented Jun 1, 2017

No, the workaround should be done by the system you use to execute your SQL with. Like in the code I've implemented here. You pass it strings and numbers and whatnot and the procedure that handles the escaping takes care of it. Almost impossible for the user to mess it up (unless we add an Unescaped kind, which would need to be called explicitly) by following the pattern they see in examples. As I said, your code might work as long as offset is a safe variable type (ie. one that doesn't create anything funny when being turned into a string) but if a user took that as an example, and without particular care swapped it out for a user input they would have a SQL injection possibility. You might say "well the user should be more careful" but that's not sufficient, the user of a properly written database handler shouldn't have to worry about these things. That's part of the point of using a handler in the first place.

@Araq
Copy link
Member

Araq commented Jun 2, 2017

Well I'm not gonna break db_mysql for everybody just because it's an imperfect solution. It needs a proper migration path and workarounds for your problem do exist. Sorry.

@Araq Araq closed this Jun 2, 2017
@PMunch
Copy link
Contributor Author

PMunch commented Jun 2, 2017

What about adding a concept for things convertible to a string and a toSQLInput that takes anything of that concept. This way everything that would previously be supported is now still supported, but strings would now be properly escaped and ints and floats would be supported in all positions. This could obviously be implemented for all the other SQL based DBs, by just editing the dbQuote procedure, and particularly the string escaping rules. I think they would be pretty similar to MySQL but I'm not 100% sure.

@xzfc
Copy link
Contributor

xzfc commented Jun 3, 2017

You should use mysql_stmt_bind_param function instead of manually escaping values.

@PMunch
Copy link
Contributor Author

PMunch commented Jun 4, 2017

Yeah I know, but I didn't want this version to differ too much from the other DB wrappings. mysql_stmt_bind_param isn't a universal thing so I went with something that could easily be implemented for the other wrappers.

@xzfc
Copy link
Contributor

xzfc commented Jun 4, 2017

Both SQLite and PostgreSQL have simmilar methods: sqlite3_bind_* and PQexecParams. So, binding can be implemented for all supported db_* wrappers in the same way.

I've done some PoC for db_sqlite. Exported execEx and fastRowsEx is same as exec and fastRows except they use sqlite3_bind_* instead of escaping.

@PMunch
Copy link
Contributor Author

PMunch commented Jun 5, 2017

Ah, then this is definitely the way to go. I don't have time to implement this right now since I'm working on my masters but I will have a look at it once I find the time if no-one does it in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MySQL escaping issues
4 participants