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.

Sign up for GitHub

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

Jump to bottom

Update exec-used documentation #7039

Closed
Pierre-Sassoulas opened this issue Jun 25, 2022 · 8 comments · Fixed by #7512
Closed

Update exec-used documentation #7039

Pierre-Sassoulas opened this issue Jun 25, 2022 · 8 comments · Fixed by #7512
Labels
Documentation 📗 Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jun 25, 2022 •
edited

Bug description

In the following code, what can be use in exec is properly restricted.

def get_user_code(name):
    return input(f'Enter code to be executed please, {name}: ')


username = "Ada"
allowed_globals = {'__builtins__' : None}
allowed_locals = {'print': print}
exec(get_user_code(username), allowed_globals, allowed_locals)

For example, if Ada is reasonable:

Enter code to be executed please, Ada: print("The more I study, the more insatiable do I feel my genius for it to be")
The more I study, the more insatiable do I feel my genius for it to be

But if she's trying to read all our env variable:

Enter code to be executed please, Ada: import os;print(os.environ)
Traceback (most recent call last):
  File "c.py", line 8, in <module>
    exec(get_user_code(username), globals, locals)
  File "<string>", line 1, in <module>
SystemError: ../Objects/dictobject.c:1438: bad argument to internal function

Command used

pylint a.py

Pylint output

************* Module a
a.py:2:11: W0141: Used builtin function 'input' (bad-builtin)
a.py:8:0: W0122: Use of exec (exec-used)

Expected behavior

No exec-used, proper care were taken.

Pylint version

2.15.0-dev0
@Pierre-Sassoulas Pierre-Sassoulas added Proposal 📨 False Positive 🦟 A message is emitted but nothing is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection labels Jun 25, 2022
@DanielNoord
Copy link
Collaborator

What would be the required change: no longer emit exec if globals and/or locals is provided?

@Pierre-Sassoulas
Copy link
Member Author

What would be the required change: no longer emit exec if globals and/or locals is provided?

Yes

@DanielNoord
Copy link
Collaborator

Kind of depends on how "safe" we want to keep these.
If you're using Python for a server or API backend I think it would be good to have this warning irrespective of globals and locals.
exec("raise SystemExit", {}, {}) could still cause many troubles.

@Pierre-Sassoulas
Copy link
Member Author

Maybe we could burst the message into "unsafe-exec-used" and "exec-used" ?

@DanielNoord
Copy link
Collaborator

I'm not sure. In the example you gave above I can still raise SystemExit. There are probably other attack vectors that we don't even know about.
If you really want to exec, just disable the message? It's a short message name anyway 😄

@Pierre-Sassoulas
Copy link
Member Author

Being able to raise exception is not the same as being able to read all the env variable and transferring them to a server using requests though. Imo if globals / locals are defined, it probably means everything we'll raise about this call will be a false positive because the user thought about it.

@DanielNoord
Copy link
Collaborator

Try exec(input(), {}, {}) and type in:
exec("""\nwith open("file.txt", "w", encoding="utf-8") as file:\n file.write("test")\n""")

I can write to your system, I can read from your system and then print the contents, I can probably even import requests and do it that way. exec should always be warned against imo. Just to make sure users are very sure they want to use it and make sure it can't be controlled by anybody else.

@Pierre-Sassoulas
Copy link
Member Author

Well that is convincing. Let's update the doc about exec-used with details from this conversation and close this.

@Pierre-Sassoulas Pierre-Sassoulas added Documentation 📗 Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Proposal 📨 False Positive 🦟 A message is emitted but nothing is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection labels Jul 2, 2022
@DanielNoord DanielNoord added Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors labels Jul 2, 2022
@DanielNoord DanielNoord changed the title False positive exec-used when precautions were taken Update exec-used documentation Jul 2, 2022
DanielNoord added a commit to DanielNoord/pylint that referenced this issue Sep 22, 2022
@DanielNoord
Update exec--used documentation
626cbcc 
Closes pylint-dev#7039
@DanielNoord DanielNoord mentioned this issue Sep 22, 2022
Merged
3 tasks
Pierre-Sassoulas added a commit that referenced this issue Sep 22, 2022
@DanielNoord @mbyrnepr2 @Pierre-Sassoulas
A collection of documentation updates (#7512)
ea86fcf 
* Update ``exec--used`` documentation

Closes #7039

* Be more explicit about third party plugins

Closes #6900

* Document behaviour of config file generators

Refs. #7478

Co-authored-by: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation 📗 Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

A collection of documentation updates DanielNoord/pylint
2 participants
@Pierre-Sassoulas @DanielNoord