-
Notifications
You must be signed in to change notification settings - Fork 696
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
Reduce time information leakage of file uploads and file downloads #3305
Conversation
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.
Would it be possible to add an additional check to this that when we unzip and read the file back, we can check it's attribute to ensure that the time is actually set to 0?
Codecov Report
@@ Coverage Diff @@
## develop #3305 +/- ##
========================================
Coverage 85.76% 85.76%
========================================
Files 34 34
Lines 2157 2157
Branches 238 238
========================================
Hits 1850 1850
Misses 250 250
Partials 57 57 Continue to review full report at Codecov.
|
Yep @heartsucker, i agree. Maybe a full retesting of the zip archive for both the single file and the bulk archive |
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.
Thanks @evilaliv3 for the fix! Changes look good to me. I can confirm this strips the compression time on the submission's gzip archive.
On develop
:
$ file 1-iridescent_cheapskate-doc.gz
1-iridescent_cheapskate-doc.gz: gzip compressed data, was "file.txt", last modified: Mon Apr 30 14:01:02 2018, max compression
On this branch:
securedrop$ file 1-infectious_carpentry-doc.gz
1-infectious_carpentry-doc.gz: gzip compressed data, was "file.txt", max compression
I agree with @heartsucker that adding a test to ensure the time is properly stripped is a good idea, but will defer to @redshiftzero on if this is required for merge.
Let's merge this and then add additional testing - both changes should get backported into 0.7.0 |
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.
I've created #3329 to implement at a later time. Thanks @evilaliv3 !
The proposed fix together with the existing implementation performed for ticket #301 should be enough to avoid complete leakage of the file upload date.
Fixes #3304