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.

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

Remove strlen call from Curl_client_write. #6954

Closed
wants to merge 2 commits into from
Closed

Conversation

@jsha
Copy link
Contributor

@jsha jsha commented Apr 24, 2021

At all call sites with an explicit 0 len, pass an appropriate nonzero len.

Fixes #6952

Copy link
Member

@bagder bagder left a comment

This is good. Just one little improvement suggestion!

Loading

@@ -616,8 +616,6 @@ CURLcode Curl_client_write(struct Curl_easy *data,
size_t len)
{
struct connectdata *conn = data->conn;
if(0 == len)
len = strlen(ptr);
Copy link
Member

@bagder bagder Apr 25, 2021

Choose a reason for hiding this comment

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

how about also adding...

DEBUGASSERT(len);

... to better catch mistakes at least in debug-builds.

Loading

@jsha jsha force-pushed the client_write_str branch from c8cad78 to 994f59b Apr 25, 2021
@jsha
Copy link
Contributor Author

@jsha jsha commented Apr 25, 2021

Good call on that assert. Looks like the fuzzer caught something on it:

Running: /src/curl_fuzzer/corpora/curl_fuzzer_dict/test_url_dict
curl_fuzzer_dict: sendf.c:620: CURLcode Curl_client_write(struct Curl_easy *, int, char *, size_t): Assertion `len' failed.
==31482== ERROR: libFuzzer: deadly signal
    #0 0x52cba1 in __sanitizer_print_stack_trace /src/llvm-project/compiler-rt/lib/asan/asan_stack.cpp:86:3
    #1 0x4759f8 in fuzzer::PrintStackTrace() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerUtil.cpp:210:5
    #2 0x4590f3 in fuzzer::Fuzzer::CrashCallback() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:3
    #3 0x7fe8731d438f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1138f)
    #4 0x7fe872503437 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x35437)
    #5 0x7fe872505039 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x37039)
    #6 0x7fe8724fbbe6  (/lib/x86_64-linux-gnu/libc.so.6+0x2dbe6)
    #7 0x7fe8724fbc91 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x2dc91)
    #8 0x59ad2a in Curl_client_write /src/curl/lib/sendf.c:620:3
    #9 0x5bfc9d in readwrite_data /src/curl/lib/transfer.c:839:26
    #10 0x5bd5ab in Curl_readwrite /src/curl/lib/transfer.c:1219:14
    #11 0x58bf01 in multi_runsingle /src/curl/lib/multi.c:2254:16
    #12 0x5896be in curl_multi_perform /src/curl/lib/multi.c:2517:14
    #13 0x5585a7 in fuzz_handle_transfer(fuzz_data*) /src/curl_fuzzer/curl_fuzzer.cc:305:3
    #14 0x557c17 in LLVMFuzzerTestOneInput /src/curl_fuzzer/curl_fuzzer.cc:93:3
    #15 0x45a893 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
    #16 0x4446a2 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6
    #17 0x44a4ea in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9
    #18 0x4761d2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #19 0x7fe8724ee83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    #20 0x41f388 in _start (/src/curl_fuzzer/curl_fuzzer_dict+0x41f388)

Loading

@bagder
Copy link
Member

@bagder bagder commented Apr 25, 2021

I think it highlights a genuine bug where the code passes on a zero but didn't intend a strlen(), and I think I propose this fix:

diff --git a/lib/transfer.c b/lib/transfer.c
index 56ad5e612..c31e22e00 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -828,21 +828,21 @@ static CURLcode readwrite_data(struct Curl_easy *data,
              error here, be sure to check over the almost identical code
              in http_chunks.c.
              Make sure that ALL_CONTENT_ENCODINGS contains all the
              encodings handled here. */
           if(data->set.http_ce_skip || !k->writer_stack) {
-            if(!k->ignorebody) {
+            if(!k->ignorebody && nread) {
 #ifndef CURL_DISABLE_POP3
               if(conn->handler->protocol & PROTO_FAMILY_POP3)
                 result = Curl_pop3_write(data, k->str, nread);
               else
 #endif /* CURL_DISABLE_POP3 */
                 result = Curl_client_write(data, CLIENTWRITE_BODY, k->str,
                                            nread);
             }
           }
-          else if(!k->ignorebody)
+          else if(!k->ignorebody && nread)
             result = Curl_unencode_write(data, k->writer_stack, k->str, nread);
         }
         k->badheader = HEADER_NORMAL; /* taken care of now */
 
         if(result)

Loading

@jsha
Copy link
Contributor Author

@jsha jsha commented Apr 25, 2021

Your proposed fix looks good to me. Thanks for spotting that. I've been trying to get the curl-fuzzer repo running, but hitting errors with clang: error: no such file or directory: 'libstandaloneengine.a'.

Loading

@bagder
Copy link
Member

@bagder bagder commented Apr 27, 2021

Feel free to add that patch as a commit here so that we can see if there seems to be any remaining flaws to fix.

Loading

@jsha
Copy link
Contributor Author

@jsha jsha commented Apr 28, 2021

Another fuzz failure, this time in Curl_pop3_write:

2021-04-27 23:15:32,339 - root - INFO - Running command: docker run --rm --privileged --volumes-from f184069eb5c9 -e OUT=/github/workspace/out -e FUZZING_ENGINE=libfuzzer -e SANITIZER=address -e CIFUZZ=True -e RUN_FUZZER_MODE=interactive gcr.io/oss-fuzz-base/base-runner bash -c run_fuzzer curl_fuzzer -seed=1337 -len_control=0 -max_total_time=141 /github/workspace/out/cifuzz-corpus/curl_fuzzer
2021-04-27 23:15:54,198 - root - INFO - Fuzzer curl_fuzzer, ended before timeout.
Fuzzer: curl_fuzzer. Detected bug:
/usr/local/bin/run_fuzzer: line 148: [: -max_len=10000: binary operator expected
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1337
INFO: Loaded 1 modules   (67412 inline 8-bit counters): 67412 [0x1037450, 0x1047ba4), 
INFO: Loaded 1 PC tables (67412 PCs): 67412 [0x1047ba8,0x114f0e8), 
INFO:     8987 files found in /github/workspace/out/cifuzz-corpus/curl_fuzzer
INFO:     4203 files found in /tmp/curl_fuzzer_corpus
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 24926 bytes
INFO: seed corpus: files: 13190 min: 1b max: 24926b total: 8957978b rss: 58Mb
#1024	pulse  cov: 5164 ft: 7661 corp: 427/5619b exec/s: 256 rss: 82Mb
#2048	pulse  cov: 7190 ft: 12280 corp: 930/18Kb exec/s: 136 rss: 84Mb
curl_fuzzer: sendf.c:620: CURLcode Curl_client_write(struct Curl_easy *, int, char *, size_t): Assertion `len' failed.
AddressSanitizer:DEADLYSIGNAL
=================================================================
==18==ERROR: AddressSanitizer: ABRT on unknown address 0x000000000012 (pc 0x7f1eff373438 bp 0x000000b52240 sp 0x7ffc41fec1d8 T0)
SCARINESS: 10 (signal)
    #0 0x7f1eff373438 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x35438)
    #1 0x7f1eff375039 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x37039)
    #2 0x7f1eff36bbe6  (/lib/x86_64-linux-gnu/libc.so.6+0x2dbe6)
    #3 0x7f1eff36bc91 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x2dc91)
    #4 0x59adaa in Curl_client_write /src/curl/lib/sendf.c:620:3
    #5 0x659ae0 in Curl_pop3_write /src/curl/lib/pop3.c:1518:18
    #6 0x65bfdc in pop3_state_command_resp /src/curl/lib/pop3.c:929:18
    #7 0x65a0c0 in pop3_statemachine /src/curl/lib/pop3.c:1009:16
    #8 0x6570ab in Curl_pp_statemach /src/curl/lib/pingpong.c:134:14
    #9 0x65925d in pop3_multi_statemach /src/curl/lib/pop3.c:1038:12
    #10 0x65d3e8 in pop3_perform /src/curl/lib/pop3.c:1188:12
    #11 0x65d212 in pop3_regular_transfer /src/curl/lib/pop3.c:1310:12
    #12 0x658dde in pop3_do /src/curl/lib/pop3.c:1221:12
    #13 0x592d11 in multi_do /src/curl/lib/multi.c:1512:14
    #14 0x58ab42 in multi_runsingle /src/curl/lib/multi.c:2004:18
    #15 0x58973e in curl_multi_perform /src/curl/lib/multi.c:2517:14
    #16 0x5585a7 in fuzz_handle_transfer(fuzz_data*) /src/curl_fuzzer/curl_fuzzer.cc:305:3
    #17 0x557c17 in LLVMFuzzerTestOneInput /src/curl_fuzzer/curl_fuzzer.cc:93:3
    #18 0x45a893 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
    #19 0x459d9a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:505:3
    #20 0x45c184 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:814:7
    #21 0x45c399 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:845:3
    #22 0x44a3c7 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:906:6
    #23 0x4761d2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #24 0x7f1eff35e83f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2083f)
    #25 0x41f388 in _start (out/curl_fuzzer+0x41f388)

DEDUP_TOKEN: raise--abort--
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: ABRT (/lib/x86_64-linux-gnu/libc.so.6+0x35438) in raise
==18==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x0,0x1,0x0,0x0,0x0,0xa,0x70,0x6f,0x70,0x33,0x3a,0x2f,0x21,0x2d,0x45,0x52,0x0,0x2,0x0,0x0,0x0,0x16,0x2b,0x4f,0x4b,0xa,0x2e,0x7e,0xa,0x2b,0x4f,0x4b,0xa,0x2e,0x2e,0xd,0x8a,0xd,0x23,0xd,0xa,0x2c,0xd,0x0,
\x00\x01\x00\x00\x00\x0apop3:/!-ER\x00\x02\x00\x00\x00\x16+OK\x0a.~\x0a+OK\x0a..\x0d\x8a\x0d#\x0d\x0a,\x0d\x00
artifact_prefix='./'; Test unit written to ./crash-1c262174f38d966a2fa6f98e314ae93a21ff32af
Base64: AAEAAAAKcG9wMzovIS1FUgACAAAAFitPSwoufgorT0sKLi4Nig0jDQosDQA=

Loading

jsha added 2 commits Apr 28, 2021
At all call sites with an explicit 0 len, pass an appropriate nonzero
len.
@jsha jsha force-pushed the client_write_str branch from 42b301a to e9da118 Apr 28, 2021
@bagder
Copy link
Member

@bagder bagder commented Apr 29, 2021

Thanks!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants