• johnywhy

    (@johnywhy)


    Hi

    This article says

    “if you use any of the helper functions, then you don’t need to do anything: the query is escaped for you.”
    https://diigo.com/08f43m

    They say get_var is one of the helper functions, which, they say, is already escaped without prepare.

    Is that correct?

    A moderator over on buddypress.org says “no, prepare is still required for get_var“. They referenced this page.

    Who’s right?

    thx

Viewing 15 replies - 1 through 15 (of 16 total)
  • catacaustic

    (@catacaustic)

    A value from get_var() is not escaped in a database-save way. It’s part ofa variable, and isn’t an issue by itself, only when it’s not used in the right way.

    The rul of thumb is pretty easy to remember. Unless you are hard-coding a specific value so that you know 100% what it will be every time without fail, escape ever other value that goes into an SQL query that you build.

    Also, if you use the standard $wpdb functions like $wpdb->insert() or $wpdb->update() these functions do the excaping for you. if you’re building an SQL query yourself you should always use the $wpdb->prepare() function to make sure that everything as as safe as it should be.

    Thread Starter johnywhy

    (@johnywhy)

    hi, thx for helping a noob ??

    i’m having a little trouble understanding your reply, basically it seems you’re saying the above-quoted article (written by the editor of the WordPress section on Smashing Magazine and contributor to WPMU DEV) is not correct. Are you?

    isn’t an issue by itself

    What do you mean “isn’t an issue”? So, get_var does not require prepare?

    Is get_var vulnerable to injection attacks, even though it’s just reading the db?

    escape ever other value that goes into an SQL query

    What do you mean “value”? You mean every other word in the SQL statement? Or, values in a table-insertion? Or, just the keywords?

    if you use the standard $wpdb functions like $wpdb->insert() or $wpdb->update() these functions do the excaping for you.

    I believe i am using a standard $wpdb function. Here’s my code:

    global $wpdb;
    $query = “SELECT ID FROM wp_t9s5y8_bp_xprofile_groups WHERE name = ‘Apprentice'”;
    $result = $wpdb->get_var($query)

    catacaustic

    (@catacaustic)

    Is get_var vulnerable to injection attacks, even though it’s just reading the db?

    get_var() returns a value from the database. The value would have to have been escaped correctly in order to be inserted into the database in the first place. The only way that the function would be vunerable to injection is if you’re using unescaped values in the SQL query to get the value.

    What you’re doing is fine. As I said above, as you’ve got this hard-coded in there, you can’t replace any part of that query with anything “bad”. The issues only come when you’re using outside, or user-entered, data in the query itself.

    As an example…

    This is safe (all hard-coded, no outside data uses, so no need to do anything more):

    $query = "SELECT ID FROM ".$wpdb->posts." WHERE post_title = 'Post Title'";

    This is NOT safe because you’re uisng outside data, allowing any SQL at all to be entered int othat string:

    $query = "SELECT ID FROM ".$wpdb->posts." WHERE post_title = '" . $_POST ['user_var'] . "'";

    Think about the value of $_POST [‘user_var’] being '; DROP TABLE wp_users;. Your SQL query would end up like this:

    SELECT ID FROM wp_posts WHERE post_title = ''; DROP TABLE wp_users;

    The tricky part there is that it’s actually two queries in one, and the second one will DROP your users table without any confirmation at all. And that example is just if the hackers are playing nice. ??

    That’s why you should use $wpdb->prepare() for creating almost all SQL queries that you need. That function will escape the values correctly so that my example of bad code above doesn’t work. Using that as an example again, if I used this:

    $query = $wpdb->prepare ("SELECT ID FROM ".$wpdb->posts." WHERE post_title = %s", $_POST ['user_var']);

    the resulting save query would be this:

    SELECT ID FROM wp_posts WHERE post_title = '\'; DROP TABLE wp_users;'

    That renders the query save instead of leaving it open to injections.

    Thread Starter johnywhy

    (@johnywhy)

    ok, thx for the clarification. Some follow-up:

    if the WHERE parameter i’m passing into my function is a dropdown selector control on the webpage, then that is user-entered data. But, it’s restricted to the values in the dropdown picker. i’m wondering if a savvy user could bypass the picker, and submit the form with values that are not in the picker (eg DROP users). Is that a valid concern with a selector control? i posted the wrong code. THIS is my function. Is it safe with a selector control?

    function bp_Get_FieldgroupID($FieldgroupName){
         global $wpdb;
         $query = "SELECT ID FROM wp_t9s5y8_bp_xprofile_groups WHERE name = '$FieldgroupName'";
         return $wpdb->get_var($query);
    }

    ====
    is your escaped code an example of escaping “every other value”, as you recommended above?

    $query = $wpdb->prepare ("SELECT ID FROM ".$wpdb->posts." WHERE post_title = %s", $_POST ['user_var']);

    ====

    you should use $wpdb->prepare() for creating almost all SQL queries that you need.

    Would it be more correct to say, “you should use $wpdb->prepare() for creating all SQL queries that include user-entered data.”

    thx!

    catacaustic

    (@catacaustic)

    i’m wondering if a savvy user could bypass the picker, and submit the form with values that are not in the picker

    Very very easily. ??

    Any browser developer tool will allow you to sent whatever values you want.

    The rule is (and this is not negotiable) is that any values that come from the browser at all must be escaped – no exceptions.

    Would it be more correct to say, “you should use $wpdb->prepare() for creating all SQL queries that include user-entered data.”

    Yes, and no. The reason for doing this is to get everyone used to using the $wpdb->preapre() function, which will give you more chance of using it when it’s really needed.

    As far as your function goes, it is not safe the way that it is now. This would be a safe version:

    function bp_Get_FieldgroupID($FieldgroupName){
         global $wpdb;
         $query = $wpdb->prepare ("SELECT ID FROM wp_t9s5y8_bp_xprofile_groups WHERE name = %s", $FieldgroupName);
         return $wpdb->get_var($query);
    }

    I’d also recommend that you use $wpdb->prefix as well instead of hard-coding the prefix in there. Might not make a difference, there’s always a chance.

    Thread Starter johnywhy

    (@johnywhy)

    very awesome answer.

    i’m still not understanding your note about “every other value”.

    i’ve seen some examples which wrap prepare around individual terms within the sql query, instead of the whole query as you’ve done. Your way is certainly easier to write and read. Is there a difference?

    The reason for doing this is to get everyone used to using the $wpdb->preapre() function, which will give you more chance of using it when it’s really needed

    well ok, i’d rather just understand and use it where it needs to be used.

    thx

    catacaustic

    (@catacaustic)

    “every other value” is every value that you do not hard-code into the query. Anything that is a variable is part of “every other value”.

    The $wpdb->prepare function should be used in the same instance. Any time that there’s an external value, you call prepare. The only time that you don’t use prepare is when you have something where there is no variables at all, eg:

    $query = "SELECT post_title FROM wp_posts WHERE post_date >= '2016-01-01 00:00:00'";

    Because there’s no variables in there and no substitution takes place at all, you don’t need to prepare that query.

    Thread Starter johnywhy

    (@johnywhy)

    The only time that you don’t use prepare is when you have something where there is no variables

    Even if the variables are not user-entered?

    I’d also recommend that you use $wpdb->prefix as well instead of hard-coding the prefix in there. Might not make a difference, there’s always a chance.

    Yes, it’ my intention to do use the table-prefix function. But, a “chance” of what?

    catacaustic

    (@catacaustic)

    Even if the variables are not user-entered?

    Yes.

    In some (not many at all) cases it may be acceptable, but for the sake of a few extra CPU cycles to guarantee that your query is safe, there’s no reason not to.

    The prefix is good to use for portability. Chances are that your stie isn’t going to move, but if your theme/plugin ever gets used on a different site with a differnt database prefix set it will break your queries because there’s no table set.

    Thread Starter johnywhy

    (@johnywhy)

    In some (not many at all) cases it may be acceptable, but for the sake of a few extra CPU cycles to guarantee that your query is safe, there’s no reason not to.

    ok, i get that you’re campaigning for alway using prepare, even in cases where it’s not needed. For the sake of straight information, let’s just keep the facts separate from your recommendation. Let’s be clear that prepare is only needed where there is user-entered data in a variable. .

    Many thanks for providing the safe version of my query!

    thx

    Thread Starter johnywhy

    (@johnywhy)

    So, you’re definitely saying that SmashingMagazine article is incorrect.

    Or, are you?

    It’s difficult to tell from your answers which are the facts, and which is your recommendation. You mix your recommendation together with the facts, so i’m not sure which is which.

    Therefor, i still don’t feel this question has been answered unambiguously.

    Anyone else out there able to offer a straight answer, without blurring the facts with your opinions?

    thx

    catacaustic

    (@catacaustic)

    What I’ve said actually agrees with that article.

    What I am saying is that you should escape everything until you have the experience and knowledge to know when it is safe not to. I am not trying to say anything bad but it does seem like you do need a bit more time to learn that for yourself. That’s not a bad thing. Everything takes time and experience.

    I’m trying to tell you the most secure ways of doing these things. That’s all. Until you are confident that you know all of these answers for yourself you should always take the precaution to keep your sites safe.

    I say that from my experience. I have had many clients come to my company with hacked and insecure sites, and most of them have had issues like this, so I want to try and push new people to take security seriously. If you don’t you will leave your site open to hackers that will exploit any vulnerability that you choose to leave open.

    Thread Starter johnywhy

    (@johnywhy)

    Yes, I understand what you’re doing. There’s no need to repeat yourself.

    And that may be very helpful to inexperienced programmers.

    Those who, like myself, have decades of professional programming experience, will be looking for factual answers.

    So, while you have good intentions for beginners, you are at the same time insulting, misleading, and wasting the time of people who are looking for correct information.

    catacaustic

    (@catacaustic)

    What “facts” are you hoping for?

    With your experience you should know all about escaping user input, and when it is required.

    I’m sorry if you don’t like my explanation, but what I’ve said is still correct. Any query that uses variables should be escaped using $wpdb->prepare(). There really is no other answer that is “correct information”.

    Thread Starter johnywhy

    (@johnywhy)

    With your experience you should know all about escaping user input, and when it is required.

    –my experience is desktop, not web, but that’s besides the point. As a database developer, I well understand the importance of protecting the database from potential sql injection attacks.

    But we’re talking here specifically about the WordPress function get_var, not web programming in general. According to what i’ve read, get_var does the escaping for you automatically, in all cases. I’m trying to get a straight “yes” or “no” answer on that. You have failed to answer the OP.

    Yes, and no. The reason for doing this is to get everyone used to using the $wpdb->preapre() function, which will give you more chance of using it when it’s really needed.

    That’s NOT factual information. It’s crusading.

Viewing 15 replies - 1 through 15 (of 16 total)
  • The topic ‘Is `prepare` Required with `get_var`?’ is closed to new replies.