Changeset 93891 in vbox

Timestamp:

Feb 22, 2022 6:08:39 PM (3 years ago)

Author:

vboxsync

Message:

Main: Guest Properties: improved property name and value validation, bugref:10185.

This commit also prevents guest properties loss if they were set while VM was running.

Location:

trunk

Files:

• include/VBox/HostServices/GuestPropertySvc.h (modified) (1 diff)
• src/VBox/HostServices/GuestProperties/VBoxGuestPropSvc.cpp (modified) (7 diffs)
• src/VBox/Main/idl/VirtualBox.xidl (modified) (1 diff)
• src/VBox/Main/include/MachineImpl.h (modified) (2 diffs)
• src/VBox/Main/src-client/ConsoleImpl.cpp (modified) (1 diff)
• src/VBox/Main/src-server/MachineImpl.cpp (modified) (8 diffs)

trunk/include/VBox/HostServices/GuestPropertySvc.h

@@ -66 +66 @@
 #define GUEST_PROP_F_ALLFLAGS         (GUEST_PROP_F_TRANSIENT | GUEST_PROP_F_READONLY | GUEST_PROP_F_TRANSRESET)
 /** @} */
+
+/**
+ * Check that a string fits our criteria for a property name.
+ *
+ * @returns IPRT status code
+ * @param   pszName   the string to check, must be valid Utf8
+ * @param   cbName    the number of bytes @a pszName points to, including the terminating character.
+ */
+DECLINLINE(int) GuestPropValidateName(const char *pszName, uint32_t cbName)
+{
+    /* Property name is expected to be at least 1 charecter long plus terminating character. */
+    AssertReturn(cbName >= 2, VERR_INVALID_PARAMETER);
+    AssertReturn(cbName < GUEST_PROP_MAX_NAME_LEN, VERR_INVALID_PARAMETER);
+
+    AssertPtrReturn(pszName, VERR_INVALID_POINTER);
+
+    AssertReturn(memchr(pszName, '*', cbName) == NULL, VERR_INVALID_PARAMETER);
+    AssertReturn(memchr(pszName, '?', cbName) == NULL, VERR_INVALID_PARAMETER);
+    AssertReturn(memchr(pszName, '|', cbName) == NULL, VERR_INVALID_PARAMETER);
+
+    return VINF_SUCCESS;
+}
+
+/**
+ * Check a string fits our criteria for the value of a guest property.
+ *
+ * @returns IPRT status code
+ * @param   pszValue  the string to check, must be valid Utf8
+ * @param   cbValue   the length in bytes of @a pszValue, including the
+ *                    terminator
+ * @thread  HGCM
+ */
+DECLINLINE(int) GuestPropValidateValue(const char *pszValue, uint32_t cbValue)
+{
+    AssertPtrReturn(pszValue, VERR_INVALID_POINTER);
+
+    /* Zero-length values are possible, however buffer should contain terminating character at least. */
+    AssertReturn(cbValue > 0, VERR_INVALID_PARAMETER);
+    AssertReturn(cbValue < GUEST_PROP_MAX_VALUE_LEN, VERR_INVALID_PARAMETER);
+
+    return VINF_SUCCESS;
+}
 
 /**

trunk/src/VBox/HostServices/GuestProperties/VBoxGuestPropSvc.cpp

@@ -416 +416 @@
     static DECLCALLBACK(int) reqThreadFn(RTTHREAD ThreadSelf, void *pvUser);
     uint64_t getCurrentTimestamp(void);
-    int validateName(const char *pszName, uint32_t cbName);
-    int validateValue(const char *pszValue, uint32_t cbValue);
     int setPropertyBlock(uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
     int getProperty(uint32_t cParms, VBOXHGCMSVCPARM paParms[]);
@@ -471 +469 @@
     this->mPrevTimestamp = u64NanoTS;
     return u64NanoTS;
-}
-
-/**
- * Check that a string fits our criteria for a property name.
- *
- * @returns IPRT status code
- * @param   pszName   the string to check, must be valid Utf8
- * @param   cbName    the number of bytes @a pszName points to, including the
- *                    terminating '\0'
- * @thread  HGCM
- */
-int Service::validateName(const char *pszName, uint32_t cbName)
-{
-    LogFlowFunc(("cbName=%d\n", cbName));
-    int rc = VINF_SUCCESS;
-    if (RT_SUCCESS(rc) && (cbName < 2))
-        rc = VERR_INVALID_PARAMETER;
-    for (unsigned i = 0; RT_SUCCESS(rc) && i < cbName; ++i)
-        if (pszName[i] == '*' || pszName[i] == '?' || pszName[i] == '|')
-            rc = VERR_INVALID_PARAMETER;
-    LogFlowFunc(("returning %Rrc\n", rc));
-    return rc;
-}
-
-
-/**
- * Check a string fits our criteria for the value of a guest property.
- *
- * @returns IPRT status code
- * @param   pszValue  the string to check, must be valid Utf8
- * @param   cbValue   the length in bytes of @a pszValue, including the
- *                    terminator
- * @thread  HGCM
- */
-int Service::validateValue(const char *pszValue, uint32_t cbValue)
-{
-    LogFlowFunc(("cbValue=%d\n", cbValue)); RT_NOREF1(pszValue);
-
-    int rc = VINF_SUCCESS;
-    if (RT_SUCCESS(rc) && cbValue == 0)
-        rc = VERR_INVALID_PARAMETER;
-    if (RT_SUCCESS(rc))
-        LogFlow(("    pszValue=%s\n", cbValue > 0 ? pszValue : NULL));
-    LogFlowFunc(("returning %Rrc\n", rc));
-    return rc;
 }
 
@@ -644 +597 @@
         rc = VERR_INVALID_PARAMETER;
     else
-        rc = validateName(pcszName, cbName);
+        rc = GuestPropValidateName(pcszName, cbName);
     if (RT_FAILURE(rc))
     {
@@ -736 +689 @@
      */
     if (RT_SUCCESS(rc))
-        rc = validateName(pcszName, cchName);
+        rc = GuestPropValidateName(pcszName, cchName);
     if (RT_SUCCESS(rc))
-        rc = validateValue(pcszValue, cchValue);
+        rc = GuestPropValidateValue(pcszValue, cchValue);
     if ((3 == cParms) && RT_SUCCESS(rc))
         rc = RTStrValidateEncodingEx(pcszFlags, cchFlags,
@@ -869 +822 @@
         && RT_SUCCESS(HGCMSvcGetCStr(&paParms[0], &pcszName, &cbName))  /* name */
        )
-        rc = validateName(pcszName, cbName);
+        rc = GuestPropValidateName(pcszName, cbName);
     else
         rc = VERR_INVALID_PARAMETER;
@@ -1342 +1295 @@
         {
             /* Send out a host notification */
-            rc = notifyHost(pszProperty, "", nsTimestamp, "");
+            rc = notifyHost(pszProperty, NULL, nsTimestamp, "");
         }
     }
@@ -1388 +1341 @@
         *pu8++ = 0;
 
-        pHostCallbackData->pcszValue    = (const char *)pu8;
+        /* NULL value means property was deleted. */
+        pHostCallbackData->pcszValue    = pszValue ? (const char *)pu8 : NULL;
         memcpy(pu8, pszValue, cbValue);
         pu8 += cbValue;

trunk/src/VBox/Main/idl/VirtualBox.xidl

@@ -5320 +5320 @@
         </desc>
       </param>
+      <param name="fWasDeleted" type="boolean" dir="in">
+        <desc>
+          The flag which indicates if property was deleted.
+        </desc>
+      </param>
     </method>
 

trunk/src/VBox/Main/include/MachineImpl.h

@@ -1234 +1234 @@
                               const com::Utf8Str &aValue,
                               LONG64 aTimestamp,
-                              const com::Utf8Str &aFlags);
+                              const com::Utf8Str &aFlags,
+                              BOOL fWasDeleted);
     HRESULT lockMedia();
     HRESULT unlockMedia();
@@ -1390 +1391 @@
                               const com::Utf8Str &aValue,
                               LONG64 aTimestamp,
-                              const com::Utf8Str &aFlags);
+                              const com::Utf8Str &aFlags,
+                              BOOL fWasDeleted);
     HRESULT lockMedia();
     HRESULT unlockMedia();

trunk/src/VBox/Main/src-client/ConsoleImpl.cpp

@@ -1849 +1849 @@
                                                         value.raw(),
                                                         pCBData->u64Timestamp,
-                                                        flags.raw());
+                                                        flags.raw(),
+                                                        !pCBData->pcszValue);
     if (SUCCEEDED(hrc))
     {

trunk/src/VBox/Main/src-server/MachineImpl.cpp

@@ -5655 +5655 @@
     ReturnComNotImplemented();
 #else // VBOX_WITH_GUEST_PROPS
+    AssertReturn(RT_SUCCESS(GuestPropValidateName(aProperty.c_str(), (uint32_t)aProperty.length() + 1 /* '\0' */)), E_INVALIDARG);
+    AssertReturn(RT_SUCCESS(GuestPropValidateValue(aValue.c_str(), (uint32_t)aValue.length() + 1  /* '\0' */)), E_INVALIDARG);
+
     HRESULT rc = i_setGuestPropertyToVM(aProperty, aValue, aFlags, /* fDelete = */ false);
     if (rc == E_ACCESSDENIED)
@@ -5737 +5740 @@
         GuestPropWriteFlags(it->second.mFlags, szFlags);
         aFlags[i] = Utf8Str(szFlags);
+
+        AssertReturn(RT_SUCCESS(GuestPropValidateName(aNames[i].c_str(), (uint32_t)aNames[i].length() + 1 /* '\0' */)), E_INVALIDARG);
+        AssertReturn(RT_SUCCESS(GuestPropValidateValue(aValues[i].c_str(), (uint32_t)aValues[i].length() + 1 /* '\0' */)), E_INVALIDARG);
     }
 
@@ -13572 +13578 @@
         else
             aFlags[i] = "";
+
+        AssertReturn(RT_SUCCESS(GuestPropValidateName(aNames[i].c_str(), (uint32_t)aNames[i].length() + 1 /* '\0' */)), E_INVALIDARG);
+        AssertReturn(RT_SUCCESS(GuestPropValidateValue(aValues[i].c_str(), (uint32_t)aValues[i].length() + 1 /* '\0' */)), E_INVALIDARG);
     }
     return S_OK;
@@ -13582 +13591 @@
                                           const com::Utf8Str &aValue,
                                           LONG64 aTimestamp,
-                                          const com::Utf8Str &aFlags)
+                                          const com::Utf8Str &aFlags,
+                                          BOOL fWasDeleted)
 {
     LogFlowThisFunc(("\n"));
@@ -13611 +13621 @@
         mHWData.backup();
 
-        bool fDelete = !aValue.length();
         HWData::GuestPropertyMap::iterator it = mHWData->mGuestProperties.find(aName);
         if (it != mHWData->mGuestProperties.end())
         {
-            if (!fDelete)
+            if (!fWasDeleted)
             {
                 it->second.strValue   = aValue;
@@ -13626 +13635 @@
             mData->mGuestPropertiesModified = TRUE;
         }
-        else if (!fDelete)
+        else if (!fWasDeleted)
         {
             HWData::GuestProperty prop;
@@ -15046 +15055 @@
                                    const com::Utf8Str &aValue,
                                    LONG64 aTimestamp,
-                                   const com::Utf8Str &aFlags)
+                                   const com::Utf8Str &aFlags,
+                                   BOOL fWasDeleted)
 {
     NOREF(aName);
@@ -15052 +15062 @@
     NOREF(aTimestamp);
     NOREF(aFlags);
+    NOREF(fWasDeleted);
     ReturnComNotImplemented();
 }