Page MenuHomeFreeBSD

devel/electron11: fix obtaining HOST_NAME_MAX
ClosedPublic

Authored by a.wolk_fudosecurity.com on Apr 30 2021, 8:11 PM.
Referenced Files
Unknown Object (File)
Sun, Jan 15, 10:51 PM
Unknown Object (File)
Dec 24 2022, 5:35 AM
Unknown Object (File)
Dec 18 2022, 5:57 AM
Unknown Object (File)
Dec 2 2022, 10:19 AM
Subscribers

Details

Summary

FreeBSD removed the HOST_NAME_MAX define in an attempt to have donwstream
consumers use the sysconf(3) API. This change has lead to various consumers
erroneously using the new API. Either mistaking _SC_HOST_NAME_MAX, which is
a parameter to sysconf(3) as the replacement of HOST_NAME_MAX or by using
sysconf(_SC_HOST_NAME_MAX) as a compile time constant which in turn can
lead to accidental introduction of variable length arrays (VLA).

The decision has been documented in /usr/include/sys/syslimits.h

/*                                                                              
 * We leave the following values undefined to force applications to either      
 * assume conservative values or call sysconf() to get the current value.       
 *                                                                              
 * HOST_NAME_MAX                                                                
 *                                                                              
 * (We should do this for most of the values currently defined here,            
 * but many programs are not prepared to deal with this yet.)                   
 */

The following snippet of code demonstrates _POSIX_HOST_NAME_MAX which is the
equivalent of HOST_NAME_MAX on most platforms and the confusion between _SC_HOST_NAME_MAX
and sysconf(_SC_HOST_NAME_MAX):

$ cat hostname.c 
#include <unistd.h>
#include <stdio.h>
#include <limits.h>
int
main(int argc, char **argv)
{
        printf("_POSIX_HOST_NAME_MAX=%d\n", _POSIX_HOST_NAME_MAX);
        printf("_SC_HOST_NAME_MAX=%d\n", _SC_HOST_NAME_MAX);
        printf("sysconf(_SC_HOST_NAME_MAX)=%ld\n", sysconf(_SC_HOST_NAME_MAX));
        return 0;
}

Output:

$ ./hostname
_POSIX_HOST_NAME_MAX=255
_SC_HOST_NAME_MAX=72
sysconf(_SC_HOST_NAME_MAX)=255
$ 

By accident _SC_HOST_NAME_MAX has a value large enough to not be spotted in most 'quick tests' people
might make.

This review addresses one of the places of misuse I found. Stay tuned as more will follow.

When committing any of these changes please credit the work as:

Sponsored by: Fudo Security

One of such consumers is port devel/electron11 where a patch lifted from www/chromium introduces
an error:

files/patch-components_sync__device__info_local__device__info__util__linux.cc
7:-  char hostname[HOST_NAME_MAX];
8:-  if (gethostname(hostname, HOST_NAME_MAX) == 0)  // Success.
9:+  int len = sysconf(_SC_HOST_NAME_MAX);
11:+  if (gethostname(hostname, _SC_HOST_NAME_MAX) == 0)  // Success.

The diff goes out of it's way to use sysconf(3) and introduces a variable length
array (VLA). It then fails to use the computed length in call to gethostname passing
in an accidental value '72'.

The proper non-intrusive fix for this codebase is to use _POSIX_HOST_NAME_MAX.

Interestingly, there was a different approach in www/chromium in another path in the same port:

files/patch-components_policy_core_common_cloud_cloud__policy__util.cc
35:+  char hostname[MAXHOSTNAMELEN];
36:+  if (gethostname(hostname, MAXHOSTNAMELEN) == 0)

which uses MAXHOSTNAMELEN instead of HOST_NAME_MAX obtained indirectly (not present
in the patched file) by using sys/params.h in the source tree. This patch however has not
been copied to this port. Which makes me wonder if it is also really needed in Chromium and
if so then why is it not needed here?

In the spirit of least intrusion into a fragile and important codebase like Chromium I'm
opting for the smallest change of using _POSIX_HOST_NAME_MAX instead of sysconf(3) and
not changing MAXHOSTNAMELEN as that is already a tested change.

Suggested commit message:

commit a9e5fa29e675a161a170594446f01e86ad788a54 (HEAD -> main)
Author: Adam Wolk <a.wolk@fudosecurity.com>
Date:   Fri Apr 30 22:05:48 2021 +0200

    devel/electron11: fix obtaining HOST_NAME_MAX
    
    Using sysconf(3) API lead to accidental introduction of variable length arrays
    (VLA) in the port. Additionally gethostname(3) has been passed _SC_HOST_NAME_MAX
    incorrectly as the HOST_NAME_MAX length.
    
    Fall back to using _POSIX_HOST_NAME_MAX as the remaining code is not ready for
    introducing sysconf(3) as a patch.
    
    Bump PORTREVISION to rebuild with the new patch.
    
    The patch was lifted from www/chromium which has a similar diff pending.
    
    Sponsored by:   Fudo Security

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Thanks for the head up and sorry for getting back this late.

devel/electron11 has been replaced with devel/electron13. I looked into the patch file and found the issue still existed.

The patch looks good to me. I will commit it later.

Thanks!

This revision is now accepted and ready to land.Dec 14 2021, 6:37 AM

Patch committed. Thanks for your contribution!