Opened 3 years ago

Closed 3 years ago

#29394 closed defect (fixed)

Drop the python2 patch sys_path_security-issue_16202

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: packages: optional Keywords:
Cc: vbraun, jdemeyer, embray, slelievre, gh-timokau, fbissey, arojas, nthiery, isuruf, pcpa, thansen, infinity0, snark, cschwan, jhpalmieri Merged in:
Authors: Matthias Koeppe, John Palmieri Reviewers: François Bissey, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: cbdce1c (Commits, GitHub, GitLab) Commit: cbdce1cef6350482d9ec65c878188e0f6530bc22
Dependencies: #25358 Stopgaps:

Status badges

Description

This patch, applied by #13579 ("Python sys.path security risk") 7 years ago was never accepted in upstream Python. We do not bother to do the same in python3; and it causes build problems (#29367).

Related tickets:

  • #25358 Test safe directory without python subprocess
  • #26457 Do not depend on a patched Python

Change History (32)

comment:1 Changed 3 years ago by mkoeppe

Cc: jhpalmieri added

comment:2 Changed 3 years ago by mkoeppe

Branch: u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202

comment:3 Changed 3 years ago by mkoeppe

Authors: Matthias Koeppe
Commit: 1a578e4e520b1becde150ff7ca1ae61bf85296d9
Status: newneeds_review

New commits:

1a578e4Remove python2 patch sys_path_security-issue_16202

comment:4 Changed 3 years ago by fbissey

Reviewers: François Bissey
Status: needs_reviewpositive_review

Good riddance.

comment:5 Changed 3 years ago by jhpalmieri

Status: positive_reviewneeds_work

comment:6 Changed 3 years ago by jhpalmieri

Branch: u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202u/jhpalmieri/drop_the_python2_patch_sys_path_security_issue_16202

comment:7 Changed 3 years ago by jhpalmieri

Commit: 1a578e4e520b1becde150ff7ca1ae61bf85296d9fd4e4086d60f3b7212bcbee03e0f360ecd8d3a05
Status: needs_workneeds_review

Doctest failures with a Python 2 build, so let's get rid of the offending doctests.


New commits:

fd4e408trac 29394: get rid of corresponding doctest

comment:8 Changed 3 years ago by fbissey

Status: needs_reviewpositive_review

Hum, I thought it was going to be handled in a different ticket, but yes that test should go, of course. And that's the only place I know where the issue would crop up.

comment:9 Changed 3 years ago by jhpalmieri

Do we need to delete the function test_safe_directory in doctest/control.py?

comment:10 in reply to:  9 Changed 3 years ago by fbissey

Replying to jhpalmieri:

Do we need to delete the function test_safe_directory in doctest/control.py?

I wouldn't think it is needed. After all it works well with python without change. Let me double check.

comment:11 Changed 3 years ago by jhpalmieri

Status: positive_reviewneeds_work

To partially answer my own question: that function gives another doctest failure with a Python 2 build. Its docstring also says "This is only relevant with Python 2, because Sage's Python 2 is patched to give a warning when the current directory is unsafe, but Python 3 is not." So I think we should remove the function entirely.

comment:12 Changed 3 years ago by fbissey

Oh right, I am forgetting we are still doing py2 builds here. Yes it should go.

comment:13 Changed 3 years ago by jhpalmieri

I'll provide a branch which removes it. If you're happy with that, once I run tests successfully with Python 2, I can set to positive review on your behalf.

comment:14 Changed 3 years ago by mkoeppe

#25358 has a better fix for test_safe_directory

comment:15 Changed 3 years ago by git

Commit: fd4e4086d60f3b7212bcbee03e0f360ecd8d3a05c1f6dcded4ce720d67c0be89b44d40a76bc6eef3

Branch pushed to git repo; I updated commit sha1. New commits:

c1f6dcdtrac 29395: remove test_safe_directory

comment:16 in reply to:  14 ; Changed 3 years ago by fbissey

Replying to mkoeppe:

#25358 has a better fix for test_safe_directory

So, that's why I thought that stuff was dealt with in another ticket. I am not completely losing my marbles.

comment:17 in reply to:  16 ; Changed 3 years ago by jhpalmieri

Replying to fbissey:

Replying to mkoeppe:

#25358 has a better fix for test_safe_directory

If you want to try to revive that, you can. It can be done on #25358, with this ticket as a prerequisite, since one of the main obstacles there was the patched Python. Once we remove this patch to Python, the current incarnation of test_safe_directory is no longer relevant, as far as I can tell.

So, that's why I thought that stuff was dealt with in another ticket. I am not completely losing my marbles.

That ticket had completely stalled, though.

comment:18 Changed 3 years ago by fbissey

Thanks John, we have to make a decision on what we do with #25358 (and how fast) before we proceed with the current changes. Plain removal is attractive to me. Less code.

comment:19 Changed 3 years ago by mkoeppe

I think test_safe_directory is a useful security mechanism. Its current implementation depends on the patch that we are now dropping (and hence on Python2)

comment:20 Changed 3 years ago by mkoeppe

Dependencies: #25358

comment:21 in reply to:  17 Changed 3 years ago by mkoeppe

Replying to jhpalmieri:

Replying to fbissey:

Replying to mkoeppe:

#25358 has a better fix for test_safe_directory

If you want to try to revive that, you can.

Done

It can be done on #25358, with this ticket as a prerequisite

No, present ticket depends on #25358

comment:22 Changed 3 years ago by mkoeppe

Branch: u/jhpalmieri/drop_the_python2_patch_sys_path_security_issue_16202u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202

comment:23 Changed 3 years ago by mkoeppe

Authors: Matthias KoeppeMatthias Koeppe, John Palmieri
Commit: c1f6dcded4ce720d67c0be89b44d40a76bc6eef3cbdce1cef6350482d9ec65c878188e0f6530bc22
Status: needs_workneeds_review

New commits:

904b1a1Test safe directory without subprocess
7f5d8dcRemove python2 patch sys_path_security-issue_16202
cbdce1ctrac 29394: get rid of corresponding doctest

comment:24 Changed 3 years ago by gh-timokau

For what it's worth I gave up on #25358 because jdmeyer had a pretty strong opinion on that. I'm not sure if that has changed. If it was up to me, I think I'd vote complete removal too. #25358 was intended as a compromise.

comment:25 Changed 3 years ago by mkoeppe

If we want, we can of course keep *both* tests in #25358. Keeping py2 as "secure" as it was; and adding security to py3.

comment:26 Changed 3 years ago by jhpalmieri

We can't keep the secure py2 test if we're getting rid of the patch, or am I misunderstanding you?

comment:27 in reply to:  26 Changed 3 years ago by mkoeppe

Replying to jhpalmieri:

We can't keep the secure py2 test if we're getting rid of the patch, or am I misunderstanding you?

Right, it would be purely decorative or for the hypothetical situation that someone decides to run sagelib with a patched python2.

comment:28 Changed 3 years ago by mkoeppe

Testing this branch at https://github.com/mkoeppe/sage/actions/runs/61077298 (on top of other tickets)

comment:29 Changed 3 years ago by jhpalmieri

This passes tests for me on OS X with builds based on Python 2 and Python 3.

comment:30 Changed 3 years ago by dimpase

Reviewers: François BisseyFrançois Bissey, Dima Pasechnik
Status: needs_reviewpositive_review

lgtm

comment:31 Changed 3 years ago by mkoeppe

Thanks

comment:32 Changed 3 years ago by vbraun

Branch: u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202cbdce1cef6350482d9ec65c878188e0f6530bc22
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.