Skip to content

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

Merged
gaborbernat merged 1 commit into
pypa:mainfrom
KOLANICH-tools:stdout_fix
Jun 29, 2022
Merged

Fixed the incorrect operation when setuptools plugins output something into stdout.#2335
gaborbernat merged 1 commit into
pypa:mainfrom
KOLANICH-tools:stdout_fix

Conversation

@KOLANICH

Copy link
Copy Markdown
Contributor

No description provided.

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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

KOLANICH commented Jun 4, 2022

Copy link
Copy Markdown
Contributor Author

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.
Comment thread src/virtualenv/discovery/py_info.py Outdated
Comment thread src/virtualenv/discovery/py_info.py Outdated
Comment thread src/virtualenv/discovery/cached_py_info.py Outdated
Comment thread setup.cfg Outdated

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/virtualenv/discovery/py_info.py Outdated
Comment thread src/virtualenv/discovery/cached_py_info.py Outdated
@KOLANICH

KOLANICH commented Jun 7, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@KOLANICH

KOLANICH commented Jun 7, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

Can you resolve the merge conflicts?

@gaborbernat gaborbernat left a comment

Copy link
Copy Markdown
Contributor

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

2 participants