-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50616/#review144343
-----------------------------------------------------------


Ship it!




Ship It!

- Mahadev Konar


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
> 
>

Reply via email to