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

Avoid leaking information in GPG key creation and expiration dates #3994

Conversation

garrettr
Copy link
Contributor

@garrettr garrettr commented Dec 17, 2018

Status

Ready for review

Description of Changes

Fixes #3912.

Changes proposed in this pull request:

  1. Use a fixed date for the creation time of each source's GPG reply key, to avoid leaking the approximate date at which they first created a SecureDrop account, per discussion in Expiration date on PGP key leaks date of source's first contact #3912 (comment).
  2. Reply keypairs should not expire, per discussion in Expiration date on PGP key leaks date of source's first contact #3912 (comment)

Testing

I added a unit test, tests/test_crypto_util.py::test_genkeypair_should_use_obfuscated_creation_and_expiration_dates.

I confirmed the test fails on develop and passes with the changes made in this PR.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
    Filed Update existing source reply keys to prevent leaking timing information #3995 to address this issue.

  2. New installs.
    No special considerations that I can think of. I don't know of any places where we use the creation/expiration dates of the reply keys in SecureDrop, so I think this should be a fairly low-risk change.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

I was unable to run make ci-lint on macOS, got an error:

devops/scripts/dev-shell-ci run make --keep-going lint typelint
devops/scripts/dev-shell-ci: line 23: sha256sum: command not found
make: *** [ci-lint] Error 127

File #3996 to address this issue.

- All source reply keys now have a fixed creation date, which is before
any actual sources were using the platform.
- Source reply keys no longer expire.
@garrettr garrettr force-pushed the issue/3912-reply-key-expiration-date-leak branch from 66196dc to b971d0e Compare December 17, 2018 05:38
Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially this looks good. I tried exporting the public and private key to run them through openssl asn1parse to inspect everything, but openssl yells and says that it's invalid ANS1 (yes, checked PEM/DER flags). I'd like someone who remembers how to pick apart the PGP packets to take a look to make sure there's not a date that's embedded in there somewhere that we're not considering.



def test_reply_keypair_creation_and_expiration_dates(source_app):
# TODO: setup copied from test_genkeypair. DRY?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to not be super DRY about this since unit tests also serve as documentaiton.

@garrettr
Copy link
Contributor Author

I'd like someone who remembers how to pick apart the PGP packets to take a look to make sure there's not a date that's embedded in there somewhere that we're not considering.

@heartsucker Good idea. Have you tried https://github.com/kazu-yamamoto/pgpdump?

@garrettr
Copy link
Contributor Author

Here's an example pgpdump of a key created with the code in this PR. LGTM!

Old: Public Key Packet(tag 6)(525 bytes)
	Ver 4 - new
	Public key creation time - Mon May 13 17:00:00 PDT 2013
	Pub alg - RSA Encrypt or Sign(pub 1)
	RSA n(4096 bits) - ...
	RSA e(17 bits) - ...
Old: User ID Packet(tag 13)(124 bytes)
	User ID - Autogenerated Key <WVK6TYFMYL4DLAZDHVUAQODLF6MX3DTKE7SHWFUV55LSYEGXZSKLCXVS5ON43GCDDW3IM2RBP2ZZ4EKH5IYFVJNE3ZO573UJ2S5CWGY=>
Old: Signature Packet(tag 2)(569 bytes)
	Ver 4 - new
	Sig type - Positive certification of a User ID and Public Key packet(0x13).
	Pub alg - RSA Encrypt or Sign(pub 1)
	Hash alg - SHA512(hash 10)
	Hashed Sub: signature creation time(sub 2)(4 bytes)
		Time - Mon May 13 17:00:00 PDT 2013
	Hashed Sub: key flags(sub 27)(1 bytes)
		Flag - This key may be used to certify other keys
		Flag - This key may be used to sign data
		Flag - This key may be used to encrypt communications
		Flag - This key may be used to encrypt storage
		Flag - This key may be used for authentication
	Hashed Sub: preferred symmetric algorithms(sub 11)(6 bytes)
		Sym alg - AES with 256-bit key(sym 9)
		Sym alg - AES with 192-bit key(sym 8)
		Sym alg - AES with 128-bit key(sym 7)
		Sym alg - CAST5(sym 3)
		Sym alg - Triple-DES(sym 2)
		Sym alg - IDEA(sym 1)
	Hashed Sub: preferred hash algorithms(sub 21)(5 bytes)
		Hash alg - SHA256(hash 8)
		Hash alg - SHA1(hash 2)
		Hash alg - SHA384(hash 9)
		Hash alg - SHA512(hash 10)
		Hash alg - SHA224(hash 11)
	Hashed Sub: preferred compression algorithms(sub 22)(3 bytes)
		Comp alg - ZLIB <RFC1950>(comp 2)
		Comp alg - BZip2(comp 3)
		Comp alg - ZIP <RFC1951>(comp 1)
	Hashed Sub: features(sub 30)(1 bytes)
		Flag - Modification detection (packets 18 and 19)
	Hashed Sub: key server preferences(sub 23)(1 bytes)
		Flag - No-modify
	Sub: issuer key ID(sub 16)(8 bytes)
		Key ID - 0x90751BED3A92782E
	Hash left 2 bytes - 51 b7
	RSA m^d mod n(4094 bits) - ...
		-> PKCS-1

@heartsucker
Copy link
Contributor

@garrettr I haven't seen that. Thanks for the recommendation 😄

@heartsucker
Copy link
Contributor

Tests need to be kicked, but this is good to merge. For whoever merges, we need to open the followup ticket to migrate keys on old instances to erase the expiration date.

@codecov-io
Copy link

Codecov Report

Merging #3994 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3994      +/-   ##
===========================================
+ Coverage    84.67%   84.69%   +0.01%     
===========================================
  Files           43       43              
  Lines         2760     2763       +3     
  Branches       299      299              
===========================================
+ Hits          2337     2340       +3     
  Misses         355      355              
  Partials        68       68
Impacted Files Coverage Δ
securedrop/securedrop/crypto_util.py 96.22% <0%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8003155...3be51e0. Read the comment docs.

@redshiftzero
Copy link
Contributor

This looks good to me, @emkll take a peek at this and let me know if you are cool with this change (doesn't need to be a full review, just so you are aware)

Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the PR @garrettr ! Used pgpdump and confirmed that the creation/expiry dates are working as expected, and no other metadata. #3995 was already opened for follow-up (thanks!).

A couple of comments, for which I will open follow-up tickets:

  • Xenial will be using, gnupg 2.1, which uses different format for private key storage (https://gnupg.org/faq/whats-new-in-2.1.html): the metadata of the private key file on disk might need to be modified as well.
  • Output of pgpdump made me notice that keys generated have certify/sign/authenticate capabilities, which are not used. Since they are stored in an application-specific keyring that doesn't strike me as an immediate problem however.

@emkll emkll merged commit ad7e7b2 into freedomofpress:develop Dec 19, 2018
@garrettr garrettr deleted the issue/3912-reply-key-expiration-date-leak branch December 27, 2018 08:25
@emkll emkll mentioned this pull request Feb 19, 2019
17 tasks
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

Successfully merging this pull request may close these issues.

Expiration date on PGP key leaks date of source's first contact
5 participants