Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix heap buffer overead in ConfigParser::UnQuote() #1763

Closed
wants to merge 1 commit into from
Closed

Fix heap buffer overead in ConfigParser::UnQuote() #1763

wants to merge 1 commit into from

Conversation

xiaoxiaoafeifei
Copy link
Contributor

@xiaoxiaoafeifei xiaoxiaoafeifei commented Mar 29, 2024 •
edited by rousskov
Loading

Detected by using AddressSanitizer.

@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Mar 29, 2024
rousskov
rousskov requested changes Mar 29, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this bug!

src/ConfigParser.cc Outdated Show resolved Hide resolved
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 29, 2024
@rousskov rousskov self-requested a review April 1, 2024 15:02
@rousskov rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Apr 1, 2024
@rousskov rousskov changed the title Fix heap buffer overflow in function ConfigParser::UnQuote Fix heap buffer overead in ConfigParser::UnQuote() Apr 1, 2024
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Apr 1, 2024
rousskov
rousskov previously approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adjusting this fix! I trust your refactoring still addresses the problem you could reproduce.

I have adjusted PR title and description (i.e. future official commit message) to meet Squid Project formatting requirements. In the future, please post all those (very useful!) testing details as a PR comment while keeping PR description as the commit message body (which we can then edit to add any details as needed, of course). Thank you.

Our CI formatting tests fail because your name is not in CONTRIBUTORS file. Please add your contact info to that file in this PR. The tools detect two variations of your credentials based on info provided by git/GitHib. Use the variation you prefer.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Apr 1, 2024
@xiaoxiaoafeifei
Copy link
Contributor Author

xiaoxiaoafeifei commented Apr 2, 2024 •
edited
Loading

Reproduce:
export CFLAGS="-g -O0 -fsanitize=address,undefined" CXXFLAGS="-g -O0 -fsanitize=address,undefined"
export CC=afl-clang-fast CXX=afl-clang-fast++
./configure
make && make install
/usr/local/squid/sbin/squid -f poc_file
poc_file:
poc_file.zip

Evidence:
==81496==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000000baf at pc 0x55d9017b242f bp 0x7ffc8a164e30 sp 0x7ffc8a164e28
READ of size 1 at 0x60b000000baf thread T0
#0 0x55d9017b242e in ConfigParser::UnQuote(char const*, char const**) /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:184:9
#1 0x55d9017b4acb in ConfigParser::TokenParse(char const*&, ConfigParser::TokenType&) /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:234:31
#2 0x55d9017b0aab in ConfigParser::NextElement(ConfigParser::TokenType&) /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:321:19
#3 0x55d9017aed45 in ConfigParser::NextToken() /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:350:21
#4 0x55d9017bd856 in ConfigParser::NextQuotedToken() /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:528:19
#5 0x55d901d752b8 in parse_wordlist(wordlist**) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:3156:21
#6 0x55d9023d4d53 in parse_externalAclHelper(external_acl**) /root/fuzz/fuzz_squid/squid/src/external_acl.cc:364:5
#7 0x55d901dfb36b in parse_line(char*) /root/fuzz/fuzz_squid/squid/src/../src/cf_parser.cci:1098:9
#8 0x55d901d936c4 in parseOneConfigFile(char const*, unsigned int) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:564:26
#9 0x55d901d6cb6d in parseConfigFileOrThrow(char const*) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:612:17
#10 0x55d901d6b863 in parseConfigFile(char const*) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:640:16
#11 0x55d902741c1b in SquidMain(int, char**) /root/fuzz/fuzz_squid/squid/src/main.cc:1597:25
#12 0x55d90273fba2 in SquidMainSafe(int, char**) /root/fuzz/fuzz_squid/squid/src/main.cc:1353:16
#13 0x55d90273fb49 in main /root/fuzz/fuzz_squid/squid/src/main.cc:1341:12
#14 0x7f0d0a305d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
#15 0x7f0d0a305e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
#16 0x55d901398154 in _start (/usr/local/squid/sbin/squid+0x1352154) (BuildId: 18d358e29b9a385368bb9f266d0f672923deef52)

0x60b000000baf is located 0 bytes after 111-byte region [0x60b000000b40,0x60b000000baf)
allocated by thread T0 here:
#0 0x55d9014323a5 in __interceptor_realloc (/usr/local/squid/sbin/squid+0x13ec3a5) (BuildId: 18d358e29b9a385368bb9f266d0f672923deef52)
#1 0x55d9049812fc in xrealloc /root/fuzz/fuzz_squid/squid/compat/xalloc.cc:131:14
#2 0x55d901d901a7 in parseOneConfigFile(char const*, unsigned int) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:530:27
#3 0x55d901d6cb6d in parseConfigFileOrThrow(char const*) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:612:17
#4 0x55d901d6b863 in parseConfigFile(char const*) /root/fuzz/fuzz_squid/squid/src/cache_cf.cc:640:16
#5 0x55d902741c1b in SquidMain(int, char**) /root/fuzz/fuzz_squid/squid/src/main.cc:1597:25
#6 0x55d90273fba2 in SquidMainSafe(int, char**) /root/fuzz/fuzz_squid/squid/src/main.cc:1353:16
#7 0x55d90273fb49 in main /root/fuzz/fuzz_squid/squid/src/main.cc:1341:12
#8 0x7f0d0a305d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)

SUMMARY: AddressSanitizer: heap-buffer-overflow /root/fuzz/fuzz_squid/squid/src/ConfigParser.cc:184:9 in ConfigParser::UnQuote(char const*, char const**)
Shadow bytes around the buggy address:
0x60b000000900: fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd fd
0x60b000000980: fd fd fd fa fa fa fa fa fa fa fa fa fd fd fd fd
0x60b000000a00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
0x60b000000a80: fa fa fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x60b000000b00: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x60b000000b80: 00 00 00 00 00[07]fa fa fa fa fa fa fa fa fa fa
0x60b000000c00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x60b000000c80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x60b000000d00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x60b000000d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x60b000000e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==81496==ABORTING

@xiaoxiaoafeifei
Copy link
Contributor Author

xiaoxiaoafeifei commented Apr 2, 2024 •
edited
Loading

Thank you for adjusting this fix! I trust your refactoring still addresses the problem you could reproduce.

I have adjusted PR title and description (i.e. future official commit message) to meet Squid Project formatting requirements. In the future, please post all those (very useful!) testing details as a PR comment while keeping PR description as the commit message body (which we can then edit to add any details as needed, of course). Thank you.

Our CI formatting tests fail because your name is not in CONTRIBUTORS file. Please add your contact info to that file in this PR. The tools detect two variations of your credentials based on info provided by git/GitHib. Use the variation you prefer.

Thanks for you reply! @rousskov
I have post all those testing details as a PR comment and added my contact to CONTRIBUTORS file

@rousskov rousskov removed the S-waiting-for-author author action is expected (and usually required) label Apr 2, 2024
@kinkie kinkie added backport-to-v6 maintainer has approved these changes for v6 backporting M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) and removed backport-to-v6 maintainer has approved these changes for v6 backporting labels Apr 2, 2024
@rousskov
Copy link
Contributor

rousskov commented Apr 2, 2024

OK to test

@rousskov rousskov removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Apr 2, 2024
squid-anubis pushed a commit that referenced this pull request Apr 2, 2024
@xiaoxiaoafeifei @squid-anubis
Fix heap buffer overead in ConfigParser::UnQuote() (#1763)
1891ce5 
Detected by using AddressSanitizer.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Apr 2, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 2, 2024
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 9, 2024
@xiaoxiaoafeifei @kinkie
Fix heap buffer overead in ConfigParser::UnQuote() (squid-cache#1763)
5d18dc5 
Detected by using AddressSanitizer.
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 9, 2024
@xiaoxiaoafeifei @kinkie
Fix heap buffer overead in ConfigParser::UnQuote() (squid-cache#1763)
5f2ee67 
Detected by using AddressSanitizer.
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 14, 2024
@xiaoxiaoafeifei @kinkie
Fix heap buffer overead in ConfigParser::UnQuote() (squid-cache#1763)
d1d6498 
Detected by using AddressSanitizer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousskov rousskov rousskov approved these changes

@kinkie kinkie kinkie approved these changes

Assignees
No one assigned
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xiaoxiaoafeifei @squid-prbot @rousskov @kinkie @squid-anubis