search

openvswitch / ovs-issues

Issue tracker repo for Open vSwitch
10 stars 3 forks source link

A use-after-free defect was discovered at line 1251 of the file /ovs/ovsdb/transaction.c. #322

Closed LuMingYinDetect closed 4 months ago

LuMingYinDetect commented 4 months ago

Dear OpenvSwitch Developers,

I hope this email finds you well. I am writing to report a potential vulnerability found in the /ovs/ovsdb/transaction.c file.

Upon investigation, it has been discovered that there exists a use-after-free defect at line 1251 of the mentioned file. For detailed information regarding this defect, please refer to the following link: https://github.com/LuMingYinDetect/openvswitch_defects/blob/main/openvswitch_detect_1.md.

As a responsible member of the community, I believe it is crucial to promptly address such security concerns to ensure the integrity and reliability of the Open vSwitch project.

Thank you for your attention to this matter. Please let me know if you require any further information or assistance from my end.

Best regards,

Mike

igsilya commented 4 months ago

Hi, @LuMingYinDetect . Thanks for the report. The analysis that there will be a use-after-free in case the 'error' is non-NULL is correct, however the Assuming 'error' is non-null assumption itself is not viable, because there is no code path that results in for_each_txn_row(txn, determine_changes); returning anything but NULL. This is also documented in the code with the can't happen error, because it literally can't happen:

  1. for_each_txn_row() function returns non-NULL only if callback returns non-NULL.
  2. determine_changes() callback always returns NULL, so for_each_txn_row() always returns NULL in this case as well.

So, while I agree that the ovsdb_txn_abort(txn); call should be removed to not confuse static analysis tools, it would be just a cosmetic change as this code is not reachable.

P.S. If you suspect a security issues in OVS, please, send an email to ovs-security@openvswitch.org instead of creating a public issue.