remote null pointer dereference trigger in admin handler

[mmmds]

It's possible to trigger NULL pointer dereference in case if request uses POST method with empty body.

cherokee/handler_admin.c

[...]
204  ret_t
205  cherokee_handler_admin_read_post (cherokee_handler_admin_t *hdl)
206  {
207     int                      re;
208     ret_t                    ret;
209     char                    *tmp;
210     cherokee_buffer_t        post = CHEROKEE_BUF_INIT;
211     cherokee_buffer_t        line = CHEROKEE_BUF_INIT;
212     cherokee_connection_t   *conn = HANDLER_CONN(hdl);
213  
214     /* Check for the post info
215      */
216     if (! conn->post.has_info) {
217         conn->error_code = http_bad_request;
218         return ret_error;
219     }
220  
221     /* Process line per line
222      */
223     ret = cherokee_post_read (&conn->post, &conn->socket, &post);
224     switch (ret) {
225     case ret_ok:
226     case ret_eagain:
227         break;
228     default:
229         conn->error_code = http_bad_request;
230         return ret_error;
231     }
232  
233     /* Parse
234      */
235     TRACE (ENTRIES, "Post contains: '%s'\n", post.buf);
236  
237     cherokee_dwriter_list_open (&hdl->dwriter);
238  
239     for (tmp = post.buf;;) {
240         char *end1 = strchr (tmp, CHR_LF);
[...]

If post body is empty then post.buf is NULL and strchr on tmp results in NULL pointer dereference.

Proof of concept:

curl -d "" http://127.0.0.1:8765/test15/

test15 is the admin handler.

### ASAN report:

=================================================================
==494==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f221a131ad3 bp 0x7f2213bf4be0 sp 0x7f2213bf4368 T8)
    #0 0x7f221a131ad2  (/lib/x86_64-linux-gnu/libc.so.6+0x89ad2)
    #1 0x7f221b898236 in strchr (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x71236)
    #2 0x5098f2 in cherokee_handler_admin_read_post /home/shm/src/webserver/cherokee/handler_admin.c:240
    #3 0x546dfe in cherokee_handler_read_post /home/shm/src/webserver/cherokee/handler.c:107
    #4 0x53770c in cherokee_connection_read_post /home/shm/src/webserver/cherokee/connection.c:684
    #5 0x489273 in process_active_connections /home/shm/src/webserver/cherokee/thread.c:1235
    #6 0x48df03 in cherokee_thread_step_MULTI_THREAD /home/shm/src/webserver/cherokee/thread.c:2086
    #7 0x48239f in thread_routine /home/shm/src/webserver/cherokee/thread.c:99
    #8 0x7f221ad316b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #9 0x7f221a1af41c in clone (/lib/x86_64-linux-gnu/libc.so.6+0x10741c)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 ??
Thread T8 created by T0 here:
    #0 0x7f221b85d253 in pthread_create (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x36253)
    #1 0x483292 in cherokee_thread_new /home/shm/src/webserver/cherokee/thread.c:247
    #2 0x46ded5 in initialize_server_threads /home/shm/src/webserver/cherokee/server.c:671
    #3 0x4700f2 in cherokee_server_initialize /home/shm/src/webserver/cherokee/server.c:1053
    #4 0x417e26 in common_server_initialization /home/shm/src/webserver/cherokee/main_worker.c:255
    #5 0x41881d in main /home/shm/src/webserver/cherokee/main_worker.c:393
    #6 0x7f221a0c882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

==494==ABORTING

Setup:

• Ubuntu 18.04 64 bit
• source code from github, commit 9a75e65b876bcc376cb6b379dca1f7ce4a055c59
• build command:

  ac_cv_func_realloc_0_nonnull=yes ac_cv_func_malloc_0_nonnull=yes LDFLAGS="-lasan" LDADD="-lasan" CFLAGS="-fsanitize=address -ggdb -O0 -fprofile-arcs -ftest-coverage" ./configure --prefix=`pwd`/bin --enable-trace --enable-static-module=all --enable-static --enable-shared=no
  make

• files in webroot mkdir /var/www/test{1..20}; for i inseq 1 20; do echo test > test$i/test.html; done
• configuration file cherokee.txt

found by: Mateusz Kocielski, Michał Dardas from LogicalTrust

[skinkie]

Thanks for this very valuable insight. While this only happens when the admin is running, it is obviously bad a practice. When evaluating the code my initial assumption would be that "has_info" should already cover for this, but that only takes care of the headers.

[skinkie]

@mmmds could you validate my pull request?

[mmmds]

We've checked it. It's ok.

[skinkie]

Thanks it is merged.