|
|
| (190 intermediate revisions by 16 users not shown) |
| Line 1: |
Line 1: |
| − | ==[[Code Review Introduction|Introduction]] == | + | {{LinkBar |
| − | '''Preface''':
| + | | useprev=PrevLink | prev= | lblprev= |
| − | This document is not a “How to perform a Secure Code review” walkthrough but more a guide on how to perform a successful review. Knowing the mechanics of code inspection is a half the battle but I’m afraid people is the other half.
| + | | usemain=MainLink | main=OWASP Code Review Guide Table of Contents | lblmain=Table of Contents |
| − | To Perform a proper code review, to give value to the client from a risk perspective and not from an academic or text book perspective we must understand what we are reviewing.
| + | | usenext=NextLink | next=Code Review Guide Foreword | lblnext=Foreword by OWASP Chair |
| | + | }} |
| | + | __NOTOC__ |
| | | | |
| − | Applications may have faults but the client wants to know the “real risk” and not necessarily what the security textbooks say.
| |
| | | | |
| − | Albeit there are real vulnerabilities in real applications out there and they pose real risk but how do we define real risk as opposed to best practice?
| + | ==[[Code Review Guide Foreword|Foreword by OWASP Chair]]== |
| | | | |
| − | This document describes how to get the most out of a secure code review. What is important when managing an engagement with a client and how to keep your eye on the ball the see the “wood from the trees”.
| + | ==Frontispiece== |
| | | | |
| | + | * [[Code Review Guide Frontispiece|About the OWASP Code Review Project]] |
| | + | * [[OCRG1.1:About The Open Web Application Security Project|About The Open Web Application Security Project]] |
| | | | |
| − | '''Introduction''':
| + | ==Guide History== |
| − | The only possible way of developing secure software and keeping it secure going into the future is to make security part of the design. When cars are designed safety is considered and now a big selling point for people buying a new car, “How safe is it?” would be a question a potential buyer may ask, also look at the advertising referring to the “Star” rating for safety a brand/model of car has.
| + | * [[Code Review Guide History]] |
| − | Unfortunately the software industry is not as evolved and hence people still buy software without paying any regard to the security aspect of the application.
| |
| | | | |
| − | This is what OWASP are trying to do, to bring security in web application development into the mainstream, to make is a selling point. 30% to 35% of Microsoft’s budget for “Longhorn”
| + | ==Methodology== |
| − | is earmarked for security, a sign of the times. http://news.bbc.co.uk/2/hi/business/4516269.stm
| |
| | | | |
| − | Every day more and more vulnerabilities are discovered in popular applications, which we all know and use and even use for private transactions over the web.
| + | *[[Code Review Introduction|Introduction]] |
| | + | *[[Code Review Preparation|Preparation]] |
| | + | *[[Security Code Review in the SDLC]] |
| | + | *[[Security Code Review Coverage]] |
| | + | *[[OCRG1.1:Application Threat Modeling|Application Threat Modeling]] |
| | + | *[[Code Review Metrics]] |
| | | | |
| − | I’m writing this document not from a purest point of view. Not everything you may agree with but from experience it is rare that we can have the luxury of being a purest in the real world.
| + | ==Crawling Code== |
| − | Many forces in the business world do not see value in spending a proportion of the budget in security and factoring some security into the project timeline.
| + | * [[Crawling Code]] |
| | + | * [[Searching for Code in J2EE/Java]] |
| | + | * [[Searching for Code in Classic ASP]] |
| | + | * [[JavaScript/Web 2.0 Keywords and Pointers]] |
| | | | |
| − | The usual one liners we hear in the wilderness:
| + | ==Code Reviews and PCI DSS== |
| | + | * [[Code Reviews and Compliance]] |
| | | | |
| − | ''“We never get hacked (that I know of), we don’t need security”
| + | ==Examples by Technical Control== |
| | + | * [[Codereview-Authentication|Authentication]] |
| | + | * [[Codereview-Authorization|Authorization]] |
| | + | * [[Codereview-Session-Management|Session Management]] |
| | + | * [[Codereview-Input Validation|Input Validation]] |
| | + | * [[Codereview-Error-Handling|Error Handling]] |
| | + | * [[Codereview-Deployment|Secure Deployment]] |
| | + | * [[Codereview-Cryptographic_Controls|Cryptographic Controls]] |
| | | | |
| − | “We never get hacked, we got a firewall”.
| + | ==Examples by Vulnerability== |
| | + | * [[Reviewing Code for Buffer Overruns and Overflows]] |
| | + | * [[Reviewing Code for OS Injection]] |
| | + | * [[Reviewing Code for SQL Injection]] |
| | + | * [[Reviewing Code for Data Validation]] |
| | + | * [[Reviewing Code for Cross-Site Scripting]] |
| | + | * [[Reviewing Code for Cross-Site Request Forgery]] |
| | + | * [[Reviewing Code for Logging Issues]] |
| | + | * [[Reviewing Code for Session Integrity]] |
| | + | * [[Reviewing Code for Race Conditions]] |
| | | | |
| − | Question: “How much does security cost”?
| + | == Language Specific Best Practice == |
| − | Answer: “How much shall no security cost”?
| |
| | | | |
| − | "Not to know is bad; not to wish to know is worse."''
| + | ===Java=== |
| − | - I love proverbs as you can see.
| + | *[[Java Gotchas]] |
| | + | *[[Leading Java Security Practice]] |
| | | | |
| − | Code inspection is a fairly low-level approach to securing code but is very effective.
| + | ===Classic ASP=== |
| − | It is in effect a look under the hood of an application (whitebox).
| + | *[[Classic ASP Design Mistakes]] |
| | | | |
| | + | ===PHP=== |
| | + | *[[Leading PHP Security Practice]] |
| | | | |
| | + | ===C/C++=== |
| | + | *[[Strings and Integers]] |
| | | | |
| | + | ===MySQL=== |
| | + | *[[Reviewing MySQL Security]] |
| | | | |
| | + | ===Rich Internet Applications=== |
| | + | *[[Reviewing Flash Applications]] |
| | + | *[[Reviewing AJAX Applications]] |
| | + | *[[Reviewing Web Services]] |
| | | | |
| − | [[Category:OWASP Code Review Project]] | + | == Example Reports == |
| | + | * [[How to Write an Application Code Review Finding]] |
| | | | |
| − | ==[[Buffer Overruns and Overflows|Buffer Overruns and Overflows]] == | + | ==Automating Code Reviews== |
| − | [[Category:OWASP Code Review Project]] | + | * [[Automated Code Review]] |
| | + | * [[Tool Deployment Model]] |
| | + | * [[Code Auditor Workbench Tool]] |
| | + | * [[The Owasp Orizon Framework]] |
| | | | |
| − | ==[[Data Validation (Code Review)|Data Validation]] == | + | ==[[The Owasp Code Review Top 9]]== |
| − | [[Category:OWASP Code Review Project]]
| |
| | | | |
| − | ==[[Error Handling]]==
| + | ==[[The Owasp Code Review Scoring System]]== |
| − | ==[[OS Injection]] ==
| |
| − | ==[[The Secure Code Environment]] == | |
| − | ==[[Transaction Analysis]] ==
| |
| − | ==[[SQL Injection]] ==
| |
| | | | |
| − | What SQL injection is:
| + | ==[[References]]== |
| | | | |
| − | SQL injection is a security vulnerability that occurs in the persistence/database layer of a web application.
| + | {{LinkBar |
| − | This vulnerability is derived from the incorrect escaping of variables embedded in SQL statements. It is in fact an instance of a more general class of vulnerabilities based on poor input validation and bad design that can occur whenever one programming or scripting language is embedded inside another.
| + | | useprev=PrevLink | prev= | lblprev= |
| | + | | usemain=MainLink | main=OWASP Code Review Guide Table of Contents | lblmain=Table of Contents |
| | + | | usenext=NextLink | next=Code Review Guide Foreword | lblnext=Foreword by OWASP Chair |
| | + | }} |
| | | | |
| − | How to Locate potentially vulnerable code
| + | [[Category:OWASP Code Review Project]] |
| − | | |
| − | | |
| − | A secure way to build SQL statements is to construct all queries with PreparedStatement instead of Statement and/or to use parameterized stored procedures.
| |
| − | Parameterized stored procedures are compiled before user input is added, making it impossible for a hacker to modify the actual SQL statement.
| |
| − | | |
| − | The account used to make the database connection must have “Least privilege” If the application only requires read access then the account must be given read access only.
| |
| − | | |
| − | Avoid disclosing error information: Weak error handling is a great way for an attacker to profile SQL injection attacks. Uncaught SQL errors normally give too much information to the user and contain things like table names and procedure names.
| |
| − | | |
| − | | |
| − | Best practices when dealing with DB’s
| |
| − | | |
| − | Use Database stored procedures, but even stored procedures can be vulnerable.
| |
| − | Use parametrized queries instead of dynamic SQL statements.
| |
| − | Data validate all external input:
| |
| − | Ensure that all SQL statements recognize user inputs as variables, and that statements are precompiled before the actual inputs are substituted for the variables in Java.
| |
| − | | |
| − | | |
| − | SQL INJECTION EXAMPLE:
| |
| − | | |
| − | String DRIVER = "com.ora.jdbc.Driver";
| |
| − | String DataURL = "jdbc:db://localhost:5112/users";
| |
| − | String LOGIN = "admin";
| |
| − | String PASSWORD = "admin123";
| |
| − | Class.forName(DRIVER);
| |
| − | | |
| − | //Make connection to DB
| |
| − | Connection connection = DriverManager.getConnection(DataURL, LOGIN, PASSWORD);
| |
| − | | |
| − | String Username = request.getParameter("USER"); // From HTTP request
| |
| − | String Password = request.getParameter("PASSWORD"); // From HTTP request
| |
| − | | |
| − | int iUserID = -1;
| |
| − | String sLoggedUser = "";
| |
| − | | |
| − | String sel = "SELECT User_id, Username FROM USERS WHERE Username = '" +Username + "' AND Password = '" + Password + "'";
| |
| − | | |
| − | Statement selectStatement = connection.createStatement ();
| |
| − | ResultSet resultSet = selectStatement.executeQuery(sel);
| |
| − | | |
| − | if (resultSet.next()) {
| |
| − | iUserID = resultSet.getInt(1);
| |
| − | sLoggedUser = resultSet.getString(2);
| |
| − | }
| |
| − | PrintWriter writer = response.getWriter ();
| |
| − | if (iUserID >= 0) {
| |
| − | writer.println ("User logged in: " + sLoggedUser);
| |
| − | } else {
| |
| − | writer.println ("Access Denied!");
| |
| − | }
| |
| − | | |
| − | When SQL statements are dynamically created as software executes, there is an opportunity for a security breach as the input data can truncate or malform or even expand the original SQL query!
| |
| − | | |
| − | Firstly the request.getParameter retrieves the data for the SQL query directly from the HTTP request without any Data validation (Min/Max length, Permitted characters, malicious characters). This error gives rise to the ability to input SQL as the payload and alter the functionality in the statement.
| |
| − | | |
| − | The application places the payload directly into the statement causing the SQL vulnerability:
| |
| − | | |
| − | | |
| − | String sel = "SELECT User_id, Username FROM USERS WHERE Username = '"
| |
| − | Username + "' AND Password = '" + Password + "'";
| |
| − | | |
| − | | |
| − | | |
| − | | |
| − | | |
| − | .NET
| |
| − | | |
| − | Parameter collections such as SqlParameterCollection provide type checking and length validation. If you use a parameters collection, input is treated as a literal value, and SQL Server does not treat it as executable code and therefore the payload can not be injected. Using a parameters collection lets you enforce type and length checks. Values outside of the range trigger an exception. Make sure you handle the exception correctly.
| |
| − | Example of the SqlParameterCollection:
| |
| − | | |
| − | using System.Data;
| |
| − | using System.Data.SqlClient;
| |
| − | | |
| − | using (SqlConnection conn = new SqlConnection(connectionString))
| |
| − | {
| |
| − | DataSet dataObj = new DataSet();
| |
| − | SqlDataAdapter sqlAdapter = new SqlDataAdapter( "StoredProc", conn);
| |
| − | sqlAdapter.SelectCommand.CommandType = CommandType.StoredProcedure;
| |
| − | //specify param type
| |
| − | sqlAdapter.SelectCommand.Parameters.Add("@usrId", SqlDbType.VarChar, 15);
| |
| − | sqlAdapter.SelectCommand.Parameters["@usrId "].Value = UID.Text; // Add data from user
| |
| − | | |
| − | sqlAdapter.Fill(dataObj); // populate and execute proc
| |
| − | }
| |
| − | | |
| − | | |
| − | Stored procedures don’t always protect against SQL injection:
| |
| − | | |
| − | CREATE PROCEDURE dbo.RunAnyQuery
| |
| − | @parameter NVARCHAR(50)
| |
| − | | |
| − | AS
| |
| − | EXEC sp_executesql @parameter
| |
| − | GO
| |
| − | | |
| − | The above procedure shall execute any SQL you pass to it. The directive sp_executesql is a system stored procedure in Microsoft® SQL Server™
| |
| − | Lets pass it.
| |
| − | | |
| − | DROP TABLE ORDERS;
| |
| − | Guess what happens? So we must be careful of not falling into the “We’re secure, we are using stored procedures” trap!
| |
