From 7ffb18b8e25a23257b4cb2f51fd1ad1454886a7a Mon Sep 17 00:00:00 2001 From: Carlos Ruiz Date: Sun, 16 Oct 2022 16:21:05 +0200 Subject: [PATCH] IDEMPIERE-5450 Form SQL Process has security issues (#1528) * IDEMPIERE-5450 Form SQL Process has security issues * - fix javadoc * - remove unnecessary code * - minor improvement * - improve discovery of single word commands - avoids the need of space in the SysConfig key --- .../i9/oracle/202210131724_IDEMPIERE-5450.sql | 10 ++ .../202210131724_IDEMPIERE-5450.sql | 7 + .../src/org/compiere/model/MSysConfig.java | 3 +- .../src/org/compiere/model/SystemIDs.java | 1 + .../webui/apps/form/WSQLProcess.java | 161 ++++++++++-------- 5 files changed, 106 insertions(+), 76 deletions(-) create mode 100644 migration/i9/oracle/202210131724_IDEMPIERE-5450.sql create mode 100644 migration/i9/postgresql/202210131724_IDEMPIERE-5450.sql diff --git a/migration/i9/oracle/202210131724_IDEMPIERE-5450.sql b/migration/i9/oracle/202210131724_IDEMPIERE-5450.sql new file mode 100644 index 0000000000..8a147bba0b --- /dev/null +++ b/migration/i9/oracle/202210131724_IDEMPIERE-5450.sql @@ -0,0 +1,10 @@ +-- IDEMPIERE-5450 Form SQL Process has security issues +SELECT register_migration_script('202210131724_IDEMPIERE-5450.sql') FROM dual; + +SET SQLBLANKLINES ON +SET DEFINE OFF + +-- Oct 13, 2022, 5:24:11 PM CEST +INSERT INTO AD_SysConfig (AD_SysConfig_ID,AD_Client_ID,AD_Org_ID,Created,Updated,CreatedBy,UpdatedBy,IsActive,Name,Value,Description,EntityType,ConfigurationLevel,AD_SysConfig_UU) VALUES (200209,0,0,TO_TIMESTAMP('2022-10-13 17:24:11','YYYY-MM-DD HH24:MI:SS'),TO_TIMESTAMP('2022-10-13 17:24:11','YYYY-MM-DD HH24:MI:SS'),100,100,'Y','ALLOWED_KEYWORDS_IN_SQL_FORM','ALTER,ANALYZE,COMMENT,CREATE,DELETE,DROP,GRANT,INSERT,REINDEX,REVOKE,SET,UPDATE,TRUNCATE,VACUUM','Comma separated list of commands allowed in the SQL Process form','D','S','52f9e30d-4a15-4a63-890c-c338e9f20f61') +; + diff --git a/migration/i9/postgresql/202210131724_IDEMPIERE-5450.sql b/migration/i9/postgresql/202210131724_IDEMPIERE-5450.sql new file mode 100644 index 0000000000..6db41a39fb --- /dev/null +++ b/migration/i9/postgresql/202210131724_IDEMPIERE-5450.sql @@ -0,0 +1,7 @@ +-- IDEMPIERE-5450 Form SQL Process has security issues +SELECT register_migration_script('202210131724_IDEMPIERE-5450.sql') FROM dual; + +-- Oct 13, 2022, 5:24:11 PM CEST +INSERT INTO AD_SysConfig (AD_SysConfig_ID,AD_Client_ID,AD_Org_ID,Created,Updated,CreatedBy,UpdatedBy,IsActive,Name,Value,Description,EntityType,ConfigurationLevel,AD_SysConfig_UU) VALUES (200209,0,0,TO_TIMESTAMP('2022-10-13 17:24:11','YYYY-MM-DD HH24:MI:SS'),TO_TIMESTAMP('2022-10-13 17:24:11','YYYY-MM-DD HH24:MI:SS'),100,100,'Y','ALLOWED_KEYWORDS_IN_SQL_FORM','ALTER,ANALYZE,COMMENT,CREATE,DELETE,DROP,GRANT,INSERT,REINDEX,REVOKE,SET,UPDATE,TRUNCATE,VACUUM','Comma separated list of commands allowed in the SQL Process form','D','S','52f9e30d-4a15-4a63-890c-c338e9f20f61') +; + diff --git a/org.adempiere.base/src/org/compiere/model/MSysConfig.java b/org.adempiere.base/src/org/compiere/model/MSysConfig.java index 4e029b5b65..73ebe83154 100644 --- a/org.adempiere.base/src/org/compiere/model/MSysConfig.java +++ b/org.adempiere.base/src/org/compiere/model/MSysConfig.java @@ -44,12 +44,13 @@ public class MSysConfig extends X_AD_SysConfig /** * */ - private static final long serialVersionUID = -5704354666566021353L; + private static final long serialVersionUID = -5465323173669763683L; public static final String ADDRESS_VALIDATION = "ADDRESS_VALIDATION"; public static final String ALERT_SEND_ATTACHMENT_AS_XLS = "ALERT_SEND_ATTACHMENT_AS_XLS"; public static final String ALLOCATION_DESCRIPTION = "ALLOCATION_DESCRIPTION"; public static final String ALLOW_APPLY_PAYMENT_TO_CREDITMEMO = "ALLOW_APPLY_PAYMENT_TO_CREDITMEMO"; + public static final String ALLOWED_KEYWORDS_IN_SQL_FORM = "ALLOWED_KEYWORDS_IN_SQL_FORM"; public static final String ALLOW_OVER_APPLIED_PAYMENT = "ALLOW_OVER_APPLIED_PAYMENT"; public static final String ALLOW_REVERSAL_OF_RECONCILED_PAYMENT = "ALLOW_REVERSAL_OF_RECONCILED_PAYMENT"; public static final String ALogin_ShowDate = "ALogin_ShowDate"; diff --git a/org.adempiere.base/src/org/compiere/model/SystemIDs.java b/org.adempiere.base/src/org/compiere/model/SystemIDs.java index ae2046d844..3ef88880e7 100644 --- a/org.adempiere.base/src/org/compiere/model/SystemIDs.java +++ b/org.adempiere.base/src/org/compiere/model/SystemIDs.java @@ -66,6 +66,7 @@ public class SystemIDs public final static int FORM_SETUP_WIZARD = 200000; public final static int FORM_ADD_AUTHORIZATION = 200016; public final static int FORM_MFA_REGISTER = 200017; + public final static int FORM_SQL_PROCESS = 111; public final static int MENU_NOTICE = 233; diff --git a/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/form/WSQLProcess.java b/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/form/WSQLProcess.java index 1cc574325f..648f420c0b 100644 --- a/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/form/WSQLProcess.java +++ b/org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/form/WSQLProcess.java @@ -21,6 +21,7 @@ package org.adempiere.webui.apps.form; +import java.math.BigDecimal; import java.sql.Connection; import java.sql.SQLException; import java.sql.Statement; @@ -36,6 +37,9 @@ import org.adempiere.webui.component.Textbox; import org.adempiere.webui.panel.ADForm; import org.adempiere.webui.theme.ThemeManager; import org.adempiere.webui.util.ZKUpdateUtil; +import org.compiere.model.MIssue; +import org.compiere.model.MSysConfig; +import org.compiere.model.SystemIDs; import org.compiere.util.CLogger; import org.compiere.util.DB; import org.compiere.util.Env; @@ -61,16 +65,11 @@ public class WSQLProcess extends ADForm implements EventListener /** * */ - private static final long serialVersionUID = -2038792517003449189L; - + private static final long serialVersionUID = -4661224754061326223L; + /** Log. */ private static final CLogger log = CLogger.getCLogger(WSQLProcess.class); - /** DML Statement */ - private static final String[] DML_KEYWORDS = new String[]{ - "SELECT", "UPDATE", "DELETE", "TRUNCATE" - }; - /** Grid used to layout components. */ private Grid m_grdMain = new Grid(); /** SQL label. */ @@ -82,17 +81,30 @@ public class WSQLProcess extends ADForm implements EventListener /** Field to hold result of SQL statement execution. */ private Textbox m_txbResultField = new Textbox(); + /** + * REGEX_REMOVE_COMMENTS + */ + private static final String REGEX_REMOVE_COMMENTS = "/\\*(?:.|[\\n\\r])*?\\*/"; + + /** + * REGEX_REMOVE_QUOTED_STRINGS + */ + private static final String REGEX_REMOVE_QUOTED_STRINGS = "'(?:.|[\\n\\r])*?'"; + + /** + * REGEX_REMOVE_LEADING_SPACES + */ + private static final String REGEX_REMOVE_LEADING_SPACES = "^\\s+"; + /** * Default constructor. */ - public WSQLProcess() - { + public WSQLProcess() { super(); } @Override - protected void initForm() - { + protected void initForm() { Row rwTop = new Row(); Row rwBottom = new Row(); Rows rows = new Rows(); @@ -151,8 +163,7 @@ public class WSQLProcess extends ADForm implements EventListener * Create Process Button. * @return button */ - public static final Button createProcessButton() - { + public static final Button createProcessButton() { Button btnProcess = new Button(); if(ThemeManager.isUseFontIconForImage()) btnProcess.setIconSclass("z-icon-Process"); @@ -164,22 +175,20 @@ public class WSQLProcess extends ADForm implements EventListener } // createProcessButton /** - * Process a semicolon delimitted list of SQL Statements. + * Process a semicolon delimited list of SQL Statements. * * @param sqlStatements one or more statements separated by a semicolon (';') - * @param allowDML whether to allow DML statements - * @return a string summarising the results + * @return a string summarizing the results */ - public static String processStatements (String sqlStatements, boolean allowDML) - { + public static String processStatements (String sqlStatements) { if (sqlStatements == null || sqlStatements.length() == 0) return ""; StringBuilder result = new StringBuilder(); // + // TODO: known issue - the command cannot contain semicolon within quotes StringTokenizer st = new StringTokenizer(sqlStatements, ";", false); - while (st.hasMoreTokens()) - { - result.append(processStatement(st.nextToken(), allowDML)); + while (st.hasMoreTokens()) { + result.append(processStatement(st.nextToken())); result.append(Env.NL); } // @@ -190,99 +199,101 @@ public class WSQLProcess extends ADForm implements EventListener * Process SQL Statements. * * @param sqlStatement a single SQL statement - * @param allowDML whether to allow DML statements - * @return a string summarising the results + * @return a string summarizing the results */ - public static String processStatement (String sqlStatement, boolean allowDML) - { + public static String processStatement (String sqlStatement) { if (sqlStatement == null) return ""; StringBuilder sb = new StringBuilder(); char[] chars = sqlStatement.toCharArray(); - for (int i = 0; i < chars.length; i++) - { + for (int i = 0; i < chars.length; i++) { char c = chars[i]; if (Character.isWhitespace(c)) sb.append(' '); else sb.append(c); } - String sql = sb.toString().trim(); - if (sql.length() == 0) + String sql = sb.toString(); + if (sql.trim().length() == 0) return ""; // - StringBuffer result = new StringBuffer("SQL> ") - .append(sql) - .append(Env.NL); - if (!allowDML) - { - boolean error = false; - String SQL = sql.toUpperCase(); - for (int i = 0; i < DML_KEYWORDS.length; i++) - { - if (SQL.startsWith(DML_KEYWORDS[i] + " ") - || SQL.indexOf(" " + DML_KEYWORDS[i] + " ") != -1 - || SQL.indexOf("(" + DML_KEYWORDS[i] + " ") != -1) - { - result.append("===> ERROR: Not Allowed Keyword ") - .append(DML_KEYWORDS[i]) - .append(Env.NL); - error = true; - } + if (!sql.contains(" ")) // command is a single word (f.e. ANALYZE), add space at the end + sql += " "; + StringBuilder result = new StringBuilder("SQL> ") + .append(sql) + .append(Env.NL); + String SQL = sql.toUpperCase(); + String cleanSQL = SQL + .replaceAll(REGEX_REMOVE_COMMENTS, "") + .replaceAll(REGEX_REMOVE_QUOTED_STRINGS, "") + .replaceFirst(REGEX_REMOVE_LEADING_SPACES, ""); + + String[] allowedKeywords = MSysConfig.getValue(MSysConfig.ALLOWED_KEYWORDS_IN_SQL_FORM, "ALTER,ANALYZE,COMMENT,CREATE,DELETE,DROP,GRANT,INSERT,REINDEX,REVOKE,SET,UPDATE,TRUNCATE,VACUUM").split(","); + boolean error = true; + for (int i = 0; i < allowedKeywords.length; i++) { + if (cleanSQL.startsWith(allowedKeywords[i] + " ")) { + error = false; + break; } - if (error) - return result.toString(); - } // !allowDML + } + if (error) { + result.append("===> ERROR: Not Allowed Command") + .append(Env.NL); + return result.toString(); + } // Process Connection conn = DB.createConnection(true, Connection.TRANSACTION_READ_COMMITTED); Statement stmt = null; - try - { + try { stmt = conn.createStatement(); - @SuppressWarnings("unused") - boolean OK = stmt.execute(sql); + long start = System.currentTimeMillis(); + stmt.execute(sql); + long end = System.currentTimeMillis(); + BigDecimal durationSeconds = BigDecimal.valueOf(end).subtract(BigDecimal.valueOf(start)).divide(BigDecimal.valueOf(1000.0)); int count = stmt.getUpdateCount(); - if (count == -1) - { + if (count == -1) { result.append("---> ResultSet"); } else result.append("---> Result=").append(count); - } - catch (SQLException e) - { + MIssue issue = new MIssue(Env.getCtx(), 0, null); + issue.setIssueSummary("SQL executed on SQL Process form"); + issue.setStackTrace(sql); + issue.setResponseText(result.toString().replace("SQL> " + sql + Env.NL, "")); + issue.setIssueSource(MIssue.ISSUESOURCE_Form); + issue.setUserName(Env.getContext(Env.getCtx(), Env.AD_USER_NAME)); + issue.setAD_Form_ID(SystemIDs.FORM_SQL_PROCESS); + issue.setProcessed(true); + issue.setComments("Duration : " + durationSeconds + " seconds"); + issue.saveEx(); + } catch (SQLException e) { String exception = e.toString(); log.log(Level.SEVERE, "process statement: " + sql + " - " + exception); result.append("===> ").append(exception); - } - finally - { + } finally { DB.close(stmt); stmt = null; - try - { + try { conn.close(); - } - catch (SQLException e2) - { + } catch (SQLException e2) { log.log(Level.SEVERE, "processStatement - close connection", e2); } conn = null; - } + } // result.append(Env.NL); return result.toString(); } - - /* - * (non-Javadoc) - * @see org.adempiere.webui.panel.ADForm#onEvent(org.zkoss.zk.ui.event.Event) + /** + * Process the events for this form + * @param event */ - public void onEvent(Event event) throws Exception - { - m_txbResultField.setText(processStatements (m_txbSqlField.getText(), false)); + public void onEvent(Event event) throws Exception { + if (event.getTarget() == m_btnSql) + m_txbResultField.setText(processStatements (m_txbSqlField.getText())); super.onEvent(event); } + }