Changeset 265097 in webkit

Timestamp:

Jul 30, 2020 2:44:45 PM (4 years ago)

Author:

sbarati@apple.com

Message:

Strip pointers instead of authing for byteOffset to not allow for a possible way to guess data pac
​https://bugs.webkit.org/show_bug.cgi?id=214952

Reviewed by Keith Miller.

In the old way of doing things, we would auth the vector pointer before subtracting
the base from it. Since we never validated the auth, this allowed for a
potential data-PAC bypass by just repeatedly calling byteOffset in a loop
and observing the integer result of the operation.

Since byteOffset does no loads/stores, it suffices to just strip the PAC
bits before doing the subtraction. This eliminates any such attacks like
the above because the PAC bits are ignored.

• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileGetTypedArrayByteOffset):

• ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::compileGetTypedArrayByteOffset):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGSpeculativeJIT.cpp (modified) (2 diffs)
• ftl/FTLLowerDFGToB3.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-07-30  Saam Barati  <sbarati@apple.com>
+
+        Strip pointers instead of authing for byteOffset to not allow for a possible way to guess data pac
+        https://bugs.webkit.org/show_bug.cgi?id=214952
+
+        Reviewed by Keith Miller.
+
+        In the old way of doing things, we would auth the vector pointer before subtracting
+        the base from it. Since we never validated the auth, this allowed for a
+        potential data-PAC bypass by just repeatedly calling byteOffset in a loop
+        and observing the integer result of the operation.
+        
+        Since byteOffset does no loads/stores, it suffices to just strip the PAC
+        bits before doing the subtraction. This eliminates any such attacks like
+        the above because the PAC bits are ignored.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileGetTypedArrayByteOffset):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileGetTypedArrayByteOffset):
+
 2020-07-29  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -7327 +7327 @@
 
     m_jit.loadPtr(MacroAssembler::Address(baseGPR, JSArrayBufferView::offsetOfVector()), vectorGPR);
-
-    JITCompiler::Jump nullVector = m_jit.branchPtr(JITCompiler::Equal, vectorGPR, TrustedImmPtr(JSArrayBufferView::nullVectorPtr()));
+#if CPU(ARM64E)
+    m_jit.removeArrayPtrTag(vectorGPR);
+#endif
 
     m_jit.loadPtr(MacroAssembler::Address(baseGPR, JSObject::butterflyOffset()), dataGPR);
     m_jit.cageWithoutUntagging(Gigacage::JSValue, dataGPR);
-
-    cageTypedArrayStorage(baseGPR, vectorGPR);
-
     m_jit.loadPtr(MacroAssembler::Address(dataGPR, Butterfly::offsetOfArrayBuffer()), arrayBufferGPR);
-    // FIXME: This needs caging.
-    // https://bugs.webkit.org/show_bug.cgi?id=175515
     m_jit.loadPtr(MacroAssembler::Address(arrayBufferGPR, ArrayBuffer::offsetOfData()), dataGPR);
 #if CPU(ARM64E)
@@ -7347 +7343 @@
     JITCompiler::Jump done = m_jit.jump();
     
-#if CPU(ARM64E)
-    nullVector.link(&m_jit);
-#endif
     emptyByteOffset.link(&m_jit);
     m_jit.move(TrustedImmPtr(nullptr), vectorGPR);
     
     done.link(&m_jit);
-#if !CPU(ARM64E)
-    ASSERT(!JSArrayBufferView::nullVectorPtr());
-    nullVector.link(&m_jit);
-#endif
 
     strictInt32Result(vectorGPR, node);

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -4362 +4362 @@
 
         LBasicBlock wastefulCase = m_out.newBlock();
-        LBasicBlock notNull = m_out.newBlock();
         LBasicBlock continuation = m_out.newBlock();
         
-        ValueFromBlock nullVectorOut = m_out.anchor(m_out.constIntPtr(0));
+        ValueFromBlock nonWastefulResult = m_out.anchor(m_out.constIntPtr(0));
 
         LValue mode = m_out.load32(basePtr, m_heaps.JSArrayBufferView_mode);
@@ -4372 +4371 @@
             unsure(continuation), unsure(wastefulCase));
 
-        LBasicBlock lastNext = m_out.appendTo(wastefulCase, notNull);
-
-        LValue vector = m_out.loadPtr(basePtr, m_heaps.JSArrayBufferView_vector);
-        m_out.branch(m_out.equal(vector, m_out.constIntPtr(JSArrayBufferView::nullVectorPtr())), 
-            unsure(continuation), unsure(notNull));
-
-        m_out.appendTo(notNull, continuation);
+        LBasicBlock lastNext = m_out.appendTo(wastefulCase, continuation);
+
+        LValue vectorPtr = m_out.loadPtr(basePtr, m_heaps.JSArrayBufferView_vector);
+        vectorPtr = removeArrayPtrTag(vectorPtr);
 
         LValue butterflyPtr = caged(Gigacage::JSValue, m_out.loadPtr(basePtr, m_heaps.JSObject_butterfly), basePtr);
         LValue arrayBufferPtr = m_out.loadPtr(butterflyPtr, m_heaps.Butterfly_arrayBuffer);
 
-        LValue vectorPtr = caged(Gigacage::Primitive, vector, basePtr);
-
-        // FIXME: This needs caging.
-        // https://bugs.webkit.org/show_bug.cgi?id=175515
         LValue dataPtr = m_out.loadPtr(arrayBufferPtr, m_heaps.ArrayBuffer_data);
         dataPtr = removeArrayPtrTag(dataPtr);
@@ -4395 +4387 @@
         m_out.appendTo(continuation, lastNext);
 
-        setInt32(m_out.castToInt32(m_out.phi(pointerType(), nullVectorOut, wastefulOut)));
+        setInt32(m_out.castToInt32(m_out.phi(pointerType(), nonWastefulResult, wastefulOut)));
     }