diff --git a/work/tech-design/opaque-url-fragment-detection.md b/work/tech-design/opaque-url-fragment-detection.md index 56944000..dd53d168 100644 --- a/work/tech-design/opaque-url-fragment-detection.md +++ b/work/tech-design/opaque-url-fragment-detection.md @@ -8,7 +8,8 @@ tags: - webkit - url - crash -status: draft +status: in-progress +chosen-approach: '_web_originalDataAsString via URLComponents(webKitUrl:)' asana: >- https://app.asana.com/1/137249556945/project/1202406491309510/task/1215403820548743 --- @@ -18,216 +19,175 @@ asana: >- --- -## Background & Requirements +## Background -The DuckDuckGo browser uses `WKWebView` for all page rendering. Every navigation goes through the `decidePolicyFor` delegate chain managed by `DistributedNavigationDelegate` (BrowserServicesKit). A central piece of that chain is classifying each `WKNavigationAction` as a **same-document navigation** (fragment-only change within the current page, e.g. `#section`) via: +Every navigation through `WKWebView` goes through `DistributedNavigationDelegate`, which classifies each `WKNavigationAction` as a **same-document navigation** (fragment-only change, e.g. `#section`) via `isSameDocumentNavigation`. Misclassification corrupts the navigation state machine. -```swift -// WKNavigationActionExtension.swift:184 -public var isSameDocumentNavigation: Bool { - switch navigationType { - case .linkActivated, .other: - return !isRedirect && newURL.hasFragment && currentURL.equals(newURL, by: .sameDocument) - case .backForward: - return (newURL.hasFragment || currentURL.hasFragment) && currentURL.equals(newURL, by: .sameDocument) - ... - } -} -``` +`isSameDocumentNavigation` relies on `hasFragment` and `.sameDocument` URL equality, both of which must correctly extract the fragment — which is where the bug lives. -`isSameDocumentNavigation` drives the `DistributedNavigationDelegate` state machine: same-document navigations reuse the existing `Navigation` object and trigger client-redirect handling rather than starting a fresh navigation, while non-same-document navigations allocate a new `Navigation`. Misclassification corrupts navigation state. +### Two URL parsers in WebKit -`hasFragment` and the `.sameDocument` equality both depend on extracting the URL fragment—which is where the bug lives. +| Parser | Used where | Fragment in opaque URLs (`data:`, `about:`, `blob:`) | +|--------|-----------|------------------------------------------------------| +| **WTF::URLParser** | WebKit internals, navigation decisions | ✅ Correctly split at `#` | +| **NSURL / CFURL** | Foundation APIs, Swift `URL` | ❌ Encodes `#` as `%23`; `.fragment` returns `nil` | -### Two URL parsers coexist in WebKit on Apple platforms - -| Parser | Used where | Standard | Fragment in opaque URLs (`data:`, `about:`, `blob:`) | -|--------|-----------|----------|------------------------------------------------------| -| **WTF::URLParser** | WebKit rendering, WKWebView navigation decisions | WHATWG URL | ✅ Correctly split at `#` | -| **NSURL / CFURL** | Foundation APIs, Swift `URL` | Old RFC 1808/2396 | ❌ Encodes `#` as `%23`, returns `nil` for `.fragment` | - -When WebKit bridges its internal `WTF::URL` to Objective-C it calls: +When WebKit bridges a `WTF::URL` to Objective-C it calls: ```c CFURLCreateAbsoluteURLWithBytes(alloc, rawBytes, len, kCFStringEncodingUTF8, nil, true) ``` -The raw bytes at this point still contain the literal `#`. CFURL re-parses them with its old-RFC parser, which does **not** recognise `#` as a fragment delimiter in opaque-scheme URLs. It encodes `#` → `%23` and stores the whole thing as the opaque path. - -Result for `data:text/html,hello#anchor`: +The raw bytes contain the literal `#`, but CFURL's old-RFC parser does not recognise `#` as a fragment delimiter in opaque-scheme URLs — it encodes it as `%23` and stores the whole thing as the opaque path. Result for `data:text/html,hello#anchor`: | Property | WTF::URL | NSURL / Swift URL | |----------|----------|-------------------| -| `.path` | `text/html,hello` | `""` (empty) | -| `.query` | nil | nil | -| `.fragment` | `"anchor"` | nil | +| `.fragment` | `"anchor"` | `nil` | | `.absoluteString` | `data:text/html,hello#anchor` | `data:text/html,hello%23anchor` | --- -## Problem Statement +## Problem -A pathological URL—a multi-MB `data:` URL or attacker-crafted href—reaches `WKNavigationAction.isSameDocumentNavigation` on the main thread. The call chain: - -1. `WebKit::WebPageProxy::decidePolicyForNavigationAction` dispatches to the main-thread delegate. -2. `DistributedNavigationDelegate` reads `wkNavigationAction.isSameDocumentNavigation`. -3. `isSameDocumentNavigation` calls `newURL.absoluteString.hashedSuffix` (and for `.backForward`, also `currentURL.absoluteString.hashedSuffix`). -4. `hashedSuffix` → `hashedSuffixRange` → `String.firstIndex(of: "#")` (`StringExtension.swift:67`) iterates grapheme-by-grapheme over `absoluteString`. -5. For a multi-MB `data:` URL this is O(n) UTF-8 grapheme decoding on the **main thread**, per navigation decision. -6. Main thread blocked >10 s under thermal-state `serious` at 100% CPU → RunningBoard scene-update watchdog kills the app: **SIGKILL, terminationReason `0x8BADF00D`**. +A multi-MB `data:` URL reaches `isSameDocumentNavigation` on the main thread. The old `hashedSuffix` implementation called `String.firstIndex(of: "#")`, iterating grapheme-by-grapheme — O(n) per navigation decision on the **main thread**. For a large enough URL this blocks the thread >10 s → RunningBoard watchdog SIGKILL (`0x8BADF00D`). The fix must: -1. Make `hashedSuffixRange` / `isSameDocumentNavigation` fast for arbitrarily large URLs (no O(n) grapheme scan). +1. Make fragment detection fast (no O(n) grapheme scan). 2. Correctly detect fragments in opaque URLs where NSURL has already encoded `#` as `%23`. --- -## Why Standard APIs Can't Help: The Information-Loss Chain +## Information-Loss Chain -Fragment information is **irreversibly lost** between WebKit's internal representation and what Swift code receives in `decidePolicyFor`: +Fragment information is irreversibly lost between WebKit and Swift: ``` -WTF::URL (WHATWG-compliant; m_string = "data:…#anchor", m_queryEnd before '#') - ↓ URL::createNSURL() → createCFURL() - ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") ← '#' still visible here - ↓ CFURL re-parses with old-RFC parser; encodes '#' → '%23' -NSURL (.absoluteString = "…%23anchor", .fragment = nil, .path = "") - ↓ [NSMutableURLRequest initWithURL: url().createNSURL()] (ResourceRequestCocoa.mm) -NSURLRequest (same NSURL) +WTF::URL (m_string = "data:…#anchor") + ↓ createCFURL() + ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") ← '#' visible here + ↓ CFURL re-parses; '#' → '%23' +NSURL (.absoluteString = "…%23anchor", .fragment = nil) ↓ WKNavigationAction.request.URL ← what Swift sees -WKNavigationAction._originalURL ← same path, same loss (createNSURL() again) ``` -Every Foundation/Swift API is downstream of the encoding step: - -- **`URL.fragment` / `.path` / `.query`** — return `nil` / `""` for all opaque URLs. -- **`URLComponents(url:)` / `(string:)` and `NSURLComponents` equivalents** — either use NSURL's already-broken cached parse tree, or re-parse `absoluteString` where `#` is already `%23`. RFC 3986 treats `%23` as a literal character, not a delimiter. Fragment stays nil in all four variants. -- **`WKNavigationActionPrivate.h`** (`_originalURL`, `_isRedirect`, `_hitTestResult`, …) — all URL-returning properties go through `WTF::URL::createNSURL()` → same `createCFURL()` path. No private API exposes the raw WTF string. - -There is **no Foundation or WebKit API**, public or private, that recovers the original `#` once the NSURL has been constructed. +Every downstream Foundation API (`URL.fragment`, `URLComponents`, `NSURLComponents`) operates on the already-broken NSURL — they all return `nil` for the fragment. No public or private WebKit API exposes the raw `WTF::URL` string … except one (see Chosen Approach). --- -## Approaches Tried +## Approaches Considered -### 1. Scan `absoluteString` for `%23` +### 1. Scan `absoluteString` for `%23` ❌ -Since NSURL encodes the original `#` as `%23` in `absoluteString`, scanning for `%23` as a fragment delimiter recovers the fragment in many cases. Implemented and partly working. +Since NSURL encodes `#` as `%23`, scanning for it recovers the fragment in many cases. Partly working, but fundamentally ambiguous: `%23` is also valid payload in `data:` URLs (SVG fill colors, anchor hrefs, CSS). The two cases produce identical `absoluteString` and cannot be distinguished from the string alone. A size guard avoids scanning large payloads but causes incorrect equality results. -**Fundamental problem:** `%23` is also valid payload data in `data:` URLs: -- SVG fill colors: `data:image/svg+xml,` — `%23` is `#` in the SVG -- HTML anchor hrefs: `data:text/html,` -- CSS: `data:text/css,color:%23333` +### 2. `CFURLCreateAbsoluteURLWithBytes` GOT-patch hook -Both cases produce identical `absoluteString`. They are **indistinguishable from the string alone**. A 4 KB size guard was added to avoid scanning large `data:` URLs, but that makes two different large data: URLs compare as equal — semantically wrong. +WebKit calls `CFURLCreateAbsoluteURLWithBytes()` at the exact moment it converts a `WTF::URL` to a `CFURL`. The raw byte buffer at that point still contains the literal `#`. The hook intercepts this call using **runtime GOT patching** (same technique as `CxaThrowSwapper` / fishhook) — not a link-time `__DATA,__interpose` section, which would not reach WebKit's internal call from `WebKit.framework` into `CoreFoundation.framework`. -### 2. Private API / `WKNavigationAction._originalURL` +Mechanism: +1. `_dyld_register_func_for_add_image` registers a callback that fires for every Mach-O image loaded into the process (including already-loaded images at registration time). +2. The callback walks the image's **indirect symbol table** (`ImageMap.rebindSymbol`) targeting only the `WebKit` dylib, finds its GOT entry for `CFURLCreateAbsoluteURLWithBytes`, and replaces it with the hook pointer. ARM64e pointer authentication is handled via `BSKStripFunctionPointer` / `BSKSignFunctionPointer`. +3. The hook calls the original via the saved pointer, then scans the raw byte buffer with `memchr` (SIMD-vectorised on ARM64/x86) for `':'` (opaque check) then `'#'`, and stores the resulting `NSRange` as an associated object on the `CFURL` result. +4. `URL.opaqueFragmentAnnotation` reads/writes that associated object; `URL.isOpaqueFragmentScanned` guards against double-scanning the same instance. +5. `uninstallCFURLSwapper()` walks all loaded images and restores the saved original pointer. -See information-loss chain above. No private API bypasses it; all paths go through `createCFURL()`. +```swift +// install: register dyld callback → patch WebKit's GOT entry at runtime +_dyld_register_func_for_add_image(_cfURLProcessMachHeader) ---- - -## Recommended Approach: `CFURLCreateAbsoluteURLWithBytes` Interpose - -### Key Insight - -WebKit calls `CFURLCreateAbsoluteURLWithBytes()` at the **exact** moment it converts a `WTF::URL` to a `CFURL`/`NSURL`. The raw byte buffer at that call site still contains the literal `#`, before CFURL encodes it. Intercepting this C function gives unambiguous access to the original bytes. - -### Mechanism: `DYLD_INTERPOSE` - -The `__DATA,__interpose` Mach-O section replaces any C function in the same dylib at load time, without method swizzling or private SPI: - -```c -#define DYLD_INTERPOSE(_replacement, _replacee) \ - __attribute__((used)) \ - static struct { const void *replacement; const void *replacee; } \ - _interpose_##_replacee \ - __attribute__((section("__DATA,__interpose"))) = \ - { (const void *)&(_replacement), (const void *)&(_replacee) } -``` - -### Implementation - -Files: -- `CFURLCreateAbsoluteURLWithBytesInterpose.m` — DEBUG-only (`#ifdef DEBUG`) -- `NSURL+FragmentByteRange.h` / `.m` — category exposing the stored range - -**Interpose logic:** - -1. Call the real `CFURLCreateAbsoluteURLWithBytes` to get the `CFURLRef`. -2. For opaque URLs (scheme present, no `//` authority), scan the raw byte buffer for `'#'` (byte `0x23`) with a simple C loop—O(n) but done once at NSURL construction, not per navigation decision. -3. If found, store the byte range as an `NSValue` associated object on the bridged `NSURL` via `objc_setAssociatedObject`. -4. Callers read `NSURL.ddg_fragmentByteRange` for the unambiguous `#` position. - -```objc -if (relativeURLBytes[i] == '#') { - NSRange fragmentRange = NSMakeRange(i, length - i); - NSURL *nsurl = (__bridge NSURL *)result; - objc_setAssociatedObject(nsurl, &kFragmentByteRangeKey, - [NSValue valueWithRange:fragmentRange], - OBJC_ASSOCIATION_RETAIN_NONATOMIC); +// hook: memchr scan + associated-object annotation +if let hashRaw = memchr(searchBase, Int32(UInt8(ascii: "#")), searchLen) { + url.opaqueFragmentAnnotation = NSRange(location: hashOff, length: len - hashOff) } ``` -### Why This Is Correct +**Downside:** couples to WebKit's dylib name and a private CFURL symbol; requires bypassing page protection to write into a read-only GOT page; depends on dyld image-load ordering; massive and fragile. -- Bytes are scanned **before** CFURL encodes `#` → `%23` — no ambiguity with payload `%23`. -- Associated object is set on the **exact NSURL instance** for that WTF::URL. -- `ddg_fragmentByteRange.location` matches WTF::URL's internal `m_queryEnd + 1`. -- Works for all opaque schemes: `data:`, `about:`, `blob:`, `javascript:`. -- DEBUG-only: zero production overhead; interpose section absent from release builds. +### 2a. `__DATA,__interpose` (static link-time interpose) ⚠️ (option, not chosen) -### Complete Call Flow with Interpose +A related but distinct mechanism: a `__DATA,__interpose` section in the binary redirects calls to the named C function to a replacement at load time, before any code runs. Simpler than GOT patching (no dyld callback, no image walk), and dyld applies it process-wide so it does intercept calls from other frameworks. -``` -WTF::URL (m_string = "data:…#anchor", m_queryEnd before '#') - ↓ URL::createCFURL() - ↓ CFURLCreateAbsoluteURLWithBytes(rawBytes = "…#anchor") - ↑↑ INTERPOSED: scan rawBytes → store NSRange on result NSURL - ↓ NSURL (.absoluteString = "…%23anchor", .fragment = nil) - ↑ .ddg_fragmentByteRange → {location=N} — unambiguous '#' position -Swift URLExtension.opaqueComponents: - DEBUG → use ddg_fragmentByteRange (exact) - release → NSString.range(of: "#") fallback (unambiguous for non-data: schemes) -``` +**Downside:** the `__DATA,__interpose` section is a well-known jailbreak / hooking fingerprint; App Store review tooling flags it, making approval unlikely. --- -## Notes +## Chosen Approach: Private WebKit URL String (`_web_originalDataAsString`) -[1] The `%23` ambiguity only affects `data:` or `about:` URLs. For `blob:`, `javascript:` the opaque path never legitimately contains `%23` as encoded payload. +NSURL instances that originate from WebKit (via `createNSURL()`) carry a private property `_web_originalDataAsString` which holds the original `WTF::URL` byte string — **before** CFURL re-encoded `#` as `%23`. Parsing that string with Swift's native `URLComponents(string:)` gives correct path/query/fragment decomposition for all opaque URLs. -[2] The primary safety fix is the O(n) crash: replace `String.firstIndex(of: "#")` with a byte-level scan and short-circuit on `data:` scheme or length threshold. The interpose is the accuracy improvement layered on top for DEBUG validation. +```swift +// URLComponentsExtension.swift +extension URLComponents { + /// Parses a WebKit-origin URL using the original WTF::URL string. + /// Falls back to URLComponents(url:) for non-WebKit or hierarchical URLs. + init?(webKitUrl: URL) { + guard webKitUrl.isOpaque, + let originalString = (webKitUrl as NSURL).value(forKey: "_web_originalDataAsString") as? String, + let swiftNativeURLComponents = URLComponents(string: originalString) else { + self.init(url: webKitUrl, resolvingAgainstBaseURL: false) + return + } + self = swiftNativeURLComponents + } +} +``` -[3] WebKit bug for NSURL not handling `#` in opaque URLs: [bugs.webkit.org/68089](https://bugs.webkit.org/show_bug.cgi?id=68089). Chromium fixed this in M72 (2019). WebKit's WTF::URLParser is correct; only the Foundation/CFURL bridge is broken. +```swift +// URLExtension.swift +public var originalWebKitString: String? { + (self as NSURL).value(forKey: "_web_originalDataAsString") as? String +} + +public var hasFragment: Bool { + guard let components = URLComponents(webKitUrl: self), + let fragment = components.fragment else { return false } + return !fragment.isEmpty +} + +public func equals(_ other: URL, by components: EqualityComponents) -> Bool { + guard let selfParsed = URLComponents(webKitUrl: self), + let otherParsed = URLComponents(webKitUrl: other) else { return false } + // component-by-component comparison … +} +``` + +### Why this is correct + +- `_web_originalDataAsString` holds the `WTF::URL`'s `m_string` verbatim — `#` is still a literal character, not `%23`. +- Swift's `URLComponents(string:)` is WHATWG-aligned and correctly splits at `#`, so `.fragment` returns the real fragment. +- No ambiguity with `%23` payload: we parse the original string, not the NSURL's re-encoded `absoluteString`. +- The property is absent on non-WebKit URLs (`URL(string:)`-created values) — the initialiser falls back to `URLComponents(url:)`, which is correct for hierarchical URLs. +- Pure Swift/Obj-C KVC access; no interpose, no associated objects, no extra files. + +### Call flow + +``` +WTF::URL (m_string = "data:…#anchor") + ↓ createNSURL() +NSURL (.absoluteString = "…%23anchor", ._web_originalDataAsString = "data:…#anchor") + ↓ +URLComponents(webKitUrl: url) + → value(forKey: "_web_originalDataAsString") → "data:…#anchor" + → URLComponents(string: "data:…#anchor") + → .fragment = "anchor" ✅ +``` --- ## Testing - **Unit tests** (`URLExtensionTests.swift`): - - `equals(by: .sameDocument)` and `equals(by: .fuzzyIdentity)` on opaque URLs with fragment: `about:blank#section`, `data:text/html,…#anchor` - - Opaque URLs with query AND fragment: `data:text/html,…?key=value#anchor` - - Large (20 MB) `data:` URL performance test: comparison must complete in < 0.1 s - - Diagnostic timing test (`tempOpaqueNSURLComponentTimings`) printing per-operation durations for all Foundation URL APIs on a 20 MB `data:` URL with query and fragment - -- **Manual test** (DEBUG build): verify `ddg_fragmentByteRange` is set on `WKNavigationAction.request.URL` when navigating to a `data:` URL with a `#` fragment via `WKWebView`. - -- **Regression**: navigation tests in `DistributedNavigationDelegateTests.swift` and `NavigationTestHelpers.swift` updated to use `.equals(by: .fuzzyIdentity)` in place of removed `.matches()` API. + - `hasFragment` and `equals(by: .sameDocument / .fuzzyIdentity)` on `about:blank#section`, `data:text/html,…#anchor`, opaque URLs with query AND fragment. + - Large (20 MB) `data:` URL: comparison must complete in < 0.1 s. + - Diagnostic timing test (`tempOpaqueNSURLComponentTimings`) for all Foundation URL APIs on a 20 MB `data:` URL. +- **Regression**: navigation tests in `DistributedNavigationDelegateTests.swift` updated to use `.equals(by: .fuzzyIdentity)` in place of the removed `.matches()` API. --- -## Additional Considerations +## Notes -### Privacy -- No user data collected or transmitted. All processing is local and in-process. - -### Security -- No extra data is stored or passed anywhere -- The approach injects into the existing API and stores a helper pointer -### Site Breakage -- The O(n) fix prevents crashes on large URLs. Fragment-detection correctness improves navigation state machine accuracy but does not change whether a navigation is allowed or blocked. - -### Operational -- No new infrastructure, network calls, or persisted state. The crash was a SIGKILL watchdog kill visible in Sentry; the fix eliminates the `0x8BADF00D` terminations for this call chain. +- **WebKit bug:** [bugs.webkit.org/68089](https://bugs.webkit.org/show_bug.cgi?id=68089). Chromium fixed `#` handling in M72 (2019). `WTF::URLParser` is correct; only the CFURL bridge is broken. +- **Primary safety fix** is the O(n) crash: `isSameDocumentNavigation` now uses `URLComponents(webKitUrl:)` which is constant-time relative to URL length (KVC + a single string parse), replacing the old grapheme-scan loop. +- **`%23` ambiguity** only affects `data:` / `about:`. For `blob:` and `javascript:` the opaque path never legitimately contains `%23` as encoded payload. +- No user data collected or transmitted; all processing is local and in-process.