jvinet / knock

A port-knocking daemon
http://www.zeroflux.org/projects/knock
GNU General Public License v2.0
549 stars 113 forks source link

Fix multiple memory safety issues #74

Closed dimkr closed 3 years ago

dimkr commented 3 years ago
        /* close the log file */
        if(logfd) {
                fclose(logfd);                                   <-- the FILE logfd points to is freed
        }

        if(res_cfg) {
                exit(1);
        }

        vprint("Re-opening log file: %s\n", o_logfile);
        logprint("Re-opening log file: %s\n", o_logfile);                    <- if (logfd) is true and the stdio buffer, etc' are used-after-free 

EDIT: this is only 32f3628, found more

dimkr commented 3 years ago

Remaining issue I haven't fixed:

EDIT: fixed in f327556

==10163==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f9aab00fdc6 in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6)
    #1 0x55ee815bb4c5 in list_new src/list.c:30
    #2 0x55ee815bb671 in list_add src/list.c:62
    #3 0x55ee815b3bc6 in parseconfig src/knockd.c:604
    #4 0x55ee815b1252 in main src/knockd.c:210
    #5 0x7f9aaacec0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

Indirect leak of 48 byte(s) in 2 object(s) allocated from:
    #0 0x7f9aab00fdc6 in calloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6)
    #1 0x55ee815bb4c5 in list_new src/list.c:30
    #2 0x55ee815bb6ca in list_add src/list.c:69
    #3 0x55ee815b3bc6 in parseconfig src/knockd.c:604
    #4 0x55ee815b1252 in main src/knockd.c:210
    #5 0x7f9aaacec0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: 72 byte(s) leaked in 3 allocation(s).
TDFKAOlli commented 3 years ago

Seems to be the door's allocated in parseconfig. Anyhow they should be freed by close_door() taking care of the allocated elements in door and door itself. But somewhere there I assume...

dimkr commented 3 years ago

Seems to be the door's allocated in parseconfig. Anyhow they should be freed by close_door() taking care of the allocated elements in door and door itself. But somewhere there I assume...

Needed a cup of coffee to figure this out.

list_free() iterates over doors, but after a loop where we call close_door() on every item. close_door() removes the item from doors (and frees item->data), so the list is empty and list_free() doesn't do anything. As a result, all item->data are freed by close_door(), but the linked list nodes are leaked.

TDFKAOlli commented 3 years ago

Ahh, yes you are right. 😄 The list elements itself are missing to be removed. Mhmm wouldn't it be cleaner if list_remove() would also free the list element? You still keep the close_door() function (as used at other places like the one time sequequence). Anyhow I understand the leak is there in general in the list_remove(), as the list element is taken out of the linked list and not freed, so leaked.

And about list_free() doing nothing... For whatever reason the first element always have an empty data pointer. (see list_add for an empty PMList pointer). So I thought that is a left-over when for(lp = doors; lp; lp = lp->next) has run. But on the other hand side, it also takes the head list element into account (so the one with the data pointer set NULL) and should also remove this. 😏 EDIT: Which would mean list_free(doors); is a problem if the elements are already freed by list_remove()... double free.