[MERGE #5206 @sethbrenith] SubString crash

Merge pull request #5206 from sethbrenith:user/sethb/string-uaf

I recently updated LiteralStringWithPropertyStringPtr to refer to the PropertyRecord's copy of the string buffer (just like PropertyString does), which can save some memory. However, if the script then takes a substring of that string, it would keep an invalid pointer as `originalFullStringReference`, which could allow the buffer to be freed.

The fix contains three parts:

1. Return the correct owning pointer from `LiteralStringWithPropertyStringPtr::GetOriginalStringReference`.
2. Add an assertion that would catch this bug eagerly, rather than only after a subsequent GC.
3. Update `SingleCharString::GetOriginalStringReference` to pass the new assertion. It was not a bug because single-char strings are pinned to a script context and copied upon marshaling, but this change makes its behavior more consistent with the other string types.

Fixes OS:17362430

Author: sethbrenith

lib/Runtime/Base/PropertyRecord.h

@@ -30,6 +30,10 @@ namespace Js
         friend class BuiltInPropertyRecords;
         friend class DOMBuiltInPropertyRecords;
 
+#if DBG
+        DEFINE_VTABLE_CTOR_NOBASE(PropertyRecord); // used for type assertions
+#endif
+
     private:
         Field(PropertyId) pid;
         //Made this mutable so that we can set it for Built-In js property records when we are adding it.

lib/Runtime/Library/ConcatString.cpp

@@ -200,6 +200,14 @@ namespace Js
         this->SetBuffer(propertyRecord->GetBuffer());
     }
 
+    void const * LiteralStringWithPropertyStringPtr::GetOriginalStringReference()
+    {
+        // If we have a property record, it's the string owner. Otherwise,
+        // the string is guaranteed to itself be an allocation block (as
+        // was asserted during the constructor).
+        return this->propertyRecord != nullptr ? static_cast<const void*>(this->propertyRecord) : this->GetString();
+    }
+
     RecyclableObject* LiteralStringWithPropertyStringPtr::CloneToScriptContext(ScriptContext* requestContext)
     {
         if (this->propertyRecord == nullptr)

lib/Runtime/Library/ConcatString.h

@@ -22,6 +22,7 @@ namespace Js
         void GetPropertyRecordImpl(_Out_ PropertyRecord const** propRecord, bool dontLookupFromDictionary = false);
         virtual void CachePropertyRecord(_In_ PropertyRecord const* propertyRecord) override;
         void CachePropertyRecordImpl(_In_ PropertyRecord const* propertyRecord);
+        virtual void const * GetOriginalStringReference() override;
 
         virtual RecyclableObject* CloneToScriptContext(ScriptContext* requestContext) override;
 

lib/Runtime/Library/JavascriptString.h

@@ -74,7 +74,7 @@ namespace Js
 
         static const char16* GetSzHelper(JavascriptString *str) { return str->GetSz(); }
         virtual const char16* GetSz();     // Get string, NULL terminated
-        virtual void const * GetOriginalStringReference();  // Get the original full string (Same as GetString() unless it is a SubString);
+        virtual void const * GetOriginalStringReference();  // Get the allocated object that owns the original full string buffer
 
 #if ENABLE_TTD
         //Get the associated property id for this string if there is on (e.g. it is a propertystring otherwise return Js::PropertyIds::_none)

lib/Runtime/Library/PropertyString.cpp

@@ -54,7 +54,7 @@ namespace Js
         return static_cast<PropertyString *>(aValue);
     }
 
-    void const * PropertyString::GetOriginalStringReference()
+    const void * PropertyString::GetOriginalStringReference()
     {
         // Property record is the allocation containing the string buffer
         return this->propertyRecordUsageCache.GetPropertyRecord();

lib/Runtime/Library/SingleCharString.cpp

@@ -34,4 +34,10 @@ namespace Js
         return Anew(arena, SingleCharString, ch,
             scriptContext->GetLibrary()->GetStringTypeStatic());
     }
+
+    void const * SingleCharString::GetOriginalStringReference()
+    {
+        // The owning allocation for the string buffer is the string itself
+        return this;
+    }
 }

lib/Runtime/Library/SingleCharString.h

@@ -16,6 +16,8 @@ namespace Js
         static SingleCharString* New(char16 ch, ScriptContext* scriptContext);
         static SingleCharString* New(char16 ch, ScriptContext* scriptContext, ArenaAllocator* arena);
 
+        virtual void const * GetOriginalStringReference() override;
+
     protected:
         DEFINE_VTABLE_CTOR(SingleCharString, JavascriptString);
 

lib/Runtime/Library/SubString.cpp

@@ -37,6 +37,13 @@ namespace Js
         const char16 * subString = string->GetString() + start;
         void const * originalFullStringReference = string->GetOriginalStringReference();
 
+#if SYSINFO_IMAGE_BASE_AVAILABLE
+        AssertMsg(AutoSystemInfo::IsJscriptModulePointer((void*)originalFullStringReference)
+            || recycler->IsValidObject((void*)originalFullStringReference)
+            || (VirtualTableInfo<PropertyRecord>::HasVirtualTable((void*)originalFullStringReference) && ((PropertyRecord*)originalFullStringReference)->IsBound()),
+            "Owning pointer for SubString must be static or GC pointer, or property record bound by thread allocator");
+#endif
+
         return RecyclerNew(recycler, SubString, originalFullStringReference, subString, length, scriptContext);
     }
 
@@ -69,7 +76,7 @@ namespace Js
         return UnsafeGetBuffer();
     }
 
-    const void * SubString::GetOriginalStringReference()
+    void const * SubString::GetOriginalStringReference()
     {
         if (originalFullStringReference != nullptr)
         {