Skip to content

STUN: address-of-pointer copy after stun_atoaddr() overwrites the resolved server address #335

Description

@garacil

Summary

In libsofia-sip-ua/stun/stun.c, both stun_test_lifetime() and stun_get_nattype(), on the branch where a server address string is supplied, copy from &sd->sd_pri_info.ai_addr right after calling stun_atoaddr(). That source is the address of the ai_addr pointer field inside the su_addrinfo_t, not the socket address it points to. The copy overwrites the resolved server address held in sd->sd_pri_addr with su_addrinfo_t tail bytes (the pointer value and the fields after it). Because sd_pri_info sits immediately before sd_pri_addr in struct stun_discovery_s, the two regions also overlap.

Why the copy is wrong and unnecessary

stun_discovery_create() aims ai_addr at the destination buffer:

stun.c:1148   sd->sd_pri_info.ai_addr = &sd->sd_pri_addr->su_sa;

stun_atoaddr() then writes the resolved address through that pointer:

stun.c:2623   dstaddr = (su_sockaddr_t *) info->ai_addr;
   ...         memcpy(&dstaddr->su_sa, res->ai_addr, sizeof(struct sockaddr));

So when stun_atoaddr() returns, sd->sd_pri_addr already contains the resolved server address. The follow-up copy is redundant, and by reading &sd->sd_pri_info.ai_addr it clobbers that address with unrelated bytes.

Affected sites:

stun.c:2740  (stun_test_lifetime)  memcpy (sd->sd_pri_addr, &sd->sd_pri_info.ai_addr, sizeof(su_sockaddr_t));
stun.c:1251  (stun_get_nattype)    memmove(sd->sd_pri_addr, &sd->sd_pri_info.ai_addr, sizeof(su_sockaddr_t));

GCC 14 at -O2 also reports the overlap at the memcpy site:

stun.c:2740: warning: 'memcpy' accessing 32 bytes at offsets 96 and 72 overlaps 8 bytes at offset 96 [-Wrestrict]

The memmove site does not warn (memmove permits overlap) but carries the same semantic bug.

Proposed fix

Remove the redundant copy in both server branches; stun_atoaddr() already wrote the resolved address into sd_pri_addr. The no-server branch's copy from sh_pri_addr is a genuine copy between two different objects and stays unchanged. A patch follows as a pull request.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions