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

Close vunerabilities with pickle #35

Closed
Theelx opened this issue Feb 18, 2021 · 9 comments
Closed

Close vunerabilities with pickle #35

Theelx opened this issue Feb 18, 2021 · 9 comments

Comments

@Theelx
Copy link
Collaborator

Theelx commented Feb 18, 2021

@dobrosketchkun If you have time, do you have any ideas for how to fix the issues shown in #34? I'm not that good with pickle or such.

@AlfredoSequeida
Copy link
Owner

I'm open to hearing suggestions about this as well. It's my understanding that the vulnerability lies with untrusted data as stated in the pickle documentation:

It is possible to construct malicious pickle data which will execute arbitrary code during unpickling. Never unpickle data that could have come from an untrusted source, or that could have been tampered with.

But I also understand the argument that It's really something for the user to be aware of (don't download random files off the internet) rather than something for us to handle. But if there is something we can do about it while still making that part of the code functional, I'm open to suggestions.

@dobrosketchkun
Copy link
Contributor

Oy-vey, what an unfortunate turn of events. I'll look into this problem on weekend and into the second one too, with the zip issue.

@AlfredoSequeida
Copy link
Owner

AlfredoSequeida commented Feb 20, 2021

Here is a suggestion for how to replace pickle. We can achieve similar logic using JSON instead. Something like this:

data = {"tag": tag, "data": ciphertext, "filename": filename}

# dumping json and encoding the string as utf-8 to get a bytes object
data_bytes = json.dumps(data).encode('utf-8')

# ... then we can do everything else like before

And similar logic for decoding.

I haven't tested it yet, but doing it this way allows us to make small modifications and keep most of the existing logic in place.

@dobrosketchkun What do you think?

@dobrosketchkun
Copy link
Contributor

It looks like a nice idea!

@Theelx
Copy link
Collaborator Author

Theelx commented Feb 20, 2021

@AlfredoSequeida Are you sure that the bytes object won't be compressed by YouTube?

@dobrosketchkun
Copy link
Contributor

Well, since pickle object is also just bytes and it works, so, I don't think it'll be an issue.

@AlfredoSequeida
Copy link
Owner

@Theelgirl I think the result should be the same since the pickle.dumps() function also returns a bytes object and that seemed to be working. But I guess I'll know once it's been implemented. If I have time later today I'll try it.

@AlfredoSequeida
Copy link
Owner

I just finished replacing the logic for pickle using json. I will be pushing that soon. As a plus side, I had an mp3 file that was not working when we were using pickle and for some reason using the json implementation fixed that. I wonder if pickle was changing the data somehow.

@AlfredoSequeida
Copy link
Owner

closed with 363a9c1

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

No branches or pull requests

3 participants