apache / trafficserver

Apache Traffic Server™ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.74k stars 781 forks source link

Records parses uninitialized data for IP addresses #11449

Closed JosiahWI closed 1 week ago

JosiahWI commented 2 weeks ago

This issue was detected by valgrind.

This is the relevant part of the code, with comments added to show where things go wrong:

void
RecHttpLoadIpAddrsFromConfVar(const char *value_name, swoc::IPRangeSet &addrs)
{
  // We declare a fixed-size buffer to load the config content into. Good so far.
  char value[1024];

  // We load the config content into the buffer. It may not fill the whole buffer, but we don't know the
  // actual content length. The unused part of the buffer is uninitialized.
  if (REC_ERR_OKAY == RecGetRecordString(value_name, value, sizeof(value))) {
    Debug("config", "RecHttpLoadIpAddrsFromConfVar: parsing the name [%s] and value [%s]", value_name, value);
    // We load the value into a swoc::TextView. This TextView constructor uses the array length as the buffer size...
    // unfortunately, we don't really mean to use the whole buffer.
    // EDIT: A previous version of this comment said the resulting buffer may be missing a null terminator, but this was false.
    swoc::TextView text(value);
    while (text) {
      auto token = text.take_prefix_at(',');
      // Now we start parsing for IPs... if the config content is empty we still parse the entire buffer. If it were to
      // hypothetically contain a leftover IP address range somehow we would likely load it.
      if (swoc::IPRange r; r.load(token)) {
        Debug("config", "RecHttpLoadIpAddrsFromConfVar: marking the value [%.*s]", int(token.size()), token.data());
        addrs.mark(r);
      }
    }
  }
  Debug("config", "RecHttpLoadIpMap: parsed %zu IpMap entries", addrs.count());
}