midsuit
/
core-jgi
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Carlos Ruiz 2022-10-16 16:21:05 +02:00 committed by GitHub
parent d421dba94e
commit 7ffb18b8e2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 106 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
migration/i9/oracle/202210131724_IDEMPIERE-5450.sql Normal file
View File

@ -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')
;

7
migration/i9/postgresql/202210131724_IDEMPIERE-5450.sql Normal file
View File

@ -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')
;

3
org.adempiere.base/src/org/compiere/model/MSysConfig.java
View File

@ -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 ADDRESS_VALIDATION = "ADDRESS_VALIDATION";
public static final String ALERT_SEND_ATTACHMENT_AS_XLS = "ALERT_SEND_ATTACHMENT_AS_XLS"; 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 ALLOCATION_DESCRIPTION = "ALLOCATION_DESCRIPTION";
public static final String ALLOW_APPLY_PAYMENT_TO_CREDITMEMO = "ALLOW_APPLY_PAYMENT_TO_CREDITMEMO"; 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_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 ALLOW_REVERSAL_OF_RECONCILED_PAYMENT = "ALLOW_REVERSAL_OF_RECONCILED_PAYMENT";
public static final String ALogin_ShowDate = "ALogin_ShowDate"; public static final String ALogin_ShowDate = "ALogin_ShowDate";

1
org.adempiere.base/src/org/compiere/model/SystemIDs.java
View File

@ -66,6 +66,7 @@ public class SystemIDs
public final static int FORM_SETUP_WIZARD = 200000; public final static int FORM_SETUP_WIZARD = 200000;
public final static int FORM_ADD_AUTHORIZATION = 200016; public final static int FORM_ADD_AUTHORIZATION = 200016;
public final static int FORM_MFA_REGISTER = 200017; public final static int FORM_MFA_REGISTER = 200017;
public final static int FORM_SQL_PROCESS = 111;
public final static int MENU_NOTICE = 233; public final static int MENU_NOTICE = 233;

149
org.adempiere.ui.zk/WEB-INF/src/org/adempiere/webui/apps/form/WSQLProcess.java
View File

@ -21,6 +21,7 @@
package org.adempiere.webui.apps.form; package org.adempiere.webui.apps.form;
import java.math.BigDecimal;
import java.sql.Connection; import java.sql.Connection;
import java.sql.SQLException; import java.sql.SQLException;
import java.sql.Statement; import java.sql.Statement;
@ -36,6 +37,9 @@ import org.adempiere.webui.component.Textbox;
import org.adempiere.webui.panel.ADForm; import org.adempiere.webui.panel.ADForm;
import org.adempiere.webui.theme.ThemeManager; import org.adempiere.webui.theme.ThemeManager;
import org.adempiere.webui.util.ZKUpdateUtil; 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.CLogger;
import org.compiere.util.DB; import org.compiere.util.DB;
import org.compiere.util.Env; import org.compiere.util.Env;
@ -61,16 +65,11 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
/** /**
* *
*/ */
private static final long serialVersionUID = -2038792517003449189L; private static final long serialVersionUID = -4661224754061326223L;
/** Log. */ /** Log. */
private static final CLogger log = CLogger.getCLogger(WSQLProcess.class); 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. */ /** Grid used to layout components. */
private Grid m_grdMain = new Grid(); private Grid m_grdMain = new Grid();
/** SQL label. */ /** SQL label. */
@ -82,17 +81,30 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
/** Field to hold result of SQL statement execution. */ /** Field to hold result of SQL statement execution. */
private Textbox m_txbResultField = new Textbox(); 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. * Default constructor.
*/ */
public WSQLProcess() public WSQLProcess() {
{
super(); super();
} }
@Override @Override
protected void initForm() protected void initForm() {
{
Row rwTop = new Row(); Row rwTop = new Row();
Row rwBottom = new Row(); Row rwBottom = new Row();
Rows rows = new Rows(); Rows rows = new Rows();
@ -151,8 +163,7 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
* Create Process Button. * Create Process Button.
* @return button * @return button
*/ */
public static final Button createProcessButton() public static final Button createProcessButton() {
{
Button btnProcess = new Button(); Button btnProcess = new Button();
if(ThemeManager.isUseFontIconForImage()) if(ThemeManager.isUseFontIconForImage())
btnProcess.setIconSclass("z-icon-Process"); btnProcess.setIconSclass("z-icon-Process");
@ -164,22 +175,20 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
} // createProcessButton } // 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 sqlStatements one or more statements separated by a semicolon (';')
* @param allowDML whether to allow DML statements * @return a string summarizing the results
* @return a string summarising the results
*/ */
public static String processStatements (String sqlStatements, boolean allowDML) public static String processStatements (String sqlStatements) {
{
if (sqlStatements == null || sqlStatements.length() == 0) if (sqlStatements == null || sqlStatements.length() == 0)
return ""; return "";
StringBuilder result = new StringBuilder(); StringBuilder result = new StringBuilder();
// //
// TODO: known issue - the command cannot contain semicolon within quotes
StringTokenizer st = new StringTokenizer(sqlStatements, ";", false); StringTokenizer st = new StringTokenizer(sqlStatements, ";", false);
while (st.hasMoreTokens()) while (st.hasMoreTokens()) {
{ result.append(processStatement(st.nextToken()));
result.append(processStatement(st.nextToken(), allowDML));
result.append(Env.NL); result.append(Env.NL);
} }
// //
@ -190,82 +199,84 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
* Process SQL Statements. * Process SQL Statements.
* *
* @param sqlStatement a single SQL statement * @param sqlStatement a single SQL statement
* @param allowDML whether to allow DML statements * @return a string summarizing the results
* @return a string summarising the results
*/ */
public static String processStatement (String sqlStatement, boolean allowDML) public static String processStatement (String sqlStatement) {
{
if (sqlStatement == null) if (sqlStatement == null)
return ""; return "";
StringBuilder sb = new StringBuilder(); StringBuilder sb = new StringBuilder();
char[] chars = sqlStatement.toCharArray(); char[] chars = sqlStatement.toCharArray();
for (int i = 0; i < chars.length; i++) for (int i = 0; i < chars.length; i++) {
{
char c = chars[i]; char c = chars[i];
if (Character.isWhitespace(c)) if (Character.isWhitespace(c))
sb.append(' '); sb.append(' ');
else else
sb.append(c); sb.append(c);
} }
String sql = sb.toString().trim(); String sql = sb.toString();
if (sql.length() == 0) if (sql.trim().length() == 0)
return ""; return "";
// //
StringBuffer result = new StringBuffer("SQL> ") 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(sql)
.append(Env.NL); .append(Env.NL);
if (!allowDML)
{
boolean error = false;
String SQL = sql.toUpperCase(); String SQL = sql.toUpperCase();
for (int i = 0; i < DML_KEYWORDS.length; i++) String cleanSQL = SQL
{ .replaceAll(REGEX_REMOVE_COMMENTS, "")
if (SQL.startsWith(DML_KEYWORDS[i] + " ") .replaceAll(REGEX_REMOVE_QUOTED_STRINGS, "")
|| SQL.indexOf(" " + DML_KEYWORDS[i] + " ") != -1 .replaceFirst(REGEX_REMOVE_LEADING_SPACES, "");
|| SQL.indexOf("(" + DML_KEYWORDS[i] + " ") != -1)
{ 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(",");
result.append("===> ERROR: Not Allowed Keyword ") boolean error = true;
.append(DML_KEYWORDS[i]) for (int i = 0; i < allowedKeywords.length; i++) {
if (cleanSQL.startsWith(allowedKeywords[i] + " ")) {
error = false;
break;
}
}
if (error) {
result.append("===> ERROR: Not Allowed Command")
.append(Env.NL); .append(Env.NL);
error = true;
}
}
if (error)
return result.toString(); return result.toString();
} // !allowDML }
// Process // Process
Connection conn = DB.createConnection(true, Connection.TRANSACTION_READ_COMMITTED); Connection conn = DB.createConnection(true, Connection.TRANSACTION_READ_COMMITTED);
Statement stmt = null; Statement stmt = null;
try try {
{
stmt = conn.createStatement(); stmt = conn.createStatement();
@SuppressWarnings("unused") long start = System.currentTimeMillis();
boolean OK = stmt.execute(sql); 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(); int count = stmt.getUpdateCount();
if (count == -1) if (count == -1) {
{
result.append("---> ResultSet"); result.append("---> ResultSet");
} }
else else
result.append("---> Result=").append(count); result.append("---> Result=").append(count);
} MIssue issue = new MIssue(Env.getCtx(), 0, null);
catch (SQLException e) 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(); String exception = e.toString();
log.log(Level.SEVERE, "process statement: " + sql + " - " + exception); log.log(Level.SEVERE, "process statement: " + sql + " - " + exception);
result.append("===> ").append(exception); result.append("===> ").append(exception);
} } finally {
finally
{
DB.close(stmt); DB.close(stmt);
stmt = null; stmt = null;
try try {
{
conn.close(); conn.close();
} } catch (SQLException e2) {
catch (SQLException e2)
{
log.log(Level.SEVERE, "processStatement - close connection", e2); log.log(Level.SEVERE, "processStatement - close connection", e2);
} }
conn = null; conn = null;
@ -275,14 +286,14 @@ public class WSQLProcess extends ADForm implements EventListener<Event>
return result.toString(); return result.toString();
} }
/**
/* * Process the events for this form
* (non-Javadoc) * @param event
* @see org.adempiere.webui.panel.ADForm#onEvent(org.zkoss.zk.ui.event.Event)
*/ */
public void onEvent(Event event) throws Exception public void onEvent(Event event) throws Exception {
{ if (event.getTarget() == m_btnSql)
m_txbResultField.setText(processStatements (m_txbSqlField.getText(), false)); m_txbResultField.setText(processStatements (m_txbSqlField.getText()));
super.onEvent(event); super.onEvent(event);
} }
} }