Issue B: SHA256 of Plaintext File is Saved when Encryption is Enabled #822

Closed
fpietrosanti opened this Issue Mar 3, 2014 · 10 comments

Projects

None yet

3 participants

@fpietrosanti
Contributor

Synopsis:
The SHA256 hash of the files whistleblowers submit are saved and displayed to the
whistleblower and receivers, even when the receiver has a public key configured.

Impact: An adversary who can log in as the whistleblower or the receiver, or who gains access to the
GlobaLeaks Node's database, can check guesses about the file that was submitted. For example, if the
adversary has a list of 1000 files they suspect were submitted, they can compare the SHA256 hash of
each to find which ones (if any) were submitted.

@fpietrosanti fpietrosanti added this to the LeastAuthorityPentest milestone Mar 3, 2014
@fpietrosanti
Contributor

the feature is not so useful for the wb that is not able to verify it clientside.
we agreed that the easy fix is to remove the feature entirely.

@evilaliv3 evilaliv3 self-assigned this Mar 9, 2014
@evilaliv3 evilaliv3 added a commit to globaleaks/GLClient-outdated that referenced this issue Mar 11, 2014
@evilaliv3 evilaliv3 addressed issue globaleaks/GlobaLeaks#822 b918f6b
@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 11, 2014
@evilaliv3 evilaliv3 addressed issue globaleaks/GlobaLeaks#822 ba2736f
@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 11, 2014
@evilaliv3 evilaliv3 removed sha256sum from etag (globaleaks/GlobaLeaks#822) 617c3a7
@evilaliv3
Contributor

@defuse:

i've removed the sha256 form receivers and whistleblowers API (rtip and wbtip respectively) and from the Etag header for files download in order to address a short term remediation.

we decided to not remove the sha256 entirely from the system but simply to avoid exposing it through the network because of the following reason:

  1. the sha256 is not the only information that currently may leak some info. in fact filename and filesize need to be properly removed as well;
  2. for the current implementation the sha256 may be useful internally to do some integrity checks as for example avoiding duplicated uploads;
  3. we identified the end encryption with openpgp.js as the proper long term fix to be addressed in the future and that will solve 1)

can you please validate the fix and evaluate if this ticket can be closed?

@defuse
defuse commented Mar 17, 2014

I looked over the diffs quickly, and they look good. It's nice to see code
getting deleted!

Does the SHA256 provide any demonstrable benefit right now? If not, we think it
should be removed entirely. As far as I can tell, it's not being used for
anything at all right now, so there's no reason to keep it around.

  • Integrity should be maintained internally with proper error checking and error
    handling, or with checksums of the ciphertext, not the plaintext.
  • When client side encryption is implemented, the SHA256 will serve no purpose
    (it cannot be used to check for duplicates since all uploaded files will be
    different), so it will have to be completely removed anyway.
  • The file size and name are information leaks, but the SHA256 is considerably
    more dangerous as it is cryptographic proof that the file is what the attacker
    thinks it is, whereas the name and size are much weaker evidence.
@evilaliv3
Contributor

alliright. we perfectly agree with you.

by the way for the moment we decided to not remove the sha256 internally in the short term due to the fact that a lot of code need to be changed; anyhow it will be removed entirely in the next stable release.

do you agree that for the pentest mitigation we can consider this issue closed?

@defuse
defuse commented Mar 17, 2014

Can you point me to the code where the SHA256 is currently used? As a short term mitigation, you could just set it to a random value instead of actually hashing the plaintext.

I will ask the rest of the team what they think about closing this issue.

@evilaliv3
Contributor

allright i agree that in the midterm we can avoid also to calculate the
sha256 so that the feature will be completely removed also it an entry in
the db will continue to exist with a value of 0.

@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 18, 2014
@evilaliv3 evilaliv3 addressed issue globaleaks/GlobaLeaks#822 af527bc
@evilaliv3
Contributor

ok, i've found it easy to remove the sha256 completely =) so the ticket can be surely be closed!

@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 18, 2014
@evilaliv3 @evilaliv3 evilaliv3 + evilaliv3 addressed issue globaleaks/GlobaLeaks#822 241fe80
@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 18, 2014
@evilaliv3 @evilaliv3 evilaliv3 + evilaliv3 removed sha256sum from etag (globaleaks/GlobaLeaks#822) 98fcf0f
@evilaliv3 evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Mar 18, 2014
@evilaliv3 evilaliv3 addressed issue globaleaks/GlobaLeaks#822 9eb45e7
@evilaliv3
Contributor

damned github, due to the merge from branch 'fix_issue_822' to 'devel' it has reported this last 3 commits has been reprinted but they are the same of 6 days ago

@defuse
defuse commented Mar 26, 2014

I reviewed the diff. It looks good. I think it's safe to close this issue.

@evilaliv3
Contributor

allright thank you @defuse

@evilaliv3 evilaliv3 closed this Apr 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment