alexgolec / tda-api

A TD Ameritrade API client for Python. Includes historical data for equities and ETFs, options chains, streaming order book data, complex order construction, and more.
https://tda-api.readthedocs.io
MIT License
1.26k stars 335 forks source link

CVE - Remote code execution in test suite. #318

Closed BillSchumacher closed 2 years ago

BillSchumacher commented 2 years ago

Please read the bug submission guidelines before submitting a bug.

Not following guidelines may result in your bug being ignored and/or closed.

Description of Bug Remote code execution is possible via rouge commit or hidden logic.

Code to Reproduce See PR.

IMPORTANT: Remember to anonymize your code. Be sure to replace API keys/Client IDs with placeholders. Also, never, ever share the contents of your token file.

Expected Behavior Code does not execute arbitrarily on my system.

Actual Behavior Code executes arbitrarily when running test suite.

Error/Exception Log, If Applicable See here to learn how to turn on debug logging: https://tda-api.readthedocs.io/en/latest/help.html

BillSchumacher commented 2 years ago

The implementation of this is very dangerous and highly suspect.

alexgolec commented 2 years ago

So let me get this straight: you're executing code arbitrarily on my system by… editing the library? This is not exactly CVE material.

Don’t get me wrong, I get the principle of what you’re trying to argue. The library is in fact generating text that happens to represent code, and you have found a way to manipulate that text. But generating text does not constitute a security vulnerability.

BillSchumacher commented 2 years ago

By passing a value to a function, I could have done this a number of ways as could have you.

BillSchumacher commented 2 years ago

If you want to see a sneakier example I can put that together too.

BillSchumacher commented 2 years ago

You asked me to show you where this was ever executed by the library, there it is.

BillSchumacher commented 2 years ago

I'm not saying you're a bad person, but I will say it would be very easy for you to sneak in nefarious logic.

alexgolec commented 2 years ago

While I think you have pointed out something good, which is that there is an opportunity here to replace this exec statement with a compiler attempt, I have to admit your demeanor and rudeness make me less than eager to continue working with you.

If you want to redeem yourself by making yourself useful, feel free to send me a PR that replaces this exec call with a call to the following:

https://stackoverflow.com/questions/37012947/how-to-tell-if-a-single-line-of-python-is-syntactically-valid

BillSchumacher commented 2 years ago

Your lax attitude about security makes me not want to work with you either, I was simply protecting someone that may not know what this code was capable of.

BillSchumacher commented 2 years ago

Consider the people that use your library and their users, the most probable victims, over my rudeness.

alexgolec commented 2 years ago

Update: running exec in the test suite is in fact correct behavior because the test needs to verify that the generated object is equal to the one that was passed in. Working as intended, no CVE granted.

BillSchumacher commented 2 years ago

Lol, sir... let's talk about pickle then.

BillSchumacher commented 2 years ago

You unfortunately don't get to decide what is a risk and what is not. exec is almost never the correct answer.

BillSchumacher commented 2 years ago

A warning at a minimum about the security risks of using pickle should be posted.

BillSchumacher commented 2 years ago

This kind of response to something that would ultimately affect me directly by running tests on your code base is unacceptable. Considering the amount of money involved.

r4m0n commented 2 years ago

This is not a chat room, please stop spamming people following this library.

BillSchumacher commented 2 years ago

Ah, well I tried here. Good luck folks.

alexgolec commented 2 years ago

+1 to @r4m0n. Please note that if the next interaction you have with this library or its community is not constructive, I will block you from interacting with us.