Skip to content

Commit

Permalink
pppoe: Fix crash when a too-long device name is given (#447)
Browse files Browse the repository at this point in the history 
Fix for github issue #446.

Signed-off-by: Eivind Næss <[email protected]>
  • Loading branch information
@enaess
enaess authored Sep 28, 2023
1 parent f5aa69b commit 91b203f
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions pppd/plugins/pppoe/if.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ openInterface(char const *ifname, UINT16_t type, unsigned char *hwaddr)
sa.sll_ifindex = ifr.ifr_ifindex;

#else
strcpy(sa.sa_data, ifname);
strlcpy(sa.sa_data, ifname, sizeof(sa.sa_data));
#endif

/* We're only interested in packets on specified interface */
Expand Down Expand Up @@ -212,7 +212,7 @@ sendPacket(PPPoEConnection *conn, int sock, PPPoEPacket *pkt, int size)
#else
struct sockaddr sa;

strcpy(sa.sa_data, conn->ifName);
strlcpy(sa.sa_data, conn->ifName, sizeof(sa.sa_data));
err = sendto(sock, pkt, size, 0, &sa, sizeof(sa));
#endif
if (err < 0) {
Expand Down

3 comments on commit 91b203f

@Reverier-Xu
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really not a problem to truncate the interface name like this? If two devices with long names are only different on the suffix, then there are still bugs here.

@paulusmack
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really not a problem to truncate the interface name like this? If two devices with long names are only different on the suffix, then there are still bugs here.

Actually, I don't know whether or not that case could ever arise. The code being modified here is only used on Linux systems that don't have struct sockaddr_ll, which is not the usual case. Perhaps it reflects an old kernel with old headers. In that case it may not be possible for interfaces to have names that are long enough to trigger this problem.

We could add a check to make openInterface() fail if the interface name is too long.

@Neustradamus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Reverier-Xu: Can you go and look all comments here:

Please sign in to comment.