google / google/robotstxt

[Security] Multiple Memory Safety and DoS Vulnerabilities in robots.cc

Open
#85 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
3.5k
Forks
255
PR merge metrics
No merged PRs in 30d

Description

### Summary

I identified several issues in the `google/robotstxt` library during fuzzing and manual code review. These observations involve URL matching consistency, algorithmic complexity, boundary handling, and integer arithmetic robustness within `robots.cc`.

The most significant finding is an inconsistency in how percent-encoded paths are normalized and matched. This can lead to different policy decisions for semantically equivalent URLs.

These findings were previously shared with the Google OSS VRP, and I was advised to open an issue here for project-level review and discussion.

---

### Observations & Analysis

#### 1. Inconsistent Percent-Encoding Matching

**Detail**

`MaybeEscapePattern()` normalizes lowercase percent-encoded sequences appearing in `robots.txt` rules (e.g. `%aa` → `%AA`).

However, the URI path extracted by `GetPathParamsQuery()` does not appear to undergo equivalent normalization before matching.

As a result, semantically equivalent percent-encoded representations may produce different matching outcomes because matching is performed using case-sensitive comparisons.

**Example**

Rule:

```text
Disallow: /secret%aa
```

Internal representation:

```text
/secret%AA
```

URI:

```text
/secret%aa
```

Observed result:

```text
ALLOWED
```

Expected result:

```text
DISALLOWED
```

**Impact**

This is not an authorization bypass in the traditional sense because `robots.txt` is not an authorization mechanism.

However, it appears to cause inconsistent policy evaluation where equivalent URL representations can lead to different crawler decisions. Applications relying on `robotstxt` matching for crawler restrictions or policy enforcement may observe unintended behavior.

---

#### 2. Algorithmic Complexity in `Matches`

**Detail**

The matching algorithm exhibits `O(N × M)` worst-case behavior.

Specific combinations of long paths and wildcard-heavy patterns (for example `*a*a*a...`) can cause significant growth in processing time.

**Measurements**

| Path Length | Pattern Size | Time |
|------------|------------|------|
| 10,000 | 200 | 0.05s |
| 80,000 | 1600 | 3.73s |

**Impact**

This may lead to excessive CPU consumption in high-throughput parsing environments.

---

#### 3. Boundary Handling in `ExtractUserAgent`

**Detail**

`ExtractUserAgent()` iterates through a `string_view` without an explicit `p < end` boundary check.

While normal usage may provide safe backing storage, the parser can advance beyond the logical range represented by the `string_view`.

**Impact**

This is primarily a robustness and defensive-programming concern. Adding explicit boundary validation would ensure that parsing remains constrained to the intended slice.

---

#### 4. Integer Arithmetic Robustness in `Matches`

**Detail**

`numpos` is stored as a 32-bit `int`.

Calculations derived from path length may overflow for sufficiently large inputs, potentially resulting in unexpected behavior.

**Impact**

Although extremely large paths are uncommon, replacing path-length-derived counters with `size_t` would improve resilience and eliminate this edge case.

---

### Reproduction (Observation #1)

The matching inconsistency can be reproduced using the bundled CLI tool.

```bash
# Create robots.txt
echo -e "User-agent: *\nDisallow: /secret%aa" > robots.txt

# Run matcher
./build/robots robots.txt FooBot "http://example.com/secret%aa"
```

Observed:

```text
ALLOWED
```

Expected:

```text
DISALLOWED
```

---

### Suggested Fixes

#### Fix 1: Consistent Percent-Encoding Normalization

Apply the same percent-encoding normalization logic to incoming URI paths before matching.

#### Fix 2: Complexity Safeguards

Consider introducing a complexity threshold, state limit, or other safeguards for wildcard-heavy patterns.

#### Fix 3: Boundary Validation

Add explicit bounds checking in `ExtractUserAgent()`:

```cpp
while (p < end && ...)
```

#### Fix 4: Type Safety

Use `size_t` for path-length-derived counters and indices in `Matches()`.

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.