Bug 17897 - Multiple 'Dynamic Stack Allocations' in security point of view
Summary: Multiple 'Dynamic Stack Allocations' in security point of view
Status: NEW
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.20
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-28 23:25 UTC by Max
Modified: 2019-04-10 11:47 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Max 2015-01-28 23:25:18 UTC
Hi,

In response to

https://www.securecoding.cert.org/confluence/display/seccode/MEM05-C.+Avoid+large+stack+allocations

I would like to share the result of static code analysis. A few examples of dynamical memory allocation on the stack in glibc 2.20 for further analysis (false/positive)

====================================================
nis/nss_nisplus/nisplus-service.c
enum nss_status
_nss_nisplus_getservbyname_r (const char *name, const char *protocol,
                  struct servent *serv,
                  char *buffer, size_t buflen, int *errnop)
{
  if (tablename_val == NULL)
    {
      __libc_lock_lock (lock);
 
      enum nss_status status = _nss_create_tablename (errnop);
 
      __libc_lock_unlock (lock);
 
      if (status != NSS_STATUS_SUCCESS)
    return status;
    }
 
  if (name == NULL || protocol == NULL)
    {
      *errnop = EINVAL;
      return NSS_STATUS_NOTFOUND;
    }
 
  size_t protocol_len = strlen (protocol);
  char buf[strlen (name) + protocol_len + 17 + tablename_len]; <================================
  int olderr = errno;
...
      /* We need to allocate a new buffer since there is no
         guarantee the returned name has a length limit.  */
      const char *entryval = NISENTRYVAL(0, 0, result);
      size_t buflen = (strlen (entryval) + protocol_len + 17
               + tablename_len);
      bufptr = alloca (buflen); <================================
      snprintf (bufptr, buflen, "[cname=%s,proto=%s],%s",
            entryval, protocol, tablename_val);
  
  
====================================================
nscd/servicescache.c

static time_t
cache_addserv (struct database_dyn *db, int fd, request_header *req,
           const void *key, struct servent *serv, uid_t owner,
           struct hashentry *const he, struct datahead *dh, int errval)
{
...
 else
    {
      /* Determine the I/O structure.  */
      size_t s_name_len = strlen (serv->s_name) + 1;
      size_t s_proto_len = strlen (serv->s_proto) + 1;
      uint32_t *s_aliases_len;
      size_t s_aliases_cnt;
      char *aliases;
      char *cp;
      size_t cnt;
 
      /* Determine the number of aliases.  */
      s_aliases_cnt = 0;
      for (cnt = 0; serv->s_aliases[cnt] != NULL; ++cnt)
    ++s_aliases_cnt;
      /* Determine the length of all aliases.  */
      s_aliases_len = (uint32_t *) alloca (s_aliases_cnt * sizeof (uint32_t)); <========

====================================================
sysdeps/mach/hurd/chroot.c

int
chroot (const char *path)
{
  const char *lookup;
  size_t len;
  file_t dir, root;
  error_t err;
 
  /* Append trailing "/." to directory name to force ENOTDIR if it's not a
     directory and EACCES if we don't have search permission.  */
  len = strlen (path);
  if (len >= 2 && path[len - 2] == '/' && path[len - 1] == '.')
    lookup = path;
  else if (len == 0)
    /* Special-case empty file name according to POSIX.  */
    return __hurd_fail (ENOENT);
  else
    {
      char *n = alloca (len + 3); <============
      memcpy (n, path, len);
====================================================

locale/programs/locfile.c

int
locfile_read (struct localedef_t *result, const struct charmap_t *charmap)
{
  const char *filename = result->name;
  const char *repertoire_name = result->repertoire_name;
  int locale_mask = result->needed & ~result->avail;
  struct linereader *ldfile;
  int not_here = ALL_LOCALES;
 
  /* If no repertoire name was specified use the global one.  */
  if (repertoire_name == NULL)
    repertoire_name = repertoire_global;
 
  /* Open the locale definition file.  */
  ldfile = lr_open (filename, locfile_hash);
  if (ldfile == NULL)
    {
      if (filename != NULL && filename[0] != '/')
    {
      char *i18npath = getenv ("I18NPATH");
      if (i18npath != NULL && *i18npath != '\0')
        {
          const size_t pathlen = strlen (i18npath);
          char i18npathbuf[pathlen + 1]; <======================================

====================================================
locale/programs/locarchive.c

 
/* Check the content of the archive for duplicates.  Add the content
   of the files if necessary.  Returns the locrec_offset.  */
static uint32_t
add_locale (struct locarhandle *ah,
        const char *name, locale_data_t data, bool replace)
{
  /* First look for the name.  If it already exists and we are not
     supposed to replace it don't do anything.  If it does not exist
     we have to allocate a new locale record.  */
  size_t name_len = strlen (name);
  uint32_t file_offsets[__LC_LAST];
  
...
  /* Copy the data for all the small categories into the LC_ALL
     pseudo-category.  */
 
  data[LC_ALL].addr = alloca (data[LC_ALL].size); <======================================
  memset (data[LC_ALL].addr, 0, data[LC_ALL].size);
...
    /* If block of small categories would cross page boundary,
       align it unless it immediately follows a large category.  */
    if (cnt == LC_ALL && lastoffset != lastpos
        && ((((lastpos & (pagesz - 1)) + data[cnt].size + pagesz - 1)
         & -pagesz)
        > ((data[cnt].size + pagesz - 1) & -pagesz)))
      {
        size_t sz = pagesz - (lastpos & (pagesz - 1));
        char *zeros = alloca (sz);
 
        memset (zeros, 0, sz);
====================================================
posix/annexc.c

static int
check_header (const struct header *header, const char **except)
{
  char line[BUFSIZ], command[sizeof fmt + strlen (header->name)
                 + 2 * strlen (CC)
                 + strlen (INC) + strlen (macrofile)]; <======================================
====================================================
catgets/catgets.c

/* Open the catalog and return a descriptor for the catalog.  */
nl_catd
catopen (const char *cat_name, int flag) <====================================== cat_name on the stack
{
====================================================

Maksymilian Arciemowicz
http://cxsecurity.com/
http://cifrex.org/
Comment 1 joseph@codesourcery.com 2015-01-29 00:17:12 UTC
Please do not file omnibus bugs like this; file one bug for each separate 
instance where you believe the stack allocation is unbounded, unless two 
instances are extremely closely related (variants of the same code, 
cut-and-pasted twice, for example).

(Unbounded stack allocations are considered bugs whether or not they cross 
privilege boundaries, but are only security issues where a privilege 
boundary is plausibly crossed.)

> posix/annexc.c
> 
> static int
> check_header (const struct header *header, const char **except)
> {
>   char line[BUFSIZ], command[sizeof fmt + strlen (header->name)
>                  + 2 * strlen (CC)
>                  + strlen (INC) + strlen (macrofile)];

This is a testcase, not installed code, so cannot be a security bug.
Comment 2 Max 2015-01-29 14:55:29 UTC
> Please do not file omnibus bugs like this; file one bug for each separate 
> instance where you believe the stack allocation is unbounded, unless two 
> instances are extremely closely related (variants of the same code, 
> cut-and-pasted twice, for example).

ok. However, I didn't check yet how long buffer may be used in the examples above. Therefore, everything is in one issue.

> (Unbounded stack allocations are considered bugs whether or not they cross 
> privilege boundaries, but are only security issues where a privilege 
> boundary is plausibly crossed.)
> 

a application crash cannot be considered as a possible DoS?
Comment 3 Florian Weimer 2015-01-29 15:00:57 UTC
(In reply to Max from comment #2)
> > Please do not file omnibus bugs like this; file one bug for each separate 
> > instance where you believe the stack allocation is unbounded, unless two 
> > instances are extremely closely related (variants of the same code, 
> > cut-and-pasted twice, for example).
> 
> ok. However, I didn't check yet how long buffer may be used in the examples
> above. Therefore, everything is in one issue.

This is unfortunately the difficult part.  We treat something as a bug only if we have evidence that the alloca is actually unbounded.  (Personally, I would just call malloc/free and ban alloca and VLAs, but that's not consensus.)

> > (Unbounded stack allocations are considered bugs whether or not they cross 
> > privilege boundaries, but are only security issues where a privilege 
> > boundary is plausibly crossed.)
> > 
> 
> a application crash cannot be considered as a possible DoS?

Sure, but you'll have to demonstrate that such crashes are possible.  We have fixed many of those as (security) bugs.
Comment 4 joseph@codesourcery.com 2015-01-29 17:16:47 UTC
On Thu, 29 Jan 2015, max at cxib dot net wrote:

> > (Unbounded stack allocations are considered bugs whether or not they cross 
> > privilege boundaries, but are only security issues where a privilege 
> > boundary is plausibly crossed.)
> > 
> 
> a application crash cannot be considered as a possible DoS?

It's only a security issue if the data causing the crash is plausibly 
controlled by someone less-privileged than the user running the 
application.

For example, if an application crashed somewhere in glibc as a result of a 
1MB line in /etc/passwd, that would be a bug in glibc, but because you 
need to be privileged to put that line in /etc/passwd in the first place, 
that's not a security issue.  But if it's caused by data that might 
plausibly be untrusted (a privileged process accessing user files, or data 
received from the network, etc.), then that's a security issue.

https://sourceware.org/glibc/wiki/Security%20Process

discusses in more detail what's considered a security issue.
Comment 5 Max 2015-01-29 22:17:16 UTC
The first PoC for catopen()

https://sourceware.org/bugzilla/show_bug.cgi?id=17905

As a developer, I expect that the same function works as expected for various libc. No matter whether it is a unix libc or glibc.