DhavalKapil / icmptunnel

Transparently tunnel your IP traffic through ICMP echo and reply packets.
https://dhavalkapil.com/icmptunnel/
3.06k stars 342 forks source link

Issue #3 Security issues and small improvements #10

Closed jjungo closed 8 years ago

jjungo commented 8 years ago

Hi,

Strncpy should be use in order to prevent nasty buffer overflows.

tunnel.c: L:212 was subject to buffer overflow because dest is provided by user and can be smaller than packet.src_addr. We need a bigger memory allocation before calling run_tunnel and pass a copy pointer into this function.

I hope I can help this project, 'cause your job is nice!

Best regards

rofl0r commented 8 years ago

Strncpy should be use in order to prevent nasty buffer overflows.

wrong. usage of strncpy is almost always a bug since it doesn't do what most ppl (incl @jjungo ) think:

$ man 3p strncpy

     The  stpncpy()  and  strncpy() functions shall copy not more than n bytes
     (bytes that follow a NUL character are not copied) from the array pointed
     to by s2 to the array pointed to by s1.

     If  the  array pointed to by s2 is a string that is shorter than n bytes,
     NUL characters shall be appended to the copy in the array pointed  to  by
     s1, until n bytes in all are written.

     If  copying  takes  place  between  objects that overlap, the behavior is
     undefined.

tl;dr: strncpy is a function to copy strings and pad the remaining space with 0s (designed to be used with record blocks), without checking if the string is properly terminated. this leads to a lot of bugs.

the proper C99-way to copy a string with buffer size check is snprintf(dest, sizeof dest, "%s", src)