ALARM_MESSAGE Sql Inject

[Jumbo-WJB]

ALARM_MESSAGE Sql Inject

file path:
https://github.com/apache/skywalking/blob/master/oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/mysql/MySQLAlarmQueryDAO.java#L63

The parameters are spliced directly into the sql query, causing sql injection：
request:

POST /graphql HTTP/1.1
Host: host:8080
Content-Length: 479
Accept: application/json, text/plain, */*
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.106 Safari/537.36
Content-Type: application/json;charset=UTF-8
Origin: http://host:8080
Referer: http://host:8080/alarm
Accept-Encoding: gzip, deflate
Accept-Language: zh-CN,zh;q=0.9,en;q=0.8
Connection: close

{"query":"query queryAlarms($keyword: String, $scope: Scope, $duration:Duration!, $paging: Pagination!) {\n    getAlarm(keyword: $keyword, scope: $scope, duration: $duration, paging: $paging) {\n      items: msgs {\n        key: id\n        message\n        startTime\n        scope\n      }\n      total\n    }}","variables":{"duration":{"start":"2020-06-20 2323","end":"2020-06-20 2338","step":"MINUTE"},"paging":{"pageNum":1,"pageSize":20,"needTotal":true},"keyword":"1111'"}}

response:

HTTP/1.1 200 
X-Application-Context: application:8080
Date: Sat, 20 Jun 2020 17:50:18 GMT
Content-Type: application/json;charset=utf-8
Connection: close
Content-Length: 488

{"data":{},"errors":[{"message":"Exception while fetching data (/getAlarm) : Syntax error in SQL statement \"select count(1) total from (select 1 from alarm_record where  1=1  and time_bucket >= ? and time_bucket <= ? and alarm_message like '%1111'%[*]'  order by start_time desc  )\"; SQL statement:\nselect count(1) total from (select 1 from alarm_record where  1=1  and time_bucket >= ? and time_bucket <= ? and alarm_message like '%1111'%'  order by start_time desc  ) [42000-196]"}]}

[wu-sheng]

Any interest to help on fixing this?

[Jumbo-WJB]

of course，you can refer to #4639 to fix this vuln.

[wu-sheng]

@Jumbo-WJB I know how to fix. My question is, should we wait for your pull request?

[neargle]

@wu-sheng Hi.
It is better not to fix only the single point in MySQLAlarmQueryDAO.java#L63 here, it seems that there are many codes using coding methods that maybe at risk of SQL injection.
It is possible to have SQL injection problems when appending a string (or other variable that can be coerced into string type) that can be controlled by an user to the SQL statement for direct sql query.
Using CodeQL and grep to filter a part of them, the following is a possible list:

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/h2/dao/H2AggregationQueryDAO.java

additionalConditions.forEach(condition -> {
sql.append(" and ").append(condition.getKey()).append("=?");
conditions.add(condition.getValue());

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/h2/dao/H2AlarmQueryDAO.java

if (!Strings.isNullOrEmpty(keyword)) {
    sql.append(" and ").append(AlarmRecord.ALARM_MESSAGE).append(" like '%").append(keyword).append("%' ");
}

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/h2/dao/H2LogQueryDAO.java

sql.append("from ").append(metricName).append(" where ");
sql.append(" 1=1 ");

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/h2/dao/H2MetadataQueryDAO.java

if (!Strings.isNullOrEmpty(keyword)) {
    sql.append(" and ").append(ServiceTraffic.NAME).append(" like \"%").append(keyword).append("%\"");
}
sql.append(" limit ").append(metadataQueryMaxSize);

if (!Strings.isNullOrEmpty(keyword)) {
    sql.append(" and ").append(EndpointTraffic.NAME).append(" like '%").append(keyword).append("%' ");
}

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/h2/dao/H2TopNRecordsQueryDAO.java

     }
sql.append(" limit ").append(condition.getTopN());

oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/mysql/MySQLAlarmQueryDAO.java

if (!Strings.isNullOrEmpty(keyword)) {
    sql.append(" and ").append(AlarmRecord.ALARM_MESSAGE).append(" like '%").append(keyword).append("%' ");
}

[wu-sheng]

You could send a pull request to fix all these directly. We prefer our contributors could do this.

[wu-sheng]

Also, as MySQL is not 1st priority/recommended storage in the product env, we want to encourage people to join us.