0

I'm new to working with databases and am attempting to write my own handler for my databases. I have a PHP file containing a class with variables and functions to handle it. This file is then imported to the project needing to access the database in whatever way. In the project the data handler is created as an object then variables are set to tell the object the server name, user, password and database name. Once that data is defined then a function can be called with all the necessary data to build the query, including the table name and such.

I originally tried inserting all data as parameters IE:

$stm = $pdo->prepare("UPDATE ? SET ? = ? WHERE ? = ?");

However, this does not work so I end up with this:

$stm = $pdo->prepare("UPDATE $table SET $records = ? WHERE $compare = ?");

I process the $table, $records, and $compare variables so they only contain alpha-numerical characters, underscores and commas. Also I have set this file to where only the server can access it. I know that the whole purpose of the prepared statement is to prevent user input from entering the query string to fight injection, so is this considered safe practice? With the programming knowledge I already have this seems sound. The functions are inside a class so an attacker shouldn't be able to just call my include file and run their own input. The data is sent to this file from a server-side PHP script and is never touched by the user in any way.

Is this considered a safe practice? Or is there something else that I need to do in order to safe guard this, or just scrap this all together?

Clarification: In this article: (Can PHP PDO Statements accept the table or column name as parameter?) it states the reasoning for whitelisting the tables and columns is to prevent users from being able to input their own data. To the best of my knowledge I have done that. The only way I see a user being able to do so is for them to either upload a script to my server to access it, or on theirs but their server will not have rights to the file. On top of that, the file that holds this coding has no server or database information on it (ie: no username, password or database name). The way I see it, if they are able to use this script, they already have full access to my server and I have bigger concerns.

Xandor
  • 440
  • 1
  • 9
  • 22
  • *"I originally tried inserting all data as parameters IE: $stm = $pdo->prepare("UPDATE ? SET ? = ? WHERE ? = ?");"* - That's because you can't bind tables/columns. – Funk Forty Niner Dec 01 '16 at 12:57
  • If you are going to allow variable table and column names make sure you *whitelist* them against a known list of good names. – Jay Blanchard Dec 01 '16 at 12:58
  • @Fred-ii- I know the reason why. That is what has caused my concern though. My coding works without errors, I am concerned with safety. – Xandor Dec 01 '16 at 13:00
  • if you're using a prepared statement and the variables are pre-defined, then you're good to go. – Funk Forty Niner Dec 01 '16 at 13:01
  • FYI, [a complete guide on the matter](https://phpdelusions.net/pdo/sql_injection_example) – Your Common Sense Dec 01 '16 at 13:02
  • @JayBlanchard I have already read that question and it didn't directly answer my concern, actually that was the question that brought up my concern. My response is too long for comments, I will edit my question with clarification. – Xandor Dec 01 '16 at 13:07
  • I specifically linked that answer because of the whitelisting instruction. – Jay Blanchard Dec 01 '16 at 13:12
  • @YourCommonSense Thank you, actually this looks like just the type of article I'm looking for. – Xandor Dec 01 '16 at 13:12
  • @JayBlanchard Yes, but you also marked this question as a duplicate from that article. My question is security, not trouble shooting, which is what that article covers. – Xandor Dec 01 '16 at 13:14
  • @Fred-ii- Yes, I am using prepared statements and this is how the variables are passed/defined: $dm->update('users', 'sessionid', $session_id, 'username', $user_name); Where users is the table, sessionid and username are the columns. The other values run through the statement as parameters. – Xandor Dec 01 '16 at 13:15
  • @Xandor the ability for the user to to input their own data is rather out of scope for this question. A good database function is *always* safe, despite such a possibility. For example, the point of prepared statements is to make your code safe *completely independently* from such a possibility. So it's better to make your identifiers equally safe. And whitelisting is as bullet-proof method for identifiers as prepared statements for data. – Your Common Sense Dec 01 '16 at 13:16
  • If I remove the dupe your question will be downvoted for several reasons. Asking questions like, *"Is this considered a safe practice? Or is there something else that I need to do in order to safe guard this, or just scrap this all together?"* are considered overly broad or opinion-based questions and in some cases some folks will not consider the entirety of your question suitable for this site. – Jay Blanchard Dec 01 '16 at 13:17
  • @JayBlanchard Fair enough. I was indeed looking for a mixture or fact and concept as well as opinion. But I can see your point. – Xandor Dec 01 '16 at 13:21
  • @YourCommonSense Yes, I see your point, especially after reading that article. I was, or course, just trying to save time with a script I could just 'include' and its ready to go. But in reality having to hard code in the tables and columns per project is no where near the end of the world, for that extra bit of security. – Xandor Dec 01 '16 at 13:30
  • @Xandor well, it's an open ended question. Weighting possibilities is out of scope for Stack Overflow. The main purpose of this site is opposite to serving an individual: it's for providing generalized industry standard solutions for everyone. And whitelisting is such a solution. If you need a code review, there is a [dedicated site](http://codereview.stackexchange.com). As far as I can tell, your character-based whitelisting is enough against regular injections, but I wouldn't bet my hand for it – Your Common Sense Dec 01 '16 at 13:34
  • @YourCommonSense Thank you for the site reference. I actually was unaware of that site and will definitely check it out. But now you have added just a bit more confusion for me. You said my 'character-based whitelisting?' To what are you referring? I sanitize the input but dont pull from or compare to any sort of list, at the moment. – Xandor Dec 01 '16 at 13:38
  • "*I process the $table, $records, and $compare variables so they only contain alpha-numerical characters, underscores and commas*" - isn't is just a sort of white-listing? BTW, I just noticed comma in the list, which makes it quite leaky. – Your Common Sense Dec 01 '16 at 13:40
  • I wasn't sure if that counted as whitelisting. The comma I added in for certain ones. For instance, in my select function, you can pass multiple values delimited by a comma, but the table variable is not allowed to have commas. To be honest, I don't know too much about SQL and injection, I am a long time programmer, of multiple languages but have never really worked with databases before. – Xandor Dec 01 '16 at 13:52
  • @Xandor I don't like that comma. I cannot think of a plausible injection from this limited list, but I don't like it. – Your Common Sense Dec 01 '16 at 15:51
  • @YourCommonSense Understandable. Removing the comma and just simply having to make a new call to my object for each value isn't a big deal. So far, I haven't needed that feature. I just added it in for the future. Not to mention something else I found has stated that this might cause a SQL error anyways. – Xandor Dec 01 '16 at 16:06

0 Answers0