search

fabiorino / crelly-slider

A free responsive slider for WordPress that supports layers. Add texts, images, videos and beautify them with transitions and animations.
MIT License
104 stars 36 forks source link

Security updates #35

Closed sioannides closed 7 years ago

sioannides commented 7 years ago

Possible SQL injection update

fabiorino commented 7 years ago

Are you sure that esc_sql() is really necessary? Reading the documentation https://codex.wordpress.org/Class_Reference/wpdb, it seems that you must use $wpdb->escape before using $wpdb->query, nothing else. But I do not use $wpdb->query. Please let me know if I'm missing something, it's really important to be safe. Thank you.

sioannides commented 7 years ago

Both $wpdb->get_row and $wpdb->get_results use $wpdb->query, so it better to add it.

fabiorino commented 7 years ago

Ok, thanks. Are you sure that esc_sql will work? Because, according to the documentation (https://codex.wordpress.org/Function_Reference/esc_sql) it doesn't escape numeric values (and the ID is an integer). Maybe $wpdb->prepare could be the solution.

sioannides commented 7 years ago

@fabiorino yes you are right. I added $wpdb->prepare before executing the queries

fabiorino commented 7 years ago

There are more files that have to be changed. They all contain SELECT * FROM... and should be correctly prepared using $wpdb->prepare. I'll look as soon as possible

sioannides commented 7 years ago

I actually had a vulnerability test on my website and the only files appeared on the report was the ones I made the changes, but if there are more queries like that...they definitely have to be changed.