Bug 107350 - Make it clearer that we do not overflow struct sockaddr_un.sun_path
Summary: Make it clearer that we do not overflow struct sockaddr_un.sun_path
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Linux (All)
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-07-23 17:58 UTC by Simon McVittie
Modified: 2018-08-02 22:23 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
sysdeps: Reassure gcc 8 that we are not overflowing struct sockaddr_un (3.41 KB, patch)
2018-07-23 17:58 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2018-07-23 17:58:34 UTC
Created attachment 140793 [details] [review]
sysdeps: Reassure gcc 8 that we are not overflowing struct sockaddr_un

Using strncpy (buffer, str, strlen (str)) is a "code smell" that
might indicate a serious bug (it effectively turns strncpy into
strcpy), and gcc 8 now warns about it. In fact we avoided the bug
here, but it wasn't at all obvious.
    
We already checked that path_len is less than or equal to
_DBUS_MAX_SUN_PATH_LENGTH, which is 99, chosen to be strictly less
than the POSIX minimum sizeof(sun_path) >= 100, so we couldn't
actually be overflowing the available buffer.
    
The new static assertion in this commit matches a comment above the
definition of _DBUS_MAX_SUN_PATH_LENGTH: we define
_DBUS_MAX_SUN_PATH_LENGTH to 99, because POSIX says struct
sockaddr_un's sun_path member is at least 100 bytes (including space
for a \0 terminator). dbus will now fail to compile on
platforms that are non-POSIX-compliant in this way, except for Windows.
    
We zeroed the struct sockaddr_un before writing into it, so stopping
one byte short of the end of sun_path ensures that we get \0
termination.

---

Part 1703 in the ongoing series "why string abstractions are better than strncpy()".

sizeof(sun_path) is actually 108 on Linux, and probably other platforms.
Comment 1 Simon McVittie 2018-07-23 19:34:11 UTC
D-Bus addresses have to come from a trusted source (if an attacker can induce you to connect to a crafted D-Bus address then you have bigger things to worry about, especially the unixexec: transport) so this wouldn't be a security problem even if it did overflow.
Comment 2 Thiago Macieira 2018-07-29 01:54:10 UTC
Comment on attachment 140793 [details] [review]
sysdeps: Reassure gcc 8 that we are not overflowing struct sockaddr_un

Review of attachment 140793 [details] [review]:
-----------------------------------------------------------------

LGTM.
Comment 3 Philip Withnall 2018-07-30 14:53:32 UTC
Comment on attachment 140793 [details] [review]
sysdeps: Reassure gcc 8 that we are not overflowing struct sockaddr_un

Review of attachment 140793 [details] [review]:
-----------------------------------------------------------------

LGTM. Your reasoning about it not being a security problem seems sound.
Comment 4 Simon McVittie 2018-08-02 22:23:49 UTC
Fixed in git for 1.10.28, 1.12.10 and 1.13.6


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.