220 lines
11 KiB
Markdown
220 lines
11 KiB
Markdown
# Patch notes — in-document links / false-positive heuristic fix
|
|
|
|
## Problem
|
|
|
|
Operator-reported false positives while testing the heuristics against
|
|
real documents containing in-document navigation:
|
|
|
|
1. **Markdown / EPUB / DOCX hyperlinks** with fragment or relative-URL
|
|
destinations (e.g. `[Section 2](#section-2)`,
|
|
`[next chapter](./chapter2.html)`, `[page](page.html#anchor)`) were
|
|
being flagged as **`Malicious(SuspiciousUri)`** — even though they
|
|
point to another region of the *same* document and don't navigate
|
|
anywhere external.
|
|
|
|
2. **`mailto:user@example.com`** links (explicitly allowed in
|
|
`Config::allowed_uri_schemes`) were *also* being flagged as
|
|
`Malicious(SuspiciousUri)`. This was the same bug surfacing in a
|
|
different shape.
|
|
|
|
3. **PDF `/GoTo` actions** (in-document page/destination jumps inside
|
|
the same PDF) were conflated with `/GoToR` (remote navigation to
|
|
*another* PDF file) at the parser level. Both were classified as
|
|
`Suspicious`, so any PDF using bookmarks or table-of-contents links
|
|
produced a noisy finding per clickable destination.
|
|
|
|
### Root cause — `classify_uri` scheme parsing
|
|
|
|
The original `classify_uri` extracted the URI scheme with:
|
|
|
|
```rust
|
|
let scheme = uri
|
|
.split("://")
|
|
.next()
|
|
.map(|s| s.to_ascii_lowercase())
|
|
.unwrap_or_default();
|
|
```
|
|
|
|
`split("://")` only matches **hierarchical** schemes (`https://`,
|
|
`ftp://`, `file://`). For anything else it returns the whole URI as
|
|
the first segment, so the "scheme" became the entire URI string:
|
|
|
|
| URI | Parsed "scheme" | Allowed? | Result |
|
|
|------------------------------|------------------------|----------|-------------|
|
|
| `https://example.com` | `https` | yes | Benign ✓ |
|
|
| `#section-2` | `#section-2` | no | Malicious ✗ |
|
|
| `./chapter2.html` | `./chapter2.html` | no | Malicious ✗ |
|
|
| `mailto:user@example.com` | `mailto:user@example.com` | no | Malicious ✗ |
|
|
| `javascript:alert(1)` | `javascript:alert(1)` | no | Malicious ✓ (by luck — same result, wrong reason) |
|
|
|
|
The intent of the original code was clearly "if there's no scheme,
|
|
treat it as a relative URL and return Benign" — see the trailing
|
|
fallback `ThreatClassification::Benign` at the end of `classify_uri`.
|
|
But that fallback was unreachable in practice, because `scheme` was
|
|
never actually empty when the URI contained any characters at all.
|
|
|
|
### Root cause — PDF `/GoTo` vs `/GoToR` conflation
|
|
|
|
The PDF parser had:
|
|
|
|
```rust
|
|
"GoToR" | "GoTo" => {
|
|
vectors.push(ExecutableVector {
|
|
vector_type: VectorType::PdfGoToR,
|
|
...
|
|
});
|
|
}
|
|
```
|
|
|
|
Both action types were emitted under the single `VectorType::PdfGoToR`
|
|
variant, and the heuristics classified that variant as `Suspicious`.
|
|
So `/GoTo` (in-document) and `/GoToR` (remote) were indistinguishable
|
|
downstream.
|
|
|
|
## Fix
|
|
|
|
### 1. Proper RFC 3986 scheme extraction
|
|
|
|
Added `extract_uri_scheme(uri: &str) -> Option<&str>` in
|
|
`src/scanner/heuristics.rs` implementing the RFC 3986 §3.1 scheme
|
|
grammar:
|
|
|
|
```
|
|
scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) ":"
|
|
```
|
|
|
|
The scheme is the longest prefix of `uri` that matches that grammar,
|
|
ending at the first `:`. If no such prefix exists (i.e. there is no
|
|
`:` in the URI, or the part before `:` doesn't match the scheme
|
|
grammar), the URI has no scheme — it's an in-document link.
|
|
|
|
| URI | Extracted scheme | In allowed list? | Result |
|
|
|------------------------------|-------------------|------------------|-------------|
|
|
| `https://example.com` | `Some("https")` | yes | phishing check → Benign |
|
|
| `#section-2` | `None` | n/a | phishing check → Benign |
|
|
| `./chapter2.html` | `None` | n/a | phishing check → Benign |
|
|
| `page.html#anchor` | `None` | n/a | phishing check → Benign |
|
|
| `mailto:user@example.com` | `Some("mailto")` | yes | phishing check → Benign |
|
|
| `javascript:alert(1)` | `Some("javascript")` | no | Malicious ✓ |
|
|
| `data:text/html,<x>` | `Some("data")` | no | Malicious ✓ |
|
|
| `vbscript:msgbox` | `Some("vbscript")`| no | Malicious ✓ |
|
|
|
|
### 2. In-document links still get phishing-checked
|
|
|
|
The user's intent — paraphrased — was:
|
|
|
|
> An in-document link to another region of the same document shouldn't
|
|
> raise a flag. It should be used to check for *other* flags, but not
|
|
> raise an alert itself.
|
|
|
|
So `classify_uri` now routes scheme-less URIs through
|
|
`classify_phishing_signal(uri, config)` (a small helper extracted from
|
|
the original logic). If a strong phishing signal fires
|
|
(`BrandHomograph`, `IpHost`, `CredentialUrl`) the URI is still
|
|
classified as `Malicious(SuspiciousUri)`. If a weak signal fires
|
|
(`Shortener`, `SuspiciousKeyword`, `PhishingTld`) it's still
|
|
`Suspicious`. Only when no phishing signal fires does the URI become
|
|
`Benign`.
|
|
|
|
Examples of in-document links that STILL get flagged (correctly):
|
|
|
|
| URI | Phishing signal | Result |
|
|
|------------------------------|------------------------|-------------|
|
|
| `#login-verify` | `SuspiciousKeyword` ("login", "verify") | Suspicious |
|
|
| `#micros0ft-attack-vector` | `BrandHomograph` ("micros0ft") | Malicious |
|
|
| `./page.html?account=verify` | `SuspiciousKeyword` | Suspicious |
|
|
|
|
### 3. PDF `/GoTo` (in-document) vs `/GoToR` (remote)
|
|
|
|
Added a new `VectorType::PdfGoTo` variant in `src/core/types.rs`
|
|
(documented as "PDF `/GoTo` action — in-document navigation").
|
|
Updated the PDF parser to dispatch on the action type:
|
|
|
|
```rust
|
|
"GoTo" => { /* emit VectorType::PdfGoTo */ }
|
|
"GoToR" => { /* emit VectorType::PdfGoToR */ }
|
|
```
|
|
|
|
Updated the heuristics `classify_vector` match:
|
|
|
|
```rust
|
|
// PDF /GoTo — in-document navigation. Benign.
|
|
VectorType::PdfGoTo => ThreatClassification::Benign,
|
|
|
|
// PDF /GoToR — remote navigation. Still Suspicious.
|
|
VectorType::PdfGoToR => ThreatClassification::Suspicious,
|
|
```
|
|
|
|
Because `inspect_vector` returns `None` for `Benign` findings, an
|
|
in-document `/GoTo` produces no alert and no quarantine entry. The
|
|
vector is still recorded in `Document::executable_vectors` so the
|
|
operator can see in-document navigation activity if they want to.
|
|
|
|
## Files changed
|
|
|
|
| File | Change |
|
|
|------|--------|
|
|
| `src/core/types.rs` | Added `VectorType::PdfGoTo` variant + `"pdf-goto"` display string. Updated doc comment on `PdfGoToR` to clarify it's for REMOTE navigation only. |
|
|
| `src/parsers/pdf_parser.rs` | Split `"GoToR" \| "GoTo"` match arm in `inspect_action` into two separate arms emitting `PdfGoTo` (in-document) and `PdfGoToR` (remote) respectively. |
|
|
| `src/scanner/heuristics.rs` | Added `VectorType::PdfGoTo => ThreatClassification::Benign` case in `classify_vector`. Replaced broken `split("://")` scheme extraction with proper RFC 3986 `extract_uri_scheme` helper. Refactored phishing-signal escalation into `classify_phishing_signal` helper that's now called for BOTH scheme-bearing and scheme-less URIs (so in-document links still get phishing-checked). |
|
|
| `src/scanner/heuristics.rs` (tests) | Added 10 new tests covering: RFC 3986 scheme extraction, fragment links, relative URLs, bare page links, `mailto:` links, in-document links with phishing signals, and the new `PdfGoTo`/`PdfGoToR` distinction. |
|
|
|
|
## Tests added
|
|
|
|
In `src/scanner/heuristics.rs::tests`:
|
|
|
|
| Test name | What it asserts |
|
|
|-----------|-----------------|
|
|
| `extract_uri_scheme_handles_rfc3986_cases` | The new scheme extractor handles hierarchical schemes, opaque schemes (`mailto:`, `javascript:`, `data:`, `vbscript:`), schemes with digits/+/-/., and correctly returns `None` for fragment links, relative URLs, and empty URIs. |
|
|
| `fragment_link_is_benign` | `#section` (Markdown) → no finding emitted. |
|
|
| `relative_url_is_benign` | `./page.html` → no finding emitted. |
|
|
| `bare_page_link_is_benign` | `page.html` → no finding emitted. |
|
|
| `fragment_with_anchor_is_benign` | `chapter1.html#section-2` → no finding emitted. |
|
|
| `mailto_link_is_benign` | `mailto:user@example.com` → no finding emitted (was a false positive before the fix). |
|
|
| `in_document_link_with_phishing_keyword_still_flagged` | `#login-verify` → Suspicious finding with `suspicious-keyword` note (proves in-document links still get phishing-checked). |
|
|
| `in_document_link_with_brand_homograph_still_flagged` | `#micros0ft-attack-vector` → Malicious finding with `brand-homograph` note. |
|
|
| `pdf_goto_in_document_navigation_is_benign` | `VectorType::PdfGoTo` → no finding emitted. |
|
|
| `pdf_gotor_remote_navigation_is_suspicious` | `VectorType::PdfGoToR` → Suspicious (preserved behavior). |
|
|
|
|
## Test results
|
|
|
|
Before the fix:
|
|
- `cargo test --lib` → 127 passed, 0 failed
|
|
- `cargo test --test pipeline_integration` → 23 passed, 0 failed
|
|
- `cargo test --test zip_bomb_defense` → 4 passed, 0 failed
|
|
|
|
After the fix:
|
|
- `cargo test --lib` → **137 passed**, 0 failed (+10 new tests)
|
|
- `cargo test --test pipeline_integration` → 23 passed, 0 failed (unchanged)
|
|
- `cargo test --test zip_bomb_defense` → 4 passed, 0 failed (unchanged)
|
|
|
|
All pre-existing tests continue to pass, including the suite that
|
|
exercises real PDF / DOCX / EPUB / Markdown fixtures in
|
|
`tests/fixtures/`.
|
|
|
|
## Known limitations / non-goals
|
|
|
|
1. **Windows file paths** like `C:\path\to\file.txt` will be parsed
|
|
by `extract_uri_scheme` as having scheme `"C"` (because `C` is a
|
|
valid RFC 3986 scheme character), and the heuristics will flag it
|
|
as Malicious because `"C"` isn't in `Config::allowed_uri_schemes`.
|
|
This is an acceptable false positive for an unusual input — Windows
|
|
file paths should be encoded as `file:///C:/path/to/file` in URIs.
|
|
The same applies to single-letter drive prefixes generally.
|
|
|
|
2. **The PDF parser doesn't yet capture `/GoTo` destination payloads.**
|
|
Both `PdfGoTo` and `PdfGoToR` vectors still have empty
|
|
`raw_payload`. Capturing the `/D` (destination) entry for `/GoTo`
|
|
and the `/F` (file reference) entry for `/GoToR` would let the
|
|
scanner log where in-document navigation is actually pointing, but
|
|
it's an enhancement — not required for the false-positive fix.
|
|
|
|
3. **Phishing-keyword matching against fragments is still substring
|
|
based.** `#login-verify` triggers `SuspiciousKeyword` because
|
|
`"login"` and `"verify"` are both in `SUSPICIOUS_URL_KEYWORDS` and
|
|
matching is `lower.contains(kw)`. This is by design — the user
|
|
explicitly said in-document links should still be checked for other
|
|
flags. If substring matching proves too noisy on real documents,
|
|
the keyword matcher in `src/scanner/signatures.rs::match_suspicious_keyword`
|
|
can be tightened to word-boundary matching in a follow-up.
|