从fastjson漏洞谈防御式编程

最近,fastjson又爆出一个漏洞,在解析特殊字符的时候,直接OOM:

首先分析一下整体流程:

在scanString时,会直接读取两个字符:

而在next方法中,每次读取都会将bp的值加一(即使没有从输入中读取字符):

1
2
3
4
5
6
public final char next() {
int index = ++bp;
return ch = (index >= this.len ? //
EOI //
: text.charAt(index));
}

在处理完x之后,继续解析剩下的字符。由于没有更多字符了,所以读到的总是EOI,然后进入如下分支:

1
2
3
4
5
6
7
if (ch == EOI) {
if (!isEOF()) {
putChar((char) EOI);
continue;
}
throw new JSONException("unclosed string : " + ch);
}

本来到这一步,isEOF应该是true了,但是isEOF是这样的:

1
2
3
public boolean isEOF() {
return bp == len || ch == EOI && bp + 1 == len;
}

刚刚多读了两个字符,已经导致bp(读到的字符数)超过了len(总的字符数),所以这个isEOF永远返回false,进而永远执行putChar。

问题在哪儿

当然,在这儿的问题在于,fastjson没有考虑到x后面直接没有字符的情况。

但是,再多问一句,为什么这种输入会导致这么严重的问题?

一般来说,这种不合理的情况后面,肯定会有很多不合理的小问题。

防御式编程

显然isEOF函数考虑不周到,读取超出了总长度,这种情况显然是EOF了,但是这个函数没有判断。

当然,很多时候,工程师会说这种情况不会发生,但是工程师确实不知道其他工程师会干出什么事来。(另外,在fastjson中,这种情况会发生)

所以,这才有了防御式编程:不要相信其他的工程师

要是当时isEOF是这样的,那这次问题就不会这么严重了:

1
2
3
public boolean isEOF() {
return bp == len || ch == EOI && bp + 1 >= len;
}

变量应该符合语义

我真的不知道bp是什么的缩写……

但是通过上面isEOF的判断,bp是已读取的字符数量,这样就可以通过bp和len的关系判断是否EOF了。

但是看下next的实现:

1
2
3
4
5
6
public final char next() {
int index = ++bp;
return ch = (index >= this.len ? //
EOI //
: text.charAt(index));
}

已读取的字符数量,在没有实际读取字符的情况下,还加一了???变量值不符合语义,维护上迟早会出问题的吧。

fastjson是如何修复的

看下fastjson官方的修复

嗯,简单直接。

但是这儿明显可以在isEOF方法中处理bp + 1 > len的情况的,在这儿不改,任何地方导致多读了字符串,都会导致OOM的。

作者

Robert Lu

发布于

2019-09-08

许可协议


评论

Robert Lu

关注我的公众号