Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the incorrect operation when setuptools plugins output something into stdout. #2335

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Fixed the incorrect operation when setuptools plugins output something into stdout. #2335

merged 1 commit into from
Jun 29, 2022

Conversation

KOLANICH
Copy link
Contributor

No description provided.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to make the CI pass

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why do we need this secrets thing and the cookie for this?

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Jun 4, 2022

Can you explain why we need this secrets thing and the cookie for this?

Cookies

  1. Cookie allows to split the stdout output generated by the script collecting the stuff and serializing it from the output generated by something else.

  2. To be honest the right way to deal with it is to create an anonymous pipe and pass its descriptor to the child and output to it. Or use other way of out-of-band signalling. But AFAIK all of them are either not cross-platform or too big to implement and are not in the stdlib. So the easiest and the shortest way I could mind is just using the cookies.

  3. The only sensible way to use the cookies in in-band signalling is to have them random.
    3.a) Well, we could have used the static cookies, but it would break if the app somehow printed them. It may sound like a paranoia, but let's assumme an adversary building a plugin trying to interfere with virtualenv operation specifically by printing the data into stdout, if it is "enough easy to do". This way we also get protection from accidential breakage. We don't really try to protect from malicious interference, the easiest way to do it reliably is just checking traceback stack with inspect module, but we should protect from anything easier than it.
    3.b) Literally hardcoded cookies are easy to break by just emitting them. Can accidentially happen if the lib somehow had to print the source of the file where they are hardcoded. Countermeasure - not using literal ones.
    3.c) OK, "nFi39jRw0xt3" + "psYkd7ei4B3d9" should be OK, right? The easiest way to break is just hardcode it. The easiest method to defeat it is to generate a random string.
    3.d) The next easiest method to break is to print them from the args or variables in stack trace. I have made a mistake, I shouldn't have outputted them as they are. Instead I should have transformed them, i.e. reversed and concatenated.

  1. I probably shouldn't have used secrets module and instead should have used a regular insecure pseudorandom generator. Derandomizing pseudorandom numbers from a foreign process is too much burden for our threat model.

src/virtualenv/discovery/py_info.py Outdated Show resolved Hide resolved
src/virtualenv/discovery/py_info.py Outdated Show resolved Hide resolved
src/virtualenv/discovery/cached_py_info.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have outstanding questions to

src/virtualenv/discovery/py_info.py Outdated Show resolved Hide resolved
src/virtualenv/discovery/cached_py_info.py Outdated Show resolved Hide resolved
@KOLANICH
Copy link
Contributor Author

KOLANICH commented Jun 7, 2022

The next easiest method to break is to print them from the args or variables in stack trace. I have made a mistake, I shouldn't have outputted them as they are. Instead I should have transformed them, i.e. reversed and concatenated.

If someone called in setuptools plugins a lib writing the whole stack trace with the variables in each stack frame, eventually the stack trace will reach the lines 523-525 of py_info and the cookies will be accidentially printed. If we applied some simple transformation to them and search for the transformed ones, thjs kind of breakage can be avoided.

  1. We just output the output that would have been output if we hadn't used a separate process for collecting the info. It was a big surprise for me that my setuptools plugin is executed during the process at all, and I guess it should be never concealed. If that plugin had a bug or nkt a bug but a feature terminating the build in certain circumstances, a user must know why the process fails.

There are 2 cookies because the output can be generated both before and after we output the json-serialized info.

@gaborbernat
Copy link
Contributor

Sounds good, though you'll need to fix the CI for this to be merge-able.

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Jun 7, 2022

Not sure if it'd pass tests now. It seems that the tests just don't like additional lines in output, so on my machine it fails the tests even on main branch.

@gaborbernat
Copy link
Contributor

Can you resolve the merge conflicts?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaborbernat gaborbernat merged commit 8856f51 into pypa:main Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants