12 ms·
SQL injection search
- PanMan 13y agoNice example, but not all are insecure. For example, the second one here is: $result = mysql_query('DELETE FROM saves WHERE id = '.(int)$_GET['delete']);
- martinml 13y agoIt still doesn't check if a particular id belongs to the user, so you can delete all the items in the table. But I agree that's different kind of problem :)
- jarofgreen 13y agoYour assuming it's a system where data belongs to set users - maybe any user is allowed to delete any piece of data.
- kbenson 13y agoThe search obviously doesn't find all cases, but is a good start. While there's nothing technically wrong with the example given, I might argue that since that won't work in all cases, it might be better to enforce a more rigorous policy of SQL query cleansing, or using bound params. Although this example is so simple I might not. Then again, the fact that $_GET is even available at the location the query is taking place means this is most likely a type of design that I abhor, that PHP makes easy. Put actions in functions or methods, and then call them.
- ownagefool 13y agoPretty much every language will make getting direct user input then passing it to a database easy. What generally makes this less easy (or at least less intuitive) is a framework. Don't compare the likes of Rails or Django to PHP. Compare Laravel4 or Symfony2. That doesn't mean PHP doesn't deserve some stick, it does, but most of it's current reported problems spawn from backwards compatibility. Nobody should be using mysql_*, they should be using prepared statements via PDO. They could solve this by deleteing all the functions you're not supposed to use, but a whole bunch of legacy PHP would stop working. I'm fairly sure this would illicit more hate than the current method of slowly deprecating.
- kbenson 13y agoWhat I meant is that $_GET, $_POST and $_REQUEST aren't available in functions unless you declare them global. The fact that $_GET is being used in the query means either that the query is being performed in the main of that script (probably lots of small php files meant to be called by the browser), or that they are in a function/method and then pull in the global $_GET array. IMHO, neither are good design choices, as one couples the program to HTTP too tightly (and not in a sane way), the other leads to crazy spaghetti PHP as the control flow can be affected by global variables set (or received from the user) far from where they are used.
- astrodust 13y agoThat's an example of hazardously bad programming practices. You're one mistake away from complete disaster. You should be sure that it takes more than one mistake to expose you to that sort of risk. Casting to int is not a general purpose escaping system, and further, if you miss even one of these your entire application can be trashed. Using mysql_query at all is a sign there's something severely wrong with your application.
- steve-howard 13y agoIt's a sign that the developer has had a fairly informal introduction to SQL. Personally I see no reason not to use the PDO extension in any situation where you would use mysql.
- ratsbane 13y agoThere is an opposing viewpoint, i.e. if you actually need an int, casting to int is one of the most reasonable ways of getting it.
- krapp 13y agoYes, and given that (IIRC) $_GET always returns strings anyway, casting to int before adding to a database makes sense. Even so, you should still attempt to validate the string with ctype_digit() and make sure it accurately represents the integer you expect. If you just cast directly to an int, you can't really predict the results. And also, doing the cast inside the sql statement makes it difficult to see. It should be done, if at all, outside the query where it's obvious to anyone looking at the code that this is something that should be paid attention to.
- astrodust 13y agoIn this case, you don't "need an int", you need a value that's safe to put in a database query. If you're inserting a value in a database, you always, always, always use the proper escaping mechanism. No exceptions. That's why using a library with a reliable, well-defined, easy to use escaping system is absolutely imperative.
- tofflos 13y agoI don't know much about PHP but I happened to rewrite some old forms a couple of years ago. The original author had relied on a technique called "magic quotes" (http://php.net/manual/en/security.magicquotes.php http://php.net/manual/en/security.magicquotes.php) which automatically sanitized user input. When we upgraded our version of PHP "magic quotes" had been deprecated and dropped. It would be interesting to know if some of these developers are relying on "magic quotes" or something similar... and also to know how large share of the total number of projects these projects represent.
- magoon 13y agoYeah no, magic quotes didn't ever sanitize.
- a1a 13y agoWhile we are at it.. XSS search: https://github.com/search?q=extension%3Aphp+%3C%3F%3D%24_GET&type=Code&ref=searchresults https://github.com/search?q=extension%3Aphp+%3C%3F%3D%24_GET...
- btipling 13y agoJavaScript version: https://github.com/search?q=extension%3Ajs+html+%2B&type=Code&ref=searchresults https://github.com/search?q=extension%3Ajs+html+%2B&type...
- gingerlime 13y agothis one seems to produce quite a lot of false-positives though, doesn't it?
- btipling 13y agoThose all look like XSS vulnerabilities to me.
- gingerlime 13y agoOf course this is a pretty good starting point to find XSS, but I still see quite a few false positives: the first result I see currently: https://github.com/matsprehn/122B/blob/1d54d2a72f25a23d63ff77edaac65b814043c7c7/js/footer.js https://github.com/matsprehn/122B/blob/1d54d2a72f25a23d63ff7... also spotted this, which looks pretty harmless: https://github.com/cameroni2003/picgrid/blob/0b3becda1f250ef598a0fd72ac101dd08f0892a2/js/globals.js https://github.com/cameroni2003/picgrid/blob/0b3becda1f250ef... a lot others look similar, plus it depends on context...
- btipling 13y agoYes, sure. Those two do not reveal vulnerabilities just from the results, but I have to wonder at not using templates on the first.
- 13y ago
- tshadwell 13y ago;.;
- thiderman 13y agoHeh, cute. This means Github could probably do some automated means of informing these people that their code is insecure and would be a danger to themselves and their users. I'm not sure if they should, but it's interesting that they could.
- jonny_eh 13y agoThere's definitely an opportunity for a service to help developers spot obvious security holes. https://codeclimate.com/ https://codeclimate.com/ is one I've used but it's Ruby only AFAIK.
- dsl 13y agoLots of these exist. Check out whitehatsec.com and veracode.com
- Navarr 13y agoI'm pretty sure anyone can write a github bot. I remember there used to be several (some of which would submit pull requests!)
- yahelc 13y agoThe same bot could notify those people via email using the email address found in commits.
- jaytaylor 13y agoGitHub has not been friendly to bots in the past[1]. [1] https://news.ycombinator.com/item?id=4982240 https://news.ycombinator.com/item?id=4982240 "GitHub Says ‘No Thanks’ to Bots — Even if They’re Nice"
- blazespin 13y agoGitHub (and friends) sound like a great repository to train and prove the value of an automated code review product.
- kbenson 13y agoI would like to say i'm surprised, but I'm not. PHP makes this easier by not even supporting parameter binding in the older, original mysql binding, so it's more prevalent. That said, I'm sure a slight tweak to the search would find a lot in other languages as well.
- marizcombinator 13y agomysql_query is deprecated... use MySQLi or PDO
- martinml 13y agoMySQLi or PDO will not automatically solve almost any of the security issues found in this search.
- astrodust 13y agoUsing either of them correctly will. It is very easy to verify that a mysqli or PDO call is correct by looking at it. The same cannot be said for mysql_query.
- shakesbeard 13y ago90% of example code for PDO uses prepared statements. 90% of example code for the mysql_* functions is ancient and full of security holes.
- Killswitch 13y agoUsing unsanitized $_GET is the least of their problems considering mysql_* is deprecated.
- soult 13y agoSo a gaping security hole that compromises all data is less important than using a deprecated interface?
- rlt3 13y agoHe said it was the least of their problems, not that it was the least important.
- kyberias 13y agoI'm sorry, isn't that the same thing?
- astrodust 13y agoFor an application with no exposure, arguably it is. When you upgrade to PHP 5.6 and your application grinds to a halt because mysql_query isn't available, you'll be wishing you'd fixed it sooner.
- zapt02 13y agoIf you're on RHEL/CentOS, 2023 is gonna be a bad year. (RHEL/CentOS is currently on PHP 5.3 and will stay there for a long time.)
- kodablah 13y agoI believe the ext/mysql is deprecated, not the function names. Mysqlnd is a drop in replacement and unencumbered by the copyleft license issues that plague the original extension.
- LawnGnome 13y ago
- josephscott 13y agoIn addition to $_GET searching for $_POST and $_REQUEST are bad too. Could even through in $_COOKIE and $_SERVER for that matter.
- hising 13y agoI guess people would pay for a service that could identify 90% of all security issues with an online service by going through source code and available routes. Anything that is available today?
- gtani 13y agoI've seen quite a few things that are germane, google "static analysis for security",pentesting etc http://pentestlab.wordpress.com/2012/11/27/automated-source-code-review/ http://pentestlab.wordpress.com/2012/11/27/automated-source-... https://www.owasp.org/index.php/Static_Code_Analysis https://www.owasp.org/index.php/Static_Code_Analysis http://code.google.com/p/yara-project/ http://code.google.com/p/yara-project/ http://ulsrl.org/ http://ulsrl.org/ http://www.lightbluetouchpaper.org/ http://www.lightbluetouchpaper.org/ (the 3rd post (these aren't the services, these are what you shd read to decide if some service's operatives are appropriately expensive, and up on current research
- _pius 13y agohttps://www.tinfoilsecurity.com/ https://www.tinfoilsecurity.com/
- dsl 13y agoTinfoil scans remotely for known vulnerabilities (i.e. WordPress version X has exploit Y). What you really want is source code analysis. WhitehatSec.com and Veracode.com are the two huge players in this space.
- kcen 13y agoFor the Rails folks out there, I have had pretty good luck with Brakeman Scanner in the past. http://brakemanscanner.org/ http://brakemanscanner.org/
- orangethirty 13y agoLooking around I found a simple CMS sold to small online stores. Through their links you can find a listing of their customers (people who use their CMS). Problem is the CMS is open to SQL injection everywhere. If a script kiddie found this info they could take down a lot of online stores. Not good.
- astrodust 13y agoThere are tools that will find these and do all kinds of keep-you-up-at-night terrifying things with your application. You don't even have to be a script kiddie. You just have to be able to click a button.
- jarofgreen 13y agoDid you tell them?
- muloka 13y agoOr better yet write a patch and submit a pull request informing them.
- artursapek 13y ago75 thousand results. And these are just the public repos!
- greenpizza13 13y agoThis is fantastic.
- tptacek 13y agoThere's a joke to be made here about "broken crypto search".
- pjscott 13y agoI'll make that joke, but it won't be very funny. https://github.com/search?p=2&q=MD5+password+extension%3Aphp&ref=searchresults&type=Code https://github.com/search?p=2&q=MD5+password+extension%3... https://github.com/search?q=CURLOPT_SSL_VERIFYHOST+NOT+deprecated+NOT+2&type=Code&ref=searchresults https://github.com/search?q=CURLOPT_SSL_VERIFYHOST+NOT+depre... There's more low-hanging fruit, if you're willing to use more specialized searches. For example, guess what mode of operation the PyCrypto library uses by default for all its block ciphers if you don't explicitly pick a sane one: https://github.com/search?p=1&q=extension%3Apy+crypto+cipher+aes+new+NOT+PyCrypto_AES+NOT+Crypto%2B%2B&ref=searchresults&type=Code https://github.com/search?p=1&q=extension%3Apy+crypto+ci...
- diminoten 13y agoThere is a huge need in the space for a well marketed quality assurance contractor who can find problems like this and fix them. "We found these issues, and we can fix them all. Pay us for finding them or pay us some more for fixing them, too." sort of thing. Why don't you see QA shops popping up like this?
- ams6110 13y agoYou'd need some pretty iron-clad liability waivers... if you miss something, you're opening yourself up for a "malpractice" sort of scenario.
- metarev 13y agoLiability can be addressed with non-reliance disclosures. Bigger issue is a market problem. People who need the help the most do not know they need it.
- steve-howard 13y agoBut then, you'd basically be getting paid for maybe kinda helping secure software.
- DanBC 13y agoThat feels a bit like "We found these vulnerabilities. You should pay us. It would be a shame if anything happened to your website"
- smsm42 13y agoIf you do it right, it won't. Many security audit companies are asked to do test audits, and I've seen security audit contracts starting with responsible disclosure of vulnerabilities. Of course, certain amount of trust is necessary so the sides have to behave in a way that is conductive to the establishment of the trust.
- jarofgreen 13y ago
- ams6110 13y agoSome of these look like deliberate examples of vulnerable code (e.g. the one named "Injection.SQL.php") Alarmingly (and sadly) most do not.
- jt2190 13y agoNo need to be alarmed (or sad.) Without some real-world context for each project, we just can't say whether the code we're looking at represents a real security problem. I'm certain that github hosts just as many one-off, let-me-scratch-some-code-together-that-I'll-never-use-again type projects as it does hard core, production quality ones. That said it's a neat technique for quickly auditing code. Someone should now write an automated tool for submitting security patches to all of these projects.
- boyter 13y agoAnd for examples not limited to github http://searchcode.com/?q=mysql_query%20%24_GET%20lang%3APHP http://searchcode.com/?q=mysql_query%20%24_GET%20lang%3APHP
- sage_joch 13y agoThis is a potentially great idea. You could make your build process include submitting your code to a search engine like this (perhaps in some obfuscated manner) and making illegal patterns fail if not manually "approved". Just because the halting problem exists doesn't mean there's not a low hanging fruit in approaching it.
- egsec 13y agoNot sure if this search would be good, but your responses triggers the id of a distributed code review like reCAPTHA. Possibly a pattern search brings up suspicious code and then there is distributed review and once enough people flag it as SQL injection, it could be flagged on Github and someone could submit a patch, etc. Clearly not bulletproof, but this would provide a way of doing code review on projects you might have no other interest in helping.
- jesstaa 13y agoYep, It's called 'static analysis'. http://en.wikipedia.org/wiki/Static_program_analysis http://en.wikipedia.org/wiki/Static_program_analysis
- deleted 13y ago[deleted]
- sage_joch 13y agoHeh. I sure romanticized a semi-mundane thing that already exists. ;-)
- peterkelly 13y agoDon't worry. That's basically what every tech company does these days anyway.
- twistedpair 13y agoTons of code review tools already exist out there, runnable on your CI machine. Sonar and Fortify are a few off the top of my head, but there are many, many more. Nothing new here. Google code search a while back was similar. Also search for index.php~, a great way to see the source for an index page with a unix temp file that Apache will forward as plain text. Oops.
- deleted 13y ago[deleted]
- postfuturist 13y agoThis isn't a search for SQL injection, its a search for a couple things that you often find in older PHP code that is generally hacked together and likely to have SQL injection vulnerabilities for historical and cultural reasons. However it's perfectly easy to avoid SQL injection even using these things. $id = mysql_real_escape_string($_GET['id']); $res = mysql_query("SELECT foo FROM bar WHERE id='$id'"); That may be ugly, but it's bulletproof regarding injection.
- siddboots 13y agoMost of the search results show exploitable code. Regardless, this is a search for SQL injection vulnerability, even if not every search result falls under that category.
- thangalin 13y agouse PDO; use PDOException; /** * Used for interacting with the database. Usage: * <pre> * $db = Database::get(); * $db->call( ... ); * </pre> */ class Database extends Obj { private static $instance; private $dataStore; /** * Sets the connection that this class uses for database transactions. */ public function __construct() { global $dbhost; global $dbname; global $dbuser; global $dbpass; try { $this->setDataStore( new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) ); } catch( PDOException $ex ) { $this->log( $ex->getMessage() ); } } /** * Returns the singleton database instance. */ public function get() { if( self::$instance === null ) { self::$instance = new Database(); } return self::$instance; } /** * Call a database function and return the results. If there are * multiple columns to return, then the value for $params must contain * a comma; otherwise, without a comma, the value for $params is used * as the return column name. For example: * *- SELECT $params FROM $proc( ?, ? ); -- with comma *- SELECT $proc( ?, ? ) AS $params; -- without comma *- SELECT $proc( ?, ? ); -- empty * * @param $proc Name of the function or stored procedure to call. * @param $params Name of parameters to use as return columns. */ public function call( $proc, $params = "" ) { $args = array(); $count = 0; $placeholders = ""; // Key is zero-based (e.g., $proc = 0, $params = 1). foreach( func_get_args() as $key => $parameter ) { // Skip the $proc and $params arguments to this method. if( $key < 2 ) continue; $count++; $placeholders = empty( $placeholders ) ? "?" : "$placeholders,?"; array_push( $args, $parameter ); } $sql = ""; if( empty( $params ) ) { // If there are no parameters, then just make a call. $sql = "SELECT recipe.$proc( $placeholders )"; } else if( strpos( $params, "," ) !== false ) { // If there is a comma, select the column names. $sql = "SELECT $params FROM recipe.$proc( $placeholders )"; } else { // Otherwise, select the result into the given column name. $sql = "SELECT recipe.$proc( $placeholders ) AS $params"; } $statement = $this->getDataStore()->prepare( $sql ); //$this->log( "SQL: $sql" ); for( $i = 1; $i <= $count; $i++ ) { //$this->log( "Bind " . $i . " to " . $args[$i - 1] ); $statement->bindParam( $i, $args[$i - 1] ); } $statement->execute(); $result = $statement->fetchAll(); $this->decodeArray( $result ); return $result; } /** * Converts an array of numbers into an array suitable for usage with * PostgreSQL. * * @param $array An array of integers. */ public function arrayToString( $array ) { return "{" . implode( ",", $array ) . "}"; } /** * Recursive method to decode a UTF8-encoded array. * * @param $array - The array to decode. * @param $key - Name of the function to call. */ private function decodeArray( &$array ) { if( is_array( $array ) ) { array_map( array( $this, "decodeArray" ), $array ); } else { $array = utf8_decode( $array ); } } private function getDataStore() { return $this->dataStore; } private function setDataStore( $dataStore ) { $this->dataStore = $dataStore; } } Example usage: $db = Database::get(); $result = $db->call( "is_existing_cookie", "existing", $cookie_value ); return isset( $result[0] ) ? $result[0]["existing"] > 0 : false; Another example: private function authenticate() { $db = Database::get(); $db->call( "authentication_upsert", "", $this->getCookieToken(), $this->getBrowserPlatform(), $this->getBrowserName(), $this->getBrowserVersion(), $this->getIp() ); } Switching to PDO is better. Critiques welcome on Code Review SE. http://codereview.stackexchange.com/questions/26507/generic-method-for-database-calls http://codereview.stackexchange.com/questions/26507/generic-...
- soheil 13y agoThis f'ing insane, and absolutely brilliant.
- easy_rider 13y agoI'm just amazed and disturbed that people who write this kind of code are aware of version control.
- PommeDeTerre 13y agoThe person who checked in code isn't necessarily the person who wrote it in the first place. When dealing with legacy code, especially code going back into the 1980s for example, it's not at all unusual for the code to have been stored in several different version control systems over that time. I know of projects that started with SCCS, moved to RCS, then to CVS, then to Perforce, then to Subversion, and most recently to Git. If those transitions are done quickly, then somebody will often just take all of the code from a checkout of the old VCS, and check it into the new system. The same can happen when initially using a VCS, after having not used one before, especially when taking a project over from a developer or even a team that was less-talented. Even if the new developer(s) are going to review the code, and fix bugs and other flaws, a version with the initial state of the code is often a useful thing to have.
- ya3r 13y agoSort the results by "last indexed" and see that people are doing it right now! https://github.com/search?q=extension%3Aphp+mysql_query+%24_GET&type=Code&s=indexed https://github.com/search?q=extension%3Aphp+mysql_query+%24_...
- wordofchristian 13y agoI'd love to see the number of search results graphed over time.
- cbsmith 13y agoMore comprehensive search for bugs: https://github.com/search?q=extension%3Aphp&type=Code&ref=searchresults https://github.com/search?q=extension%3Aphp&type=Code...