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

Run _elevatePrivileges in a thread #346

Open
chevah-robot opened this issue May 12, 2015 · 6 comments
Open

Run _elevatePrivileges in a thread #346

chevah-robot opened this issue May 12, 2015 · 6 comments

Comments

@chevah-robot
Copy link

T2843 bug was created by adiroiban on 2015-05-12 18:04:39Z.
Last changed on 2017-09-20 18:32:47Z.

In order to not affect other thread the _elevatePrivileges context manager should be execute in a thread.

It might need to be refactored from a context manager to a callback based code

instead of


                with process_capabilities._elevatePrivileges(
                        win32security.SE_CREATE_SYMBOLIC_LINK_NAME):
                    win32file.CreateSymbolicLink(
                        link_path, target_path, flags)

we might have


def elevated_work():
    win32file.CreateSymbolicLink(
       link_path, target_path, flags)
    return 'something'


result = process_capabilities._elevatePrivileges(
    win32security.SE_CREATE_SYMBOLIC_LINK_NAME, callback=elevated_work)

the callback is then executed in a separate impersoanted thread, as on Windows we can impersonate just a thread...

but this will imply converting all calls to async/deferred calls...and will need a lot of work...

so for now, it should be OK if we keep the current context manager and just us a lock.

@chevah-robot
Copy link
Author

Comment by alibotean at 2016-06-07 15:42:37Z.

so for now, it should be OK if we keep the current context manager and just us a lock.

Ok.

@chevah-robot
Copy link
Author

Comment by alibotean at 2016-06-07 16:40:06Z.

I've re-read the ticket description several times but I think I'm missing the point/don't understand it.

Basically my understanding is:

  • we are elevating priviliges for an operation on thread A; since we're using process token all other threads executing until we lower privileges will have elevated ones
  • other threads executing are "less" secure

Now here's my problem: using a lock implies knowing which methods to lock. Seems to me that almost any call that can wind down to the disk/process is liable to be locked. Am I missing something?


I've looked at the usages of the _elevatePrivileges method and it's used only in makeLink and _setOwner.

I would attempt to do the conversion and do away with any locks.

Please shed some light. Thanks.

@chevah-robot
Copy link
Author

Comment by adiroiban at 2016-06-10 13:30:27Z.

the lock will not work. Ignore that part.

the problem is that compat is designed as a blocking api ... so I don't really know how to change this...

we should have a voice chat and talk about this, as things are ugly

@chevah-robot
Copy link
Author

Comment by alibotean at 2016-06-13 10:31:31Z.

the lock will not work. Ignore that part.

Understood.

the problem is that compat is designed as a blocking api ... so I don't really know how to change this...
we should have a voice chat and talk about this, as things are ugly

We could start another thread, and wait for it's completion (join): in this way we get a blocking API and a dedicated thread for processing the elevated job.

I will get in touch and discuss.

@chevah-robot
Copy link
Author

Comment by adiroiban at 2017-02-06 21:19:04Z.

This is not criticial as we allowed it not be fixed for so many time :(

@chevah-robot
Copy link
Author

Comment by alibotean at 2017-02-07 10:49:34Z.

This is not criticial as we allowed it not be fixed for so many time :(

There were always more pressing matters. This will require careful consideration and testing as it implies more threads and waiting.

Once we fix the bugs in the client side I will give this a try as the database is in good shape now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants