Race condition in JSXPathResult::visitAdditionalChildren during GC

e8db86e

XPath의 node set 처리에서 발생하는 concurrent GC UAF가 실제로 확인된 취약점입니다.

패치 이전에는 GC thread가 m_value.toNodeSet()을 직접 순회하는 도중, main thread에서 XPathNodeList::sort가 호출될 수 있었습니다. XPathNodeList::firstNode 등을 통해 이 경로에 진입하면 내부 vector가 재할당되고, GC thread의 iterator가 무효화됩니다.

이 문제는 오래전부터 잠재적 위험으로 인식되고 있었습니다. 당시의 FIXME 주석에는 "This looks like it might race, but I'm not sure"라는 내용이 남아 있었는데, 배포된 코드 안에 잔존하던 known-unknown이었습니다.

Source/WebCore/xml/XPathResult.h

-    XPath::NodeSet m_nodeSet; // FIXME: m_value에 저장된 node set을 왜 여기서도 중복으로 갖고 있는가?
+    Lock m_nodeSetLock;
+    XPath::NodeSet m_nodeSet WTF_GUARDED_BY_LOCK(m_nodeSetLock);

Source/WebCore/xml/XPathResult.cpp

+template<typename Visitor>
+void XPathResult::visitAdditionalChildrenInGCThread(Visitor& visitor)
+{
+    Locker locker { m_nodeSetLock };
+    for (auto& node : m_nodeSet)
+        addWebCoreOpaqueRoot(visitor, node.get());
+}

Significance

Concurrent GC UAF는 browser engine에서 exploit 가치가 가장 높은 취약점 클래스 중 하나입니다. GC thread가 상시 실행 중인 만큼, JS 측에서 timing을 정밀하게 맞출 필요가 없습니다.

Audit directions

diff에서 확인할 수 있는 snapshotItem()은 잠긴 m_nodeSet이 아닌 m_value.toNodeSet()에서 직접 읽어오기 때문에, 새로 도입된 lock의 보호 범위 밖에 있습니다. main thread에서 convertTo(ORDERED_NODE_SNAPSHOT_TYPE)가 GC-triggered sort와 동시에 실행되거나, snapshotItem()이 순회 중인 상태에서 m_value의 node set이 변경되면, 동일한 race 클래스가 그대로 남아 있게 됩니다.

destructor는 m_nodeSet과 m_value.toNodeSet()이 동일한 노드를 포함한다고 단언(assert)합니다. convertTo()는 다양한 type transition을 처리하는 함수인 만큼, m_nodeSet을 갱신하지 않은 채 m_value만 변경하는 경로가 있다면 이 invariant가 깨집니다.

m_nodeSetLock은 non-recursive Lock입니다. lock을 보유한 상태에서 RefPtr destructor를 통해 GC mark가 유발되는 경로가 있다면, 동일한 lock을 획득하려는 GC thread와 deadlock이 발생할 수 있습니다.

이번 fix보다 먼저 작성된 FIXME 주석이 존재했던 만큼, WebKit 전체에서 visitAdditionalChildrenInGCThread를 구현한 다른 위치를 검색해야 합니다. main thread에서도 변경되는 collection을 순회하는 구현이 있다면, 동일한 race 패턴이 존재할 가능성도 배제하기 어렵습니다.