tuntap_get_ifname should not return static sized buffer as null terminated char *

[majestrate]

tuntap_get_ifname may return a char * that is not null terminated which may cause buffer overflows in codebases in which the interface name is equal to IFNAMSIZ and assume its return value is null terminated. correct this and make api a bit more sane for that case.

[majestrate]

On Sat, Jan 05, 2019 at 10:23:02PM -0800, Pichot wrote:

chipot requested changes on this pull request.

@@ -57,8 +57,9 @@ tuntap_destroy(struct device *dev) { }

char -tuntap_get_ifname(struct device dev) {

• return dev->if_name; +tuntap_get_ifname(struct device dev, char dst, size_t sz) {
• strncpy(dst, dev->if_name, (sz > IFNAMSIZ ? IFNAMSIZ : sz));

This does not solve your problem at all, as strncpy does not null-terminate the destination string. that should've been strlcpy

What are you trying to achieve ? Always getting a null-terminated buffer, accepting eventual truncation ?

If someone use this function with a dest buffer of IFNAMSIZ, it will still be not null-terminated.

Maybe the buffer in struct device shall actually be one byte larger to store a \0 in case the interface name is exactly IFNAMSIZ. Then we can return it with the previous api without safety issues.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/LaKabane/libtuntap/pull/15#pullrequestreview-189617705

[majestrate]

On Sat, Jan 05, 2019 at 10:23:02PM -0800, Pichot wrote:

chipot requested changes on this pull request.

@@ -57,8 +57,9 @@ tuntap_destroy(struct device *dev) { }

char -tuntap_get_ifname(struct device dev) {

• return dev->if_name; +tuntap_get_ifname(struct device dev, char dst, size_t sz) {
• strncpy(dst, dev->if_name, (sz > IFNAMSIZ ? IFNAMSIZ : sz));

This does not solve your problem at all, as strncpy does not null-terminate the destination string.

What are you trying to achieve ? Always getting a null-terminated buffer, accepting eventual truncation ?

If someone use this function with a dest buffer of IFNAMSIZ, it will still be not null-terminated.

Maybe the buffer in struct device shall actually be one byte larger to store a \0 in case the interface name is exactly IFNAMSIZ. Then we can return it with the previous api without safety issues.

actually yea you're right it should be IFNAMIZ + 1

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/LaKabane/libtuntap/pull/15#pullrequestreview-189617705

[tleguern]

According to my tests on Linux (Ubuntu 18.04) it is acceptable to provide a 16 bytes long, non-null-terminated string as an interface name with the ioctl(2) call SIOCSIFNAME. Applications such as ip(1) or ifconfig(1) then truncate the name before use.

[tleguern]

@majestrate can you squash your commits so the only change left is the modification of the size of if_name in private.h?

[majestrate]

squashed