fix Concurrency issue denial of service
Denis Andzakovic has worked out
Atftpd does not lock the `thread_list_mutex` mutex before assigning the `current` thread data structure. As a result, the daemon is vulnerable to a denial of service attack due to null pointer dereference. If `thread_data` is null when assigned to current and modified by another thread prior to the check on line 61, then atftpd crashes when dereferencing `current->next`. The following `tftpd_list.c` snippet shows the issue:
```C
46 /*
47 * Add a new thread_data structure to the list. Thread list mutex is locked
48 * before walking the list and doing the insertion.
49 */
50 int tftpd_list_add(struct thread_data *new)
51 {
52 struct thread_data *current = thread_data;
53 int ret;
54
55 pthread_mutex_lock(&thread_list_mutex);
56
57 number_of_thread++;
58
59 ret = number_of_thread;
60
61 if (thread_data == NULL)
62 {
63 thread_data = new;
64 new->prev = NULL;
65 new->next = NULL;
66 }
67 else
68 {
69 while (current->next != NULL)
70 current = current->next;
```
The `tftpd_list_add`, `tftpd_list_remove`, `tftpd_list_find_multicast_server_and_add` and `tftpd_list_kill_threads` methods all contained this vulnerable code pattern.
The denial of service can be triggered by sending a huge amount of tftpd packets simultaneously when running atftpd in daemon mode. As this vulnerability is timing constrained, the amount of packets required before a crash occurs varies. The following was generated by looping 16 threads sending tftp packets:
```None
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `./atftpd -m1000 --user doi --group doi --logfile - -v --daemon --no-fork --port'.
Program terminated with signal SIGSEGV, Segmentation fault.
69 while (current->next != NULL)
[Current thread is 1 (Thread 0x7ff1e4a93700 (LWP 27285))]
(gdb) bt
(gdb) p current
$1 = (struct thread_data *) 0x0
(gdb) x/i $pc
=> 0x557838dc9d5b <tftpd_list_add+59>: mov 0x118(%rbx),%rax
(gdb) info reg
rax 0x1 1
rbx 0x0 0
rcx 0x0 0
rdx 0x0 0
rsi 0x0 0
rdi 0x1 1
rbp 0x2 0x2
```
This issue can be resolved by locking the `thread_list_mutex` prior to assigning `current`. The following patch file details an example fix within the `tftpd_list_add` method:
```None
--- a/tftpd_list.c 2019-01-11 16:38:22.421927371 +1300
+++ b/tftpd_list.c 2019-01-11 16:38:37.638450535 +1300
@@ -49,11 +49,10 @@
*/
int tftpd_list_add(struct thread_data *new)
{
+ pthread_mutex_lock(&thread_list_mutex);
struct thread_data *current = thread_data;
int ret;
- pthread_mutex_lock(&thread_list_mutex);
-
number_of_thread++;
ret = number_of_thread;
```
Denis Andzakovic
Principal Security Consultant
Pulse Security
denis.andzakovic@...
www.pulsesecurity.co.nz
Martin Dummer
2019-04-14
| changed | tftpd_list.c |