search

consolibyte / quickbooks-php

QuickBooks Integration for PHP
Eclipse Public License 1.0
457 stars 338 forks source link

Security Related Concerns - replace the user of serialize() with json_encode() #265

Open starshine531 opened 5 years ago

starshine531 commented 5 years ago

I'm making a web connector integration with Quickbooks with your SDK and have a couple of security related concerns.

I noticed that you use unserialize in some places. Somewhat recently, unserialize was found to be vulnerable to attack. Basically, it's recommended to never use unserialize on user supplied data. Has your SDK been updated to address this issue?

Also, I don't see your mysqli driver using parameterized queries. Has this been updated sufficiently to prevent sql injection?

starshine531 commented 5 years ago

It looks like serialize/unserialize, might be able to be replaced with json_encode/json_decode. From what I have read, json_decode doesn't have the same vulnerability as unserialize because it cannot be decoded into any class, only the stdClass. It also does not use the __wakeup method (which is part of what makes the vulnerability possible). Other than that, it accomplishes the same thing.

consolibyte commented 5 years ago

We should be able to replace this with json_encode/decode, and I do think that'd be a good thing. As a temporary work-around, you can make sure you validate any user-supplied input you pass into anything that gets serialized. The $extra parameter to ->enqueue() is just about the only thing serialized here. As a precaution, you should be careful what you pass in there and make sure users aren't passing in classes/objects.

The mysqli driver uses Drupal-style parameter substitution, and is not vulnerable to SQL injections.