google / google/robotstxt

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

Open
#87 0 comments 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

Research direction

Start with robots.cc and reproduce the percent-encoding case using the bundled CLI command in the issue. Read MaybeEscapePattern(), GetPathParamsQuery(), Matches(), and ExtractUserAgent() to verify each observation separately. The issue is done only after the project agrees on scope and confirms fixes for the selected findings with regression coverage and bounded behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.