This adds a SHA512 hash of the resulting merged tree to merge commit messages. Assuming gpg uses a sufficiently strong hash internally, this results in our gpg signed commits avoiding any potential SHA1 issues w.r.t. the commit's resulting contents (but NOT its history).
The hash can be verified by running git ls-tree --full-tree -r --name-only HEAD | LANG=C sort | xargs -n 1 sha512sum | sha512sum in the repository root.
Given how few lines of code this requires and how easy it is to independently calculate, concept ACK as a quick fix.
Creating a 'git-sha512-treehashes.txt' file in the root folder of the repo. That is a list of every sha512 tree hash for every merge commit (in this reop history).
Each new merge commit should append it's sha512-tree-hash to this file. This would make falsifying sha1 root hash far-harder as modifying any file will update the root-hash from two different locations.
Hmm, what we really need to do is have a second PGP signature over just this hash in the commit message that can be verified completely independently from git.
On February 26, 2017 8:31:24 PM EST, Pieter Wuille notifications@github.com wrote:
This adds a SHA512 hash of the resulting merged tree to merge commit messages. This may or may not make SHA1 collision attacks harder (since the GPG signatures sign a SHA1 hash of the commit's contents, this is not a final solution at all).
The hash can be verified by running
git ls-tree --full-tree -r --name-only HEAD | LANG=C sort | xargs -n 1 sha512sum | sha512sumin the repository root. You can view, comment on, or merge this pull request online at:-- Commit Summary --
- Make TestBlockValidity optional in CreateNewBlock
- Support construction of multiple subsequent block templates
- Abstract out BlockAssembler options
- Make CNB validity test an option
- Add SHA512 tree hash to merge commits
-- File Changes --
M contrib/devtools/github-merge.py (46) M qa/rpc-tests/segwit.py (6) M src/init.cpp (3) M src/miner.cpp (66) M src/miner.h (13) M src/test/miner_tests.cpp (72) M src/validation.cpp (1) M src/validation.h (2)
-- Patch Links --
https://github.com/bitcoin/bitcoin/pull/9871.patch https://github.com/bitcoin/bitcoin/pull/9871.diff
-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9871
80 | + piece = fi.read(65536) 81 | + if piece: 82 | + intern.update(piece) 83 | + else: 84 | + break 85 | + fi.close()
Nit: use with instead of open/close:
with open(f, 'rb') as fi:
while True:
piece = fi.read(65536)
if piece:
intern.update(piece)
else:
break
utACK fa89670
@TheBlueMatt The PGP signature on a git commit signs the entire commit object, including the message and thus SHA512 hash. So the SHA512 tree hash can be verified directly; you don't need another PGP signature.
@petertodd oh? IIRC it just signs a few SHA1 hashes, not the message directly.
On February 26, 2017 11:37:48 PM EST, Peter Todd notifications@github.com wrote:
@TheBlueMatt The PGP signature on a git commit signs the entire commit object, including the message and thus SHA512 hash. So the SHA512 tree hash can be verified directly; you don't need another PGP signature.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9871#issuecomment-282626312
@TheBlueMatt Nope, look at my OpenTimestamps code to timestamp git commits: https://github.com/opentimestamps/opentimestamps-client/blob/master/ots-git-gpg-wrapper#L87
It signs the SHA1 hash of the entire commit object (excluding the signature, of course).
@petertodd I'm aware. But GnuPG then takes a SHA1 hash of that. It's hardcoded by git to use SHA1, I believe.
@petertodd I don't think so (but I'd be glad to be wrong). Look at the actual data with git cat-file -p <commit>. It does not include the otherwise mandatory hash type line, so I assume gpg prepends that before verification. That can't work if it's anything but the default. git does not use detached signatures.
Hmm, I read conflicting information. I hope I'm wrong.
@sipa Don't worry, you are wrong. :)
Like I said above, look at the git integration code I linked above from the OpenTimestamps client. Notably, the command line flags -bsau are passed to gpg by git when signing, which creates an ascii-encoded detached signature.
It doesn't look like Git uses detached signatures?
@petertodd no worries, did git change since that SO question or was your comment noting that the GPG signing process used here (in bitcoin/bitcoin) is different?
FWIW, I also saw https://grimoire.ca/git/detached-sigs
Cool, so this is much more useful than I thought :)
So as long as the merge commit signers use SHA512 (or something like that, maybe SHA256) for GPG this solution is essentially perfect (besides git upgrading to a new hash function).
And for those who care, this is where git calls gpg for signing.
I'm not sure how this works: git ls-tree --full-tree -r --name-only HEAD returns a list of filenames. Is it enough assurance to hash that, you don't need the file contents?
Travis fails are, as usual, unrelated.
I read in the actual files, and hash those, and then hash the result as like the result of sha512sum (look at the command line).
I read in the actual files, and hash those, and then hash the result as like the result of sha512sum (look at the command line).
Oh, then things are alright again, thanks :)
Going to test this.
210 | + try: 211 | + subprocess.check_call([GIT,'commit','--amend','-m',message.encode('utf-8')]) 212 | + except subprocess.CalledProcessError as e: 213 | + printf("ERROR: Cannot update message.",file=stderr) 214 | + exit(4) 215 | + second_sha512 = tree_sha512sum()
Why is the above call to tree_sha512sum() guarded by a try except clause and this one is not?
Anyway, I think the right place to put the second check would be verify-commits.sh.
Concept ACK
utACK fa89670d34ac7839e7e2fe6f59fdfd6cc76c4483
utACK fa89670
We should probably have some better handling for symlinks, no?
@TheBlueMatt I think that may be overkill, but ok... how would you construct something that's easily shell-verifiable?
We should probably have some better handling for symlinks, no?
What about just rejecting symlinks? We don't use them in this repository and probably shouldn't as not all OSes have that concept.
@sipa not sure about shell-verifiable, but git-cat has some symlinks options. I'm more than happy to reject them wholesale, however, given the dubious value and obvious annoyance/risk.
On February 28, 2017 5:53:15 AM EST, "Wladimir J. van der Laan" notifications@github.com wrote:
We should probably have some better handling for symlinks, no?
What about just rejecting symlinks? We don't use them in this repository and probably shouldn't as not all OSes have that concept.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9871#issuecomment-283007962
ACK to rejecting symlinks.
We don't need to go overkill here and make this a scheme every project could adopt.
I'm just going to merge this - this is a nice and simple change that alleviates the direct worries about SHA1 compromise. More advanced things or functionality to reject symlinks can be added to the script later.