fix(wechat_qrcode): Init nBytes after the count value is determined #3480

Konano · 2023-04-24T06:07:37Z

Merge with test case: opencv/opencv_extra#1061

A malicious crafted image will crash the wechat_qrcode module by invalid memory access.

The earliest PoC by @GZTimeWalker: https://gist.github.com/GZTimeWalker/3ca70a8af2f5830711e9cccc73fb5270

Sample image:

In DecodedBitStreamParser::decodeByteSegment, an attacker can add a byte segment with the Character Count Indicator of non-zero but empty content at the end of the data segment, where available is 0 and count is updated to 0, but nBytes remains non-zero.

opencv_contrib/modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

Lines 193 to 200 in 960b3f6

    
           int nBytes = count; 
        
           BitSource& bits(*bits_); 
        
           // Don't crash trying to read more bits than we have available. 
        
           int available = bits.available(); 
        
           // try to repair count data if count data is invalid 
        
           if (count * 8 > available) { 
        
               count = (available + 7 / 8); 
        
           }

Since count is 0, the readBytes in L203 is a null pointer.

opencv_contrib/modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

Lines 202 to 203 in 960b3f6

    
           ArrayRef<char> bytes_(count); 
        
           char* readBytes = &(*bytes_)[0];

Then DecodedBitStreamParser::decodeByteSegment will call DecodedBitStreamParser::append, where the nBytes parameter is not 0.

opencv_contrib/modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

Line 227 in 960b3f6

append(result, readBytes, nBytes, err_handler);

Eventually the program will crash at line 108 because it accesses a null pointer.

opencv_contrib/modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

Line 108 in 960b3f6

result.append((const char*)bufIn, nIn);

We think the essence of the issue is that count was modified without updating nBytes, resulting in inconsistent values for the two variables. Unlike #3479, we think that the initialization of nBytes should be done after the value of count has been determined, which helps avoid other potential errors.

Moreover, we also found some wrong calculations in line 199, which are corrected together in this PR.

Also fixed #3478.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
The PR is proposed to the proper branch
There is a reference to the original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

…ined

ssst0n3 · 2023-04-24T07:01:06Z

Amazing, too fast

k3lpi3b4nsh33 · 2023-04-24T07:08:15Z

Orz!!

TUGOhost · 2023-04-24T07:10:53Z

amazing

syuy · 2023-04-24T07:11:30Z

Cool!

Lakr233 · 2023-04-24T07:22:42Z

LGTM

helin0815 · 2023-04-24T07:39:19Z

太猛了哥

KarKLi · 2023-04-24T07:45:26Z

amazing!

KongWeibin · 2023-04-24T07:49:08Z

interesting

Timon-2022 · 2023-04-24T08:05:11Z

niuwa!!!!mobai dalao!

Naville · 2023-04-24T08:10:58Z

ChatGPT 3.5:

What's the bug in this code?

int available = bits.available();
     // try to repair count data if count data is invalid
     if (count * 8 > available) {
         count = (available + 7 / 8);
     }

     ArrayRef<char> bytes_(count);

There is a bug in the line count = (available + 7 / 8);.
The expression 7/8 will be evaluated as integer division, resulting in a value of 0. Therefore, count will always be set to > available rather than being adjusted to the correct number of bytes needed to store the count data.
To fix the bug, the expression should be count = (available + 7) / 8;, which will correctly round up to the nearest integer number of bytes needed to store the count data.

yilongli-tubi · 2023-04-24T09:16:07Z

Amazing! This bug seems to just crash my WeChat client program...

yoghurt-lee · 2023-04-24T09:16:50Z

amazing！

Nagico · 2023-04-24T09:43:12Z

~~But today is not Thursday~~😂

DapengFeng · 2023-04-24T11:07:44Z

LGTM

sfc9982 · 2023-04-24T11:16:43Z

Long live these volunteers! ❤️

UPON-2021 · 2023-04-24T11:41:44Z

amazing！

Apple-QAQ · 2023-04-24T11:57:15Z

Too Fast!

xieyumc · 2023-04-24T12:06:42Z

QQ和微信都会crash

NoahZhang1 · 2023-04-24T13:02:03Z

Cool!! Really fast

mochaaP

lgtm

Konano · 2023-04-24T14:10:19Z

@opencv-alalek @asmorkalov Would it be possible to review and merge of my pull request? Thank you very much!

HRan2004 · 2023-04-24T15:05:53Z

666

RJHD-GIT · 2023-04-24T15:31:39Z

有品

JulianSong · 2023-04-25T06:17:32Z

LGTM

WanliZhong

@Konano @GZTimeWalker Hi, thanks for your contribution! Your patch will be the main branch to fix this bug. There are some comments:

put a test image to opencv_extra/testdata/cv/qrcode/
create a new pr to opencv_extra which branch name should be patch-1 in Konano’s repository.
then update your code in opencv_contrib, the CI test will automatically pull the new test data to test your pr

add test code in modules/wechat_qrcode/test/test_qrcode.cpp

TEST(Objdetect_QRCode_bug, issue_3478) {
    auto detector = wechat_qrcode::WeChatQRCode();
    std::string image_path = findDataFile("qrcode/issue_3478.png");
    Mat src = imread(image_path, IMREAD_GRAYSCALE);
    ASSERT_FALSE(src.empty()) << "Can't read image: " << image_path;
    std::vector<std::string> outs = detector.detectAndDecode(src);
    ASSERT_EQ(1, (int) outs.size());
    ASSERT_EQ(16, (int) outs[0].size());
    ASSERT_EQ("KFCVW50         ", outs[0]);
}

after testing, your method does not pass the test sample on windows platform

I suggest you add code at Line 202

// issue https://github.com/opencv/opencv_contrib/issues/3478
if (bytes_->empty())
    return;

Konano · 2023-04-25T06:47:00Z

@WanliZhong Thank you for your help!

You mentioned that our method did not pass the test sample on windows platform, so I checked the test logs and the error message for test failure is as follows:

unknown file: error: C++ exception with description "OpenCV(4.7.0-dev) C:\build\precommit-contrib_windows64\4.x\opencv\modules\ts\src\ts.cpp:1071: error: (-2:Unspecified error) OpenCV tests: Can't find required data file: qrcode/issue_3478.png in function 'cvtest::findData'

" thrown in the test body.

It looks like the new test case is not loaded?

WanliZhong · 2023-04-25T06:49:18Z

@WanliZhong Thank you for your help!

You mentioned that our method did not pass the test sample on windows platform, so I checked the test logs and the error message for test failure is as follows:
unknown file: error: C++ exception with description "OpenCV(4.7.0-dev) C:\build\precommit-contrib_windows64\4.x\opencv\modules\ts\src\ts.cpp:1071: error: (-2:Unspecified error) OpenCV tests: Can't find required data file: qrcode/issue_3478.png in function 'cvtest::findData'

" thrown in the test body.
It looks like the new test case is not loaded?

Before runing the test, you should add an env variable to refer the test data OPENCV_TEST_DATA_PATH=path/to/opencv_extra/testdata

GZTimeWalker · 2023-04-25T06:55:44Z

@WanliZhong All changes are applied, and we updated the branch https://github.com/Konano/opencv_extra/tree/patch-1

The error on windows x64 are caused by the wrong environment variables, we tested the code localy, it works fine.

WanliZhong · 2023-04-25T06:58:20Z

Good! Let's check if the test can be passed on all platform

Henryk07 · 2023-04-25T07:20:14Z

so fast!! 厉害！

modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

zihaomu

Thanks for your contribution! 👍

zihaomu · 2023-04-25T08:02:17Z

Please take a look. @asmorkalov, @opencv-alalek, @vpisarev.
This error only occurs in wechat_qrcode, but not in the qrcode of opencv/opencv.

WanliZhong · 2023-04-25T08:15:09Z

There is a slight difference between qrcode and wechat_qrcode. qrcode module will return an empty string, since the qrcode is considered to be wrong case. But wechat_qrcode will ignore the error and return "KFCVW50 "

GZTimeWalker · 2023-04-25T08:20:00Z

There is a slight difference between qrcode and wechat_qrcode. qrcode module will return an empty string, since the qrcode is considered to be wrong case. But wechat_qrcode will ignore the error and return "KFCVW50 "

From my point of view, since the data in the mixed mode of the QRCode is parsed from many segments, a decoder should have to output as many partially correct results as possible.

fengyuentau · 2023-04-25T08:21:38Z

modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp

+    size_t nBytes = count;
+
+    ArrayRef<char> bytes_(nBytes);
+    // issue https://github.com/opencv/opencv_contrib/issues/3478
+    if (bytes_->empty())
+        return;


If I understand correctly, bytes_->empty() should be equivalent to nBytes == 0. So why not using nBytes == 0 for simplicity and efficiency?

I think this change would be a more visual indication of the intention to avoid null pointers?

Konano · 2023-04-25T08:39:04Z

🎉 All test cases on all platform have been passed!

view6view · 2023-04-25T08:47:48Z

tql，大佬

hanxu317317 · 2023-04-25T09:12:29Z

昨天刚发现. 今天就修复~够速度 .

7a3lv7 · 2023-04-26T02:55:10Z

Amazing

Konano · 2023-04-26T06:47:56Z

It seems that everything is done. Can you please review and merge? @alalek.

hcqbuqingzhen · 2023-04-26T07:31:25Z

NIU 大佬

karzeoy · 2023-04-26T08:19:01Z

amazing！

Takeoff0518 · 2023-04-26T08:33:57Z

OrzOrzOrzOrz

Konano · 2023-04-26T08:39:20Z

This pr has too much attention on the internet and has been merged, maybe we should lock it to avoid too many pointless comments. @WanliZhong @zihaomu

Konano and others added 3 commits April 24, 2023 14:04

fix(wechat_qrcode): Initialize nBytes after the count value is determ…

4db1088

…ined

fix(wechat_qrcode): Incorrect count data repair

197bd15

chore: format expr

5781267

fix(wechat_qrcode): Avoid null pointer exception

240689f

opencv-alalek added bug category: wechat_qrcode labels Apr 24, 2023

mochaaP approved these changes Apr 24, 2023

View reviewed changes

aisk approved these changes Apr 24, 2023

View reviewed changes

WanliZhong mentioned this pull request Apr 25, 2023

Fix Wechat QRcode bug #3481

Closed

6 tasks

WanliZhong reviewed Apr 25, 2023

View reviewed changes

fix(wechat_qrcode): return when bytes_ is empty

2ea1d89

test(wechat_qrcode): add test case

2c7ecf3

aisk reviewed Apr 25, 2023

View reviewed changes

modules/wechat_qrcode/src/zxing/qrcode/decoder/decoded_bit_stream_parser.cpp Show resolved Hide resolved

zihaomu approved these changes Apr 25, 2023

View reviewed changes

fengyuentau reviewed Apr 25, 2023

View reviewed changes

zihaomu added the test label Apr 26, 2023

vpisarev self-requested a review April 26, 2023 07:08

vpisarev approved these changes Apr 26, 2023

View reviewed changes

vpisarev merged commit ccc2772 into opencv:4.x Apr 26, 2023

opencv locked as resolved and limited conversation to collaborators Apr 26, 2023

	int nBytes = count;
	BitSource& bits(*bits_);
	// Don't crash trying to read more bits than we have available.
	int available = bits.available();
	// try to repair count data if count data is invalid
	if (count * 8 > available) {
	count = (available + 7 / 8);
	}

	ArrayRef<char> bytes_(count);
	char* readBytes = &(*bytes_)[0];

fix(wechat_qrcode): Init nBytes after the count value is determined #3480

fix(wechat_qrcode): Init nBytes after the count value is determined #3480

Conversation

Konano commented Apr 24, 2023 • edited by zihaomu Loading

Pull Request Readiness Checklist

ssst0n3 commented Apr 24, 2023

k3lpi3b4nsh33 commented Apr 24, 2023

TUGOhost commented Apr 24, 2023

syuy commented Apr 24, 2023

Lakr233 commented Apr 24, 2023

helin0815 commented Apr 24, 2023

KarKLi commented Apr 24, 2023

KongWeibin commented Apr 24, 2023

Timon-2022 commented Apr 24, 2023

Naville commented Apr 24, 2023 • edited Loading

yilongli-tubi commented Apr 24, 2023

yoghurt-lee commented Apr 24, 2023

Nagico commented Apr 24, 2023

DapengFeng commented Apr 24, 2023

sfc9982 commented Apr 24, 2023

UPON-2021 commented Apr 24, 2023

Apple-QAQ commented Apr 24, 2023

xieyumc commented Apr 24, 2023

NoahZhang1 commented Apr 24, 2023

mochaaP left a comment

Choose a reason for hiding this comment

Konano commented Apr 24, 2023

HRan2004 commented Apr 24, 2023

RJHD-GIT commented Apr 24, 2023

JulianSong commented Apr 25, 2023

WanliZhong left a comment • edited Loading

Choose a reason for hiding this comment

Konano commented Apr 25, 2023

WanliZhong commented Apr 25, 2023

GZTimeWalker commented Apr 25, 2023 • edited Loading

WanliZhong commented Apr 25, 2023 • edited Loading

Henryk07 commented Apr 25, 2023

zihaomu left a comment

Choose a reason for hiding this comment

zihaomu commented Apr 25, 2023 • edited Loading

WanliZhong commented Apr 25, 2023 • edited Loading

GZTimeWalker commented Apr 25, 2023

fengyuentau Apr 25, 2023

Choose a reason for hiding this comment

Konano Apr 25, 2023

Choose a reason for hiding this comment

zihaomu Apr 25, 2023

Choose a reason for hiding this comment

Konano commented Apr 25, 2023 • edited Loading

view6view commented Apr 25, 2023

hanxu317317 commented Apr 25, 2023

7a3lv7 commented Apr 26, 2023

Konano commented Apr 26, 2023

hcqbuqingzhen commented Apr 26, 2023

karzeoy commented Apr 26, 2023

Takeoff0518 commented Apr 26, 2023

Konano commented Apr 26, 2023

Konano commented Apr 24, 2023 •

edited by zihaomu

Loading

Naville commented Apr 24, 2023 •

edited

Loading

WanliZhong left a comment •

edited

Loading

GZTimeWalker commented Apr 25, 2023 •

edited

Loading

WanliZhong commented Apr 25, 2023 •

edited

Loading

zihaomu commented Apr 25, 2023 •

edited

Loading

WanliZhong commented Apr 25, 2023 •

edited

Loading

Konano commented Apr 25, 2023 •

edited

Loading