Skip to end of metadata
Go to start of metadata

Do not make any assumptions about the size of environment variables because an adversary might have full control over the environment. If the environment variable needs to be stored, the length of the associated string should be calculated and the storage dynamically allocated (see STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator).

Noncompliant Code Example

This noncompliant code example copies the string returned by getenv() into a fixed-size buffer:

void f() {
  char path[PATH_MAX]; /* Requires PATH_MAX to be defined */
  strcpy(path, getenv("PATH"));
  /* Use path */
}

Even if your platform assumes that $PATH is defined, defines PATH_MAX, and enforces that paths not have more than PATH_MAX characters, the $PATH environment variable still is not required to have less than PATH_MAX chars. And if it has more than PATH_MAX chars, a buffer overflow will result. Also, if $PATH is not defined, then strcpy() will attempt to dereference a null pointer.

Compliant Solution

In this compliant solution, the strlen() function is used to calculate the size of the string, and the required space is dynamically allocated:

void f() {
  char *path = NULL;
  /* Avoid assuming $PATH is defined or has limited length */
  const char *temp = getenv("PATH");
  if (temp != NULL) {
    path = (char*) malloc(strlen(temp) + 1);
    if (path == NULL) {
      /* Handle error condition */
    } else {
      strcpy(path, temp);
    }
    /* Use path */
  }
}

Risk Assessment

Making assumptions about the size of an environmental variable can result in a buffer overflow.

Recommendation

Severity

Likelihood

Remediation Cost

Priority

Level

ENV01-C

High

Likely

Medium

P18

L1

Automated Detection

Tool

Version

Checker

Description

CodeSonar
5.1p0

LANG.MEM.BO
LANG.MEM.TO
(general)

Buffer overrun
Type overrun
CodeSonar's taint analysis includes handling for taint introduced through the environment

Compass/ROSE



Can detect violations of the rule by using the same method as STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator

Klocwork
2018
ABV.ANY_SIZE_ARRAY
ABV.GENERAL
ABV.ITERATOR
ABV.MEMBER
ABV.STACK
ABV.TAINTED
ABV.UNKNOWN_SIZE
ABV.UNICODE.BOUND_MAP
ABV.UNICODE.FAILED_MAP
ABV.UNICODE.NNTS_MAP
ABV.UNICODE.SELF_MAP

Parasoft C/C++test
10.4.2

CERT_C-ENV01-a
CERT_C-ENV01-b
CERT_C-ENV01-c

Don't use unsafe C functions that do write to range-unchecked buffers
Avoid using unsafe string functions which may cause buffer overflows
Avoid overflow when writing to a buffer

Polyspace Bug Finder

R2019b

CERT C: Rec. ENV01-C

Checks for tainted NULL or non-null-terminated string (rec. partially covered)

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

Related Guidelines

MITRE CWECWE-119, Improper Restriction of Operations within the Bounds of a Memory Buffer
CWE-123, Write-what-where Condition
CWE-125, Out-of-bounds Read

Bibliography

[IEEE Std 1003.1:2013]Chapter 8, "Environment Variables"
[Viega 2003]Section 3.6, "Using Environment Variables Securely"



10 Comments

  1. True, although I think it's still valuable if only as a reminder. I'm pretty sure I've seen code like this:

    void f() {
        /* allocate enough room for PATH */
        char path [PATH_MAX];
        /* PATH must be defined */
        strcpy(path, getenv("PATH"));
        /* use path */
    }
    

    Might this be a better example?

    1. Yes. Reworked the NCCE/CS to use your sample.

      1. Oops...accompanying text need to be updated too.

        It does seem that several platforms, including Linux and OSX allow paths to be constructed that are longer than PATH_MAX. While this is an issue, we shouldn't deal with it here (maybe set aside for the POSIX section.) So we focus on that fact that an attacker can set $PATH to anything they wish, incl. a string longer than PATH_MAX.

        1. Not only that, an implementation that doesn't impose a restriction on the length of a pathname need not define PATH_MAX at all. (When it is defined, it gives the maximum length of a pathname that a system call is able to process at a time. Pathnames that are longer than that are valid, they just cannot be processed in a single call but may require intermediate steps.)

          1. True, PATH_MAX need not be defined. If it isn't, then the NCCE won't compile. Added the assumption that PATH_MAX is at least defined (smile)

            1. In my experience, assuming that PATH_MAX and other similar resource limit constants (or optional types) are necessarily defined, expand to values suitable for use in array declarations, or represent parameters that cannot change at runtime is a sufficiently common mistake to warrant a guideline warning users against such assumptions.

  2. The Compliant Solution as of JUN-14-2010 is a mess:

    void f() {
      char *copy = NULL;
      /* avoid assuming $PATH is defined or has limited length */
      const char *temp = getenv("PATH");
      if (temp != NULL) {
        path = (char*) malloc(strlen(temp) + 1);
        if (copy == NULL) {
          /* Handle error condition */
        } else {
          strcpy(path, temp);
        }
        /* use path */
      }
    }
    

    It uses copy and path when it should use only one of them in addition to temp. path is undeclared and the test for if (copy == NULL) is always true. I find it rather embarrasing that bugs like this slip in such a carefully crafted coding guideline. Has it never been considered to at least test compile the Compliant Solutions (at least with -c; no main() required, but correct headers, please)? I don't dare asking for test linting... :-)

    1. I've fixed the compliant solution (and even compiled it (smile)

  3. This recommendation and example appears to be entirely redundant with the getenv() example in STR31-C.  We should probably eliminate this, because recommendation instead of the example, because this code example violates STR31-C.