-
Notifications
You must be signed in to change notification settings - Fork 293
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
Added secure_write to function for cookie / token saves #77
Conversation
Like a patch release should closely follow the merge. |
@minrk @SylvainCorlay @Carreau @jasongrout I can't add reviewers to this PR directly it seems, so here's your ping :) |
Also ping @Zsailer |
jupyter/jupyter_client#469 has the same change for the client. It was suggested we move this to core after we get both places patched. |
One thing to note is that for Windows you can't set the group permissions with chmod (see failing appveyor test on the server PR) :/ It might require something like https://stackoverflow.com/questions/12168110/setting-folder-permissions-in-windows-using-python to get passing. Thoughts? |
Sounds reasonable. Are there other Jupyter projects that already depend on pywin32? We need to make sure that the dependency only applies on Windows. Do you have a pointer to the advisory that asks for this change? |
@rolweber I added you to the relevant thread. |
Travis tests failed: File "/home/travis/build/jupyter/jupyter_server/jupyter_server/serverapp.py", line 685, in _default_cookie_secret
self._write_cookie_secret_file(key)
File "/home/travis/build/jupyter/jupyter_server/jupyter_server/serverapp.py", line 694, in _write_cookie_secret_file
with secure_write(self.cookie_secret_file, True) as f:
File "/opt/python/3.7.1/lib/python3.7/contextlib.py", line 239, in helper
return _GeneratorContextManager(func, args, kwds)
File "/opt/python/3.7.1/lib/python3.7/contextlib.py", line 82, in __init__
self.gen = func(*args, **kwds)
TypeError: secure_write() takes 1 positional argument but 2 were given The second argument |
Gah copy paste issue from jupyer_client change -- forgot the mode option |
The logic currently is:
That means the file which gets opened in 3 is not necessarily the file that was checked in 2.
And could you please add a newline at the end of |
I'm wondering if renaming of the open file could lead to checking the wrong file. But even then, the solution would be safer than not doing anything at all. |
So on some systems I've run into runtime issues editing permissions while a file is open will result in failure (looking at you windows 7). In testing I also found the open command was not changing file permissions for existing files, which is why I did the remove first. Because of these problems I opted for a two-pass solution. Since this is only a write and not a read command and the write is a truncate operation I believe this is safe. A user would need admin or user permissions to move the file being touched. So they already have escalated permissions to simply read/write the affected file anytime after the secure_write completes. Anything in the file ahead of time is removed with the truncate on write command, so injection shouldn't be possible as well. Do you think there is still something open I'm not thinking about? |
If it's Windows that requires special treatment, I'm very much in favor of treating that case in the Removing the file and creating a new one makes sense. I'm not sure if permissions could be set on some systems that would prevent the user from removing the file, while still allowing writes to it. But you're right, we shouldn't overthink this. Whoever messes with file permissions or renaming during the Still, could you please refactor the logic so that the file is created and opened only once for the non-Windows case? |
Yep I will make the change tomorrow then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks! |
@rolweber @SylvainCorlay could we get a patch release with these changes? Since there some security concern driving this change it'd be good to not have the issue visible with no patch available for users. |
I'm not in a position to make releases, but I agree that this change merits one. |
This follows an advisory suggesting this change be made to help protect the secret files from being exposed to other users on a box running jupyter.