----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50616/#review144346 -----------------------------------------------------------
Ship it! Ship It! - Sumit Mohanty On July 29, 2016, 9:19 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50616/ > ----------------------------------------------------------- > > (Updated July 29, 2016, 9:19 p.m.) > > > Review request for Ambari, Mahadev Konar, Sumit Mohanty, and Yusaku Sako. > > > Bugs: AMBARI-17962 > https://issues.apache.org/jira/browse/AMBARI-17962 > > > Repository: ambari > > > Description > ------- > > The Ambari coverity scan found two "High impact security" issues, both SQL > Injections. They are both the same coding issue, but one is in > OracleConnector.java, and one is in the analogous method in > PostgresConnector.java. > > This is the key description: > ``` > CID 167755 (#1 of 1): SQL injection (SQLI)9. sql_taint: Insecure > concatenation of a SQL statement. The value searchClause is tainted. > > Perform one of the following to guard against SQL injection attacks. > * Parameterize the SQL statement using ? positional characters. Bind the > tainted values to the ? positional parameters using one of the > PreparedStatement.set* methods. > * Validate user-supplied values against predefined constant values. > Concatenate these constant values into the SQL statement. > * Cast tainted values to safe types such as integers. Concatenate these type > safe values into the statement. > > [More > Information|https://scan3.coverity.com/doc/en/cov_checker_ref.html#id_sql_generic] > ``` > > This is the one in OracleConnector.java, lines 32 -55: > > ``` > 32 @Override > 8. taint_path_param: Parameter searchClause receives the tainted data. > 33 protected PreparedStatement getQualifiedPS(Statements statement, String > searchClause, Workflows.WorkflowDBEntry.WorkflowFields field, boolean > sortAscending, int offset, int limit) throws IOException { > 34 if (db == null) > 35 throw new IOException("db not initialized"); > 36 > 37 String order = " ORDER BY " + field.toString() + " " + (sortAscending ? > SORT_ASC : SORT_DESC); > 38 > 39 String query = "select * \n" + > 40 " from ( select " + > 41// "/*+ FIRST_ROWS(n) */ \n" + > 42 " a.*, ROWNUM rnum \n" + > 43 " from (" > CID 167755 (#1 of 1): SQL injection (SQLI)9. sql_taint: Insecure > concatenation of a SQL statement. The value searchClause is tainted. > Perform one of the following to guard against SQL injection attacks. > > Parameterize the SQL statement using ? positional characters. Bind the > tainted values to the ? positional parameters using one of the > PreparedStatement.set* methods. > Validate user-supplied values against predefined constant values. > Concatenate these constant values into the SQL statement. > Cast tainted values to safe types such as integers. Concatenate these > type safe values into the statement. > > More Information > 44 + statement.getStatementString() + searchClause + order + > 45 ") a \n" + > 46 " where ROWNUM <= " + (offset + limit) + ") \n" + > 47 "where rnum >= " + offset; > 48 > 49 try { > 10. sql_sink: Passing the tainted value query to the SQL API > java.sql.Connection.prepareStatement(java.lang.String) may allow an attacker > to inject SQL. > 50 return db.prepareStatement(query); > 51 } catch (SQLException e) { > 52 throw new IOException(e); > 53 } > 54 > 55 } > ``` > This is the one in PostgresConnector.java, lines 495-504: > > ``` > 8. taint_path_param: Parameter searchClause receives the tainted data. > 495 protected PreparedStatement getQualifiedPS(Statements statement, String > searchClause) throws IOException { > 496 if (db == null) > 497 throw new IOException("postgres db not initialized"); > 498 try { > 499 // LOG.debug("preparing " + statement.getStatementString() + > searchClause); > CID 167743 (#1 of 1): SQL injection (SQLI)9. sql_taint: Insecure > concatenation of a SQL statement. The value searchClause is tainted. Passing > the tainted command to the SQL API > java.sql.Connection.prepareStatement(java.lang.String) may allow an attacker > to inject SQL. > Perform one of the following to guard against SQL injection attacks. > > Parameterize the SQL statement using ? positional characters. Bind the > tainted values to the ? positional parameters using one of the > PreparedStatement.set* methods. > Validate user-supplied values against predefined constant values. > Concatenate these constant values into the SQL statement. > Cast tainted values to safe types such as integers. Concatenate these > type safe values into the statement. > > More Information > 500 return db.prepareStatement(statement.getStatementString() + > searchClause); > 501 } catch (SQLException e) { > 502 throw new IOException(e); > 503 } > 504 } > ``` > > #Solution > Remove code supporting an unsupported REST API call to obtain jobtracker > information. his entry point is handled by > {{org.apache.ambari.eventdb.webservice.WorkflowJsonService}}. By removing > this class and cleaning up orphaned code, the SQL injection issue list above > will be solved. > > > Diffs > ----- > > ambari-server/src/main/java/org/apache/ambari/eventdb/db/DBConnector.java > b859114 > > ambari-server/src/main/java/org/apache/ambari/eventdb/db/MySQLConnector.java > c59b800 > > ambari-server/src/main/java/org/apache/ambari/eventdb/db/OracleConnector.java > 92a5763 > > ambari-server/src/main/java/org/apache/ambari/eventdb/db/PostgresConnector.java > 421d001 > ambari-server/src/main/java/org/apache/ambari/eventdb/model/DataTable.java > 4fcac8e > ambari-server/src/main/java/org/apache/ambari/eventdb/model/Jobs.java > 75fd3ca > > ambari-server/src/main/java/org/apache/ambari/eventdb/model/TaskAttempt.java > 526d63d > ambari-server/src/main/java/org/apache/ambari/eventdb/model/TaskData.java > 37007df > > ambari-server/src/main/java/org/apache/ambari/eventdb/model/TaskLocalityData.java > e50a1a0 > > ambari-server/src/main/java/org/apache/ambari/eventdb/model/WorkflowContext.java > c2b0468 > > ambari-server/src/main/java/org/apache/ambari/eventdb/model/WorkflowDag.java > 07056bf > ambari-server/src/main/java/org/apache/ambari/eventdb/model/Workflows.java > 541ce79 > > ambari-server/src/main/java/org/apache/ambari/eventdb/webservice/JAXBContextResolver.java > 974155c > > ambari-server/src/main/java/org/apache/ambari/eventdb/webservice/WorkflowJsonService.java > 3ef56ab > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariServer.java > 5d0bb18 > > Diff: https://reviews.apache.org/r/50616/diff/ > > > Testing > ------- > > # Local test results: > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 1:11:45.717s > [INFO] Finished at: Fri Jul 29 15:04:53 EDT 2016 > [INFO] Final Memory: 61M/1865M > [INFO] > ------------------------------------------------------------------------ > > # Jekins test results: PENDING > > > Thanks, > > Robert Levas > >
