WooCommerce Can’t Count Either
In continuation of yesterday’s post about bbPress, I decided to look for a more impactful race condition vulnerability. What’s more impactful on an online business than ecommerce?
WooCommerce is up for the thread-safety test in this post and probably a couple of other to follow.
What changes with every sale most of the time? Product stock. In WooCommerce it’s decreased when an order is paid. The code responsible for this is currently located in WC_Product_Data_Store_CPT::update_product_stock
. The docblock acknowledges that this method contains critical sections of code and that locking is used (spoiler: it’s not).
Uses queries rather than update_post_meta so we can do this in one query (to avoid stock issues). Uses locking to update the quantity. If the lock is not acquired, change is lost.
But looking at the actual body of the method you’re met (in version 4.0.1) with this comment:
// @todo: potential race condition. Read current stock level and lock the row. If the lock can’t be acquired, don’t wait.
Yikes! The lock was never implemented. It’s a tiny bit better than using a get_post_meta
and update_post_meta
chain, for one reason: doing direct database queries is faster; the race condition window is tinier. But essentially it’s the same: there are windows between fetching the value and updating it. So it’s still very vulnerable. Let’s prove this.
First, I created a product with 2000 stock quantity. Then I wrote a jMeter request scenario that orders this product as a new customer and pays for it via a mock payment gateway. Instead of writing a full scenario the same can be done with the following code as well:
// Create a random customer $customer = new WC_Customer(); $customer->set_email( md5( random_bytes( 32 ) ) . '@sandbox.threadsafe.org' ); $customer_id = $customer->save(); $order = new WC_Order(); $order->set_customer_id( $customer_id ); $item = new WC_Order_Item_Product(); $item->set_quantity( 1 ); $item->set_product_id( 18 ); // A product with 2000 stock and no backorders $order->add_item( $item ); $order->save(); $order->payment_complete(); // Paid and ready to go
After a pool of 20 concurrent shop sessions buys the product 100 times in succession my product is left with the following stock:
67 products left! This happened because while updating the stock concurrent threads overwrote the stock value with stale data. There are 2000 paid orders for the product, but there’s still 67 units of the product left.
What would this mean for your business? In this specific scenario you’ll have to speak to up to 67 customers. Tell them that you don’t have any stock left, oops. Or backorder and explain that they need to wait. If WooCommerce is integrated with your accounting software, or with your warehouse or dropshipper, then the problem further extends to third-parties.
Enough talk, though. There’s an evident problem.
What can be done?
I’ve opened a pull request in the WooCommerce GitHub repository. The fix is fairly straightforward. It only works for increment and decrement operations. I use direct SQL math operations instead of separate queries.
For the set operation this needs another strategy. Maybe use a FROM DUAL
“upsertish” database statement, which is atomic. Very similar to how WP_Lock‘s wpdb
backend functions. Luckily set operations are used only on the Product management screen.
The patch still doesn’t address a higher-level issue. The checkout mechanism has a race condition while placing the order, by the time it checks whether there’s enough stock a concurrent request would have already placed a valid order. This will result in negative stock quantities by a couple of stock units with my patch. But at least the stock value will match the number of orders to the dot.
Disclaimer
This is not a security threat.
This work was sponsored by threadsafe.org.
My personal favorite WooCommerce race condition is issue #24936. (Note that this issue is marked as “closed” but the race condition still exists for orders which are manually created by the administrator.)
Indeed https://github.com/woocommerce/woocommerce/issues/24936 I’ve seen this happen to a client site. The solution they went with is adding a hook to notify admins of such cases. I can see that the issue itself wasn’t solved. Thanks for sharing.
[…] Kovshenin poängterar att såväl bbPress som WooCommerce har svårt att räkna. Fascinerande […]
Didn’t they fix this in WC 4.3?
Yep, it was merged into 4.1.0-beta.1 so is contained in 4.3 🙂
This is just one fix of many more that are needed.
Hello ,
I’m Nannie.
If you’ve ever been too busy and couldn’t finish a academic paper, then you’ve come to the right place. I assist students in all areas of the writing technique. I can also write the assignment from start to finish.
My career as an academic writer started during high school. After learning that I was very skilled in the field of academic writing, I decided to take it up as a job .
Talented Academic Writer- Nannie- http://www.queronaao.com Corp
Hello, I am Kyran a professional in content writing.
I love solving people’s problems and make them happy. That is what I have been doing for decades now.
I have been writing since I was 12 years old and never knew it would turn out to be a full-time career. I have also been able to manage several assignments that involves writing. And I worked in three organizations as a volunteer to assist people.My interest has always been to help people succeed. And I go the extra mile to make that happen.
I enjoy writing academic papers and have helped people from countries like Mexico.
I work with a company whose mission is to provide quality writing and make people happy. In fact, many people come to me for professional help on a daily basis because they know I always deliver. And I will continue to provide nothing but the best to build trust like I have been doing for the past few years.
Expert writer – Kyran – http://www.thechumscrubber.com Confederation