# Tag Info

36

Just a philosophical answer here. I think you're asking the wrong question. You shouldn't ask yourself "is my SQL code safe or vulnerable?" This is too hard to answer in any individual case, and coming up with the right answer could depend on a lot of contextual things such as the system configuration, the whole program structure, etc. You don't want to ...

19

Your code is vulnerable to SQL injection! Use something like this to prevent it: cursor.execute("INSERT INTO table (state_int, turn_left, turn_right, move_forward) VALUES (?,?,?,?)",(database.records_table_name)) Note that you cannot (or you shouldn't) enter the table in like this: query = '''INSERT INTO %s...''' Ideally the table should be hard coded. ...

18

Looking at your code, there are several things that immediately I can tell are issues. Before we even get to performance, there are basic security measures that need to be taken first. SQL Injection Like you've seen in the comments, your code is highly susceptible to SQL injection. But, what is it exactly? By definition, it is the insertion or "...

18

There's a common credo believed when evaluating the security of software: If it's homemade, it's unlikely secure. Sec.SE has a Q&A about homemade algorithms, which is somewhat germane to your circumstance. I suggest you look into preparing your queries, as that would be your best action to take in this situation. It's essentially what you're trying to ...

17

This seems extra complexity with no purpose. You take any type variable and automatically convert it to a parameter (this is good). But then something strange happens, you look at the type of the variable and convert that to a string so you can call a function named after the type to do a standard set of options that only change based on the type. Why ...

17

On demand -> issues mentioned in comments, reviewed & posted as an answer: The answer to your question has already been given: Yes, you have a serious vulnerability, and the first step to solving this is using the extensions that aren't deprecated, like PDO and mysqli. Both of these extensions support the easiest, and pretty solid (not perfect, but ...

17

I don't know that my code is vulnerable to SQL injection. Yes, it is. You should never put any variables directly into SQL statements. Even if you think that the variables may possibly be safe, it's just really bad practice, and you will mess it up sooner or later. In your case, an attacker could use the profile fields, which would very likely lead to SQL ...

13

There's no way for us tell, from reading this fragment of code, whether there is a SQL injection problem. On the one hand, the query is constructed using string interpolation, which is risky, but the interpolated string is database.records_table_name and we can't tell whether this comes from an untrusted source. (You say in the post that it doesn't, but ...

12

There's only so much a code analysis tool can see. using (SqlCommand command = CreateCommand(query, parameters)) You're using a parameterized SqlCommand, and given the example you appear to have everything under your own control, strongly typed and all. Depends who is calling this ExecuteNonQuery method, and where query is coming from. It appears the ...

11

AdoConverter For better extensibility, the methods in that class shouldn't be calling each others the way ToLongParameter is calling ToIntegerParameter. Also instead of hard-coding the type Private Type TypeMappings BooleanMap As ADODB.DataTypeEnum ByteMap As ADODB.DataTypeEnum CurrencyMap As ADODB.DataTypeEnum DateMap As ADODB.DataTypeEnum ...

11

Let us review! Why are you strip_slashing and strip_tagging the username and passwords before they enter the database? Why do you care? (You don't, I'll get to that in the end). Calling __construct() inside of another method of the same object is not recommended. (What does __construct() do? As opposed to what do setUser() and setPassword() do) Hashing your ...

10

You should not mix PHP logic and HTML. All logic should be placed on top of the document and HTML (+ some minor PHP loops/echos/ifs) below it; $var =$_GET['q']; is unneeded and you loose memory; never use the shorttag (<?); You forgot to set the errormode for PDO, that means it doesn't throw exceptions. To set it, use PDO::setAttribute: $pdo->... 10 Yes,$category in your query string, coming directly from $_GET is a problem. You should validate$category, and that's not exactly the same as escaping. I assume you have a limited set of valid categories in your database, so validation should be something like: $valid_categories = array("sector", "industry", "company"); if (!in_array($category, $... 10 Give me parameterized SQL or give me death! Seriously. Use prepared statements! Don't use mysqli_query as that makes your code vulnerable to SQL Injection. By using prepared statements, you won't need the mysqli_real_escape_string calls anymore. This code looks nasty:$result1 = mysqli_query($con,$sql); if (!$result1 || mysqli_query($con, $sql)) { ... 10 SQL Injection-wise, this is completely safe. You don't run any risk of SQL Injection. However, some parts of your code is not optimal: mysqli_stmt_execute($stmt); mysqli_stmt_bind_result($stmt,$user['username']); mysqli_stmt_execute($stmt); Why are you executing the statement twice? Now, imagine that you hade your if-else switched so that you wanted to ... 10 Your approach is completely wrong. There is a difference between escaping and validation Validation is the act of making sure a piece of input conforms to certain rules (be it logical, or business rules). It should not be used for security. You can't catch all possible edge cases with a blacklist, don't try. Escaping (or Sanitization) is the act of ... 10 The recommended way to avoid SQL injection attacks is to use parameters. Also I can recommend that you create a stored procedure instead of using dynamic SQL. You can pass your dropdown indexes as parameters to your SP and use the old trick of using it as conditional on the where clause. Your select can end up like this: select CT.columnA, CT.columnB from ... 10 PHP already has a function for escaping HTML characters. htmlspecialchars(). In order to completely and absolutely prevent XSS, all you need to do is pass anything that's about to be echoed on a page through that function. So for example: <ul> <?php foreach ($items as $item) : ?> <li><?= htmlspecialchars($item); ?></li> &...

9

I would opt for strict type checking here. It seems a bit lazy to force it to a single when implicit in the function name. No need to use a variant and force it to a Single via a cast. IMHO, if the function ToSingleParameter is expecting a Single, then it should get a Single value and complain with a type mismatch error if it doesn't receive it. I've ...

9

This is not a constructive way to protect your scripts from SQL injection — the objective of SQL-injection prevention should not be to disallow characters or words, but to escape characters. What you have now, prevents your users from submitting perfectly valid English words such as select, update, etc. and/or perfectly valid characters such as --, " and '. ...

8

Unless I misunderstand, you are using unfiltered user input from txtEntryID. Never trust user input. What if the user fills txtEntryID with 123 OR 1=1? The query will delete everything: DELETE FROM PersonalData WHERE DataID = 123 OR 1=1 What if, then, the user inputs 123; UPDATE Account SET credit = 1000 WHERE userId = me: DELETE FROM PersonalData WHERE ...

8

PHP PDO Your PHP script looks airtight, good use of PDO and prepared statements. Nothing to say about this at all other than good job. SELECT * SQL SELECT * statements should not be used in production code unless absolutely necessary. There are several reasons for this: It's can cause inefficient query plans. And generally returns more data than you ...

8

First and foremost, do what @AlexL suggested. Once that is done, here are my tips on improving SQL performance (and PHP readability as a side benefit). INSERT INTO ... (NULL) As duly pointed out on another answer, there is no point to doing this. If the id column is not set to AUTO_INCREMENT just do this: ALTER TABLE rocket_newsites.sites MODIFY id INT ...

7

Error Handling / Execution Flow: Beware of Raptors I realize you haven't implemented it yet, but you've set yourself up for some twisted execution flow: Exit Function 'fixed indentation ErrorHandler: EvaluateQuery = "" End Function When you do implement error handling, you'll likely be cleaning up resources (closing recordsets, connections, etc.) -...

7

Yes, this code is vulnerable to SQL injection, just like any dynamically composed SQL that is not properly quoted. The quoting mechanism for identifiers is database dependent, though. A bigger concern is how this situation came about in the first place. If you ever need to programmatically choose a column like that, your schema is probably poorly designed. ...

7

I'll tell you, I definitely had trouble reproducing your expression. The Refiddle you linked to was using JavaScript's regex engine, not PHP's PCRE. Now, what you're doing is something that is complicated. Just so everyone knows, here is the official documentation regarding fonts of the web. If we dig a little, we will find a sentence saying: Therefore, ...

7

Yes, it is vulnerable. Somebody can make a request to this page using as the value of the product_id field this string: '; DROP TABLE products; @mikebabcock said it best in a comment: never insert a user-assigned variable directly into any SQL statement. Always sanitize it first somehow. There are countless articles on the web for preventing SQL ...

7

Is this method safe? Well, assuming ord is safe, it should be safe, because you will only ever have strings containing numbers in your query. But what is also safe are prepared statements, and they are well tested over the years. And prepared statements don't have the downsides of your solution: untested in practice bad performance time wise (...

7

Does this open the door to destructive injection attacks? It depends, but potentially, yes it can, especially with dynamic SQL, which you should try to avoid unless it is absolutely necessary. You should be able to sanitize the input before passing it to the database server, how to do it really depends on what technologies you are using. NOTE: DO NOT TRY ...

7

Security SQL Injection It's probably[*] secure, but it's not the right way to do it, see for example here why prepared statements are better than escaping. Sooner or later, you will mess up when escaping. Prepared statements are not difficult to use, and result in code that is more secure and more readable, there is really no good reason not to use them. ...

Only top voted, non community-wiki answers of a minimum length are eligible