apache / apache/trafficserver

header_rewrite: Parser::preprocess() indexes tokens[0] after pop_back() can empty the vector

Open
#13,639 0 comments 0 reactions 1 assignee Claimed by @bneradt View on GitHub
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

## Summary

`Parser::preprocess()` in header_rewrite can pop the only token off the vector and then index `tokens[0]` without checking whether anything is left.

## Detail

[`plugins/header_rewrite/parser.cc:194`](https://github.com/apache/trafficserver/blob/master/plugins/header_rewrite/parser.cc#L194) consumes a trailing flags section:

```cpp
if (tokens.size() > 0) {
std::string m = tokens[tokens.size() - 1];

if (!m.empty() && (m[0] == '[')) {
if (m[m.size() - 1] == ']') {
...
tokens.pop_back(); // consume it, so we don't concatenate it into the value
} else {
...
}
}
}

// Special case for "conditional" values
if (tokens[0].substr(0, 2) == "%{") {
```

A configuration line whose only token is a flags section, `[L]` on a line by itself for instance, gives `tokens.size() == 1`. The `pop_back()` empties the vector, and the very next statement indexes `tokens[0]`.

`std::vector::operator[]` does no bounds checking, so this is an out-of-bounds read on a configuration file that a user can write.

## Proposed fix

An `if (tokens.empty())` guard after the `pop_back()`, returning `false` with a `TSError` describing the offending line. A flags-only line is not a valid rule, so rejecting it with a message beats reading past the end of the vector.

## Context

Pre-existing, and adjacent to a hunk in [PR #13591](https://github.com/apache/trafficserver/pull/13591). Filing it separately to keep that PR purely mechanical.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.