Vulnerability report #55

jeaye · 2017-08-05T01:52:06Z

Hey there, I'm actively interested in inspecting open source C projects for vulnerabilities and bugs. Unfortunately, fiche has some issues. I reported this privately to you three weeks ago; since then, you have opened #54 asking for a new maintainer.

Anyone looking to install fiche on a public server should beware.

Summary

2 use after free bugs
1 stack overflow bug
1 format string truncation
1 incorrect argument order for declaration/definition
2 uses of uninitialized variables (benign, in this case)
3 unchecked malloc calls
1 unchecked fopen call
Various warnings

Static analysis

Diff applied to Makefile

diff --git a/Makefile b/Makefile
index b75fc63..dfc04b1 100644
--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,7 @@
 # solusipse.net
 # -----------------------------------
 
-CFLAGS+=-pthread -O2
+CFLAGS+=-pthread -O2 -Wall -Wextra -pedantic
 prefix=/usr/local
 
 all: fiche

Clang's static analysis

$ scan-build make -B
scan-build: Using '/usr/bin/clang-4.0' for static analysis
/usr/bin/../lib/clang/ccc-analyzer -pthread -O2 -Wall -Wextra -pedantic    fiche.c   -o fiche
fiche.c: In function ‘load_list’:
fiche.c:340:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (fread(buffer, fsize, 1, fp) != fsize)
                                     ^~
fiche.c: In function ‘parse_parameters’:
fiche.c:537:54: warning: ‘snprintf’ output truncated before the last format character [-Wformat-truncation=]
                 snprintf(symbols, sizeof symbols, "%s", "abcdefghijklmnopqrstuvwxyz0123456789-+_=.ABCDEFGHIJKLMNOPQRSTUVWXYZ");
                                                      ^
fiche.c:537:17: note: ‘snprintf’ output 68 bytes into a destination of size 67
                 snprintf(symbols, sizeof symbols, "%s", "abcdefghijklmnopqrstuvwxyz0123456789-+_=.ABCDEFGHIJKLMNOPQRSTUVWXYZ");
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fiche.c:319:12: warning: Use of memory after it is freed
    return strstr(BANLIST, ip_address);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
fiche.c:325:12: warning: Use of memory after it is freed
    return strstr(WHITELIST, ip_address);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
scan-build: 2 bugs found.
scan-build: Run 'scan-view /tmp/scan-build-2017-07-12-181831-20724-1' to examine bug reports.

Cppcheck's analysis

$ cppcheck --enable=all --inconclusive fiche.c 
Checking fiche.c ...
[fiche.h:112] -> [fiche.c:370]: (style, inconclusive) Function 'set_address' argument 1 names different: declaration 'serveraddr' definition 'server_address'.
[fiche.h:114] -> [fiche.c:380]: (style, inconclusive) Function 'set_address6' argument 1 names different: declaration 'serveraddr6' definition 'server_address6'.
[fiche.h:91] -> [fiche.c:390]: (style, inconclusive) Function 'bind_to_port' argument 2 names different: declaration 'serveraddr' definition 'server_address'.
[fiche.h:93] -> [fiche.c:399]: (style, inconclusive) Function 'bind_to_port6' argument 2 names different: declaration 'serveraddr6' definition 'server_address6'.
[fiche.h:98] -> [fiche.c:450]: (warning) Function 'save_to_file' argument order different: declaration 'buffer, slug, data' definition 'slug, buffer, data'
[fiche.c:220]: (style) Obsolete function 'asctime' called. It is recommended to use 'strftime' instead.
[fiche.c:74]: (error) Uninitialized variable: server_address
[fiche.c:66]: (error) Uninitialized variable: server_address6
Checking fiche.c: BSD...
Checking fiche.c: HAVE_INET6...
(information) Cppcheck cannot find all the include files (use --check-config for details)

Dynamic analysis

Diff applied to Makefile

diff --git a/Makefile b/Makefile
index b75fc63..323f77a 100644
--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,7 @@
 # solusipse.net
 # -----------------------------------
 
-CFLAGS+=-pthread -O2
+CFLAGS+=-pthread -ggdb -Wall -Wextra -pedantic -fsanitize=address -fno-omit-frame-pointer
 prefix=/usr/local
 
 all: fiche

Clang's address sanitizer

This targets a case where the output dir (basedir) is not writeable, or doesn't exist. In that case, this loop will run indefinitely, smashing the stack with each iteration's new random byte. If you limit this loop to be within the slug's size, you'll crash on the unchecked fopen in save_to_file.

$ ./fiche -d "http://localhost/" -o "/" -p 9999 -s 4 -B 8192 
====================================
Domain name: http://http://localhost//
Saving files to: /
Fiche started listening on port 9999.
Buffer size set to: 8192.
Slug size set to: 4.
Log file: (null)
====================================
=================================================================
==31816==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7f2ef66fccec at pc 0x00000047353c bp 0x7f2ef66fcb40 sp 0x7f2ef66fc2f0
READ of size 15 at 0x7f2ef66fccec thread T1
    #0 0x47353b in printf_common(void*, char const*, __va_list_tag*) (/home/jeaye/projects/reversing/fiche/fiche+0x47353b)
    #1 0x47411b in __interceptor_vprintf (/home/jeaye/projects/reversing/fiche/fiche+0x47411b)
    #2 0x4741ee in printf (/home/jeaye/projects/reversing/fiche/fiche+0x4741ee)
    #3 0x50a620 in generate_url /home/jeaye/projects/reversing/fiche/fiche.c:431:9
    #4 0x509d5d in thread_connection /home/jeaye/projects/reversing/fiche/fiche.c:145:9
    #5 0x4dc512 in __asan::AsanThread::ThreadStart(unsigned long, __sanitizer::atomic_uintptr_t*) (/home/jeaye/projects/reversing/fiche/fiche+0x4dc512)
    #6 0x7f2ef9e3e296 in start_thread (/usr/lib/libpthread.so.0+0x7296)
    #7 0x7f2ef924a1ee in __GI___clone (/usr/lib/libc.so.6+0xed1ee)

Address 0x7f2ef66fccec is located in stack of thread T1
SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow (/home/jeaye/projects/reversing/fiche/fiche+0x47353b) in printf_common(void*, char const*, __va_list_tag*)
Shadow bytes around the buggy address:
  0x0fe65ecd7940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd7950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd7960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd7970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd7980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe65ecd7990: 00 00 00 00 00 00 00 00 ca ca ca ca 00[04]cb cb
  0x0fe65ecd79a0: cb cb cb cb ca ca ca ca 00 00 00 00 00 00 00 00
  0x0fe65ecd79b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd79c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd79d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe65ecd79e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
Thread T1 created by T0 here:
    #0 0x431f50 in __interceptor_pthread_create (/home/jeaye/projects/reversing/fiche/fiche+0x431f50)
    #1 0x5096d9 in perform_connection /home/jeaye/projects/reversing/fiche/fiche.c:206:9
    #2 0x508d46 in main /home/jeaye/projects/reversing/fiche/fiche.c:89:19
    #3 0x7f2ef917d439 in __libc_start_main (/usr/lib/libc.so.6+0x20439)

==31816==ABORTING

Recommendation

I highly recommend that you don't write new software (especially web services) in C and there are various areas of fiche's code which are just waiting for a subtle change to allow a buffer overflow exploit, which could result in remote code execution. You have a ticket open for porting fiche to Rust and I think that's a superb idea.

In the meantime, please consider addressing these issues, adding those warning flags to your build, and running both static and dynamic analysis as part of your continuous integration.

The text was updated successfully, but these errors were encountered:

dsalychev · 2017-08-27T18:54:01Z

Generally, it's a good idea to point to the problems in the project. However, your "recommendations" ruined almost everything. Where is your patch? It's not fair to spoil this project this ugly way. You'd better to keep private what should be private.

jeaye · 2017-08-27T19:11:41Z

I'd rather this project be euthanized than patched. The project is dead, looking for a new maintainer, and entirely unsafe in its implementation. If I were to send any PR, it'd be to add this warning to the README.

As mentioned in the OP, this was handled privately, via encrypted email, and remained unaddressed for three weeks before I posted this publicly. Now, at least, people can know what they're getting into and the new maintainer has a list of items to tackle first.

Seriously though, my recommendation stands. People should stop using fiche on their public servers until it's been entirely reworked into something safer. To prevent issues like this in other projects, C is clearly not the ideal tool for the job.

solusipse · 2017-09-02T18:31:48Z

Hello @jeaye. Thank you for your work and sorry for the delayed response. You inspired me to rewrite the whole program and I thought I'll respond right after doing that. Unfortunatelly, as it often happens, I had to abandon project for a while. I've finally found some time this weekend and I completed basic functionality. Let me address your concerns.

I of course agree with all you said, fiche had some serious problems. I wrote it as my first serious C project, never actively maintained it, and accepted too many page requests without giving them a proper consideration.

I committed many sins and I plead guilty! And what's even worse, even I never trusted it and kept it in fully-separated environment on my server (btw, that's still recommended and we have to redone support for containers!).

But now I'm here to change myself! So, if that's not a problem, please take a look at the new code, and help me to correct it! Sorry for not listening to your advice regarding not-using C.
I agree that Rust would be a safer solution, and there's already a nice project that is a clone of fiche written in Rust. I'm going to place a link to it in our Readme file. And I'm also going to keep fiche as a C project for now.
Maybe just to prove that team effort can make even C safe? Hope we will achieve that!

Last thing, regarding issue #54, I opened it independly, right before your email. You sent me a message that I was able to decipher on 14.07 and discussed issue was created on 13.07. Its creation wasn't caused by your notice, once again sorry for the lack of response.

Below you can see my report created with the new version:

scan-build

scan-build make -B

Empty output (no errors detected):

analyze-cc main.c fiche.c -pthread -O2 -Wall -Wextra -Wpedantic -Wstrict-overflow -fno-strict-aliasing -std=gnu11 -g -O0 -o fiche
scan-build: Removing directory '/tmp/scan-build-2017-09-02-19-31-24-790359-Csy0W8' because it contains no report.

cppcheck

cppcheck --enable=all --inconclusivefiche.c fiche.h main.c

Nothing to worry about here (information message is caused by cppcheck's lack support for dealing with standard headers):

Checking fiche.c ...
1/3 files checked 77% done
Checking fiche.h ...
2/3 files checked 86% done
Checking main.c ...
3/3 files checked 100% done
(information) Cppcheck cannot find all the include files (use --check-config for details)

valgrind

Diff:

diff --git a/fiche.c b/fiche.c
index f005816..62d4a75 100644
--- a/fiche.c
+++ b/fiche.c
@@ -455,8 +455,10 @@ static int start_server(Fiche_Settings *settings) {
     print_separator();
 
     // Run dispatching loop
-    while (1) {
+    int i = 0;
+    while (i < 5) {
         dispatch_connection(s, settings);
+        i++;
     }
 
     // Give some time for all threads to finish

Output:

valgrind --leak-check=full --show-leak-kinds=all --tool=memcheck ./fiche                                                                               
==17185== Memcheck, a memory error detector
==17185== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==17185== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==17185== Command: ./fiche
==17185== 
[Fiche][STATUS] Starting fiche on Sat Sep  2 19:55:14 2017...
[Fiche][STATUS] Domain set to: http://example.com.
[Fiche][STATUS] Server started listening on port: 9999.
============================================================
[Fiche][STATUS] Sat Sep  2 19:55:16 2017
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost).
[Fiche][STATUS] Received 36 bytes, saved to: rte5.
============================================================
[Fiche][STATUS] Sat Sep  2 19:55:16 2017
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost).
[Fiche][STATUS] Received 36 bytes, saved to: tnmz.
============================================================
[Fiche][STATUS] Sat Sep  2 19:55:17 2017
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost).
[Fiche][STATUS] Received 36 bytes, saved to: 894i.
============================================================
[Fiche][STATUS] Sat Sep  2 19:55:18 2017
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost).
[Fiche][STATUS] Received 36 bytes, saved to: rwuk.
============================================================
[Fiche][STATUS] Sat Sep  2 19:55:18 2017
[Fiche][STATUS] Incoming connection from: 127.0.0.1 (localhost).
[Fiche][STATUS] Received 36 bytes, saved to: 0ybq.
============================================================
==17185== 
==17185== HEAP SUMMARY:
==17185==     in use at exit: 0 bytes in 0 blocks
==17185==   total heap usage: 121 allocs, 121 frees, 73,857 bytes allocated
==17185== 
==17185== All heap blocks were freed -- no leaks are possible
==17185== 
==17185== For counts of detected and suppressed errors, rerun with: -v
==17185== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Testing sequence used:

 $ date | md5sum | nc localhost 9999
http://example.com/rte5
 $ date | md5sum | nc localhost 9999
http://example.com/tnmz
 $ date | md5sum | nc localhost 9999
http://example.com/894i
 $ date | md5sum | nc localhost 9999
http://example.com/rwuk
 $ date | md5sum | nc localhost 9999
http://example.com/0ybq

jeaye · 2017-09-04T20:08:16Z

Thanks for the detailed response, @solusipse. I'm really glad you've taken the time to rework fiche into something safer. Are you open to setting up TravisCI, which you're already using for builds, to run this static and dynamic analysis continuously? This would allow Github to automatically test each PR made by others and each push made by you to ensure it doesn't reintroduce these sorts of issues.

Since you've closed #54, are we to assume that you're going to actively maintain fiche for the time being? Furthermore, is the rewrite to be considered stable?

Regarding the choice for C and working together to help the source be safer: the time that it would take all of us to have reasonable confidence in the safety of the code, and empirical tests to help substantiate it, fiche could be written in Python or Ruby or Clojure multiple times over.

The simple fact that you need to worry about unchecked mallocs, undefined behavior with unchecked return values (returns NULL on error, sets errorno, leaving buf undefined), duplicate cleanup code (here, here, here, here, and here), and the usage of uninitialized variables, just to name a few in the new implementation, really shows that you're not just writing a paste service. Instead, you're spending most of your time managing trivial resources and dancing with undefined behavior. The actual paste service code doesn't take 800+ lines (main.c and fiche.c).

Hopefully you can see how scary it should be for anyone to run services like this on a public-facing machine.

solusipse · 2017-09-05T23:59:26Z

@jeaye Thanks!

Are you open to setting up TravisCI, which you're already using for builds, to run this static and dynamic analysis continuously?

Great idea! I did that for static analysis, going to write tests and integrate in also the dynamic analysis.

Since you've closed #54, are we to assume that you're going to actively maintain fiche for the time being? Furthermore, is the rewrite to be considered stable?

That is correct.

Regarding the choice for C and working together to help the source be safer: the time that it would take all of us to have reasonable confidence in the safety of the code, and empirical tests to help substantiate it, fiche could be written in Python or Ruby or Clojure multiple times over.

Well, you're probably right. I'd love to see clones of fiche written in these languages! As for C, I believe that while it is still around (maybe that's unfortunate?) we should have an opportunity to write also new software with that language.

Moreover, with your great help we are going to eliminate all potential issues soon and we won't have to worry so much. Speaking of which:

unchecked mallocs

Fixed!

undefined behavior with unchecked return values

Fixed! Thank you!

As for

duplicate cleanup code

I don't see why this should be considered a problem when tested properly. Sure, these are places where something can go wrong easily, but fortunately we don't have many of them, so we're able to check them carefully.

As for uninitialised variable you've highlighted, once again I don't see why it's a security problem. As you can see, the first operation on that variable is allocation performed here. Sure, it might be considered a bad style, but that's rather subjective.

Please let me know if I'm correct regarding these two.

As for

The actual paste service code doesn't take 800+ lines

isn't that platform/language/implementation dependant?

As for

Hopefully you can see how scary it should be for anyone to run services like this on a public-facing machine.

Sure! But still, there are things that scare me more! Maybe I'm insane, but I'm willing to take the risk. As for others, we can add huge warning in our Readme saying something about possible terrible implications of using that software. Not kidding, I'm willing to do that if you think that's the right thing! I was also thinking about recommending a clone written in Rust - hope you'll find information soothing! 😘

jeaye · 2017-09-17T17:17:54Z

Thanks, @solusipse, for fixing up those further issues. I think we see things very differently and I don't want to take up any more of your time, so I'll close this ticket.

The rewrite, continuous analysis, and revitalized development is great to see. Cheers.

jeaye closed this as completed Sep 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vulnerability report #55

Vulnerability report #55

jeaye commented Aug 5, 2017

dsalychev commented Aug 27, 2017

jeaye commented Aug 27, 2017

solusipse commented Sep 2, 2017

jeaye commented Sep 4, 2017

solusipse commented Sep 5, 2017

jeaye commented Sep 17, 2017

Vulnerability report #55

Vulnerability report #55

Comments

jeaye commented Aug 5, 2017

Summary

Static analysis

Diff applied to Makefile

Clang's static analysis

Cppcheck's analysis

Dynamic analysis

Diff applied to Makefile

Clang's address sanitizer

Recommendation

dsalychev commented Aug 27, 2017

jeaye commented Aug 27, 2017

solusipse commented Sep 2, 2017

scan-build

cppcheck

valgrind

jeaye commented Sep 4, 2017

solusipse commented Sep 5, 2017

jeaye commented Sep 17, 2017