Add a tree sha512 hash to merge commits #9871
Conversation
|
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 ***@***.***> 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 | sha512sum` in
the repository root.
You can view, comment on, or merge this pull request online at:
#9871
-- 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:
#9871
|
| intern.update(piece) | ||
| else: | ||
| break | ||
| fi.close() |
There was a problem hiding this comment.
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
|
@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 ***@***.***> 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:
#9871 (comment)
|
|
@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).
|
|
@sipa Nope, that's simply not how git works. The commit object itself is what's piped to GnuPG, not just the hash of that object. |
|
@petertodd I'm aware. But GnuPG then takes a SHA1 hash of that. It's hardcoded by git to use SHA1, I believe. |
|
@sipa GnuPG uses whatever hash algorithm you've told it to use; defaults used to be SHA1, but that's both easy to change, and IIRC the default is now SHA256. EDIT: Just checked with 1.4.18, default is now SHA256. |
|
@petertodd I don't think so (but I'd be glad to be wrong). Look at the actual data with |
|
@sipa It's missing because it's not a clearsigned signature; it's an ascii encoded detached signature. |
|
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? |
|
@dcousens They're talking about the equivalent of segwit vs how Bitcoin works now; it's not relevant to what we're talking about here. |
|
@petertodd no worries, did FWIW, I also saw https://grimoire.ca/git/detached-sigs |
|
@dcousens The "detached sigs" your links are referring to isn't the same thing as the "detached sigs" we're talking about above. |
|
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. |
sipa
changed the title
|
I'm not sure how this works: 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). |
|
@laanwj it does that command to grab the list of files. Then it opens each file in the list and hashes it. |
Oh, then things are alright again, thanks :) Going to test this. |
| except subprocess.CalledProcessError as e: | ||
| printf("ERROR: Cannot update message.",file=stderr) | ||
| exit(4) | ||
| second_sha512 = tree_sha512sum() |
There was a problem hiding this comment.
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 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? |
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" ***@***.***> 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:
#9871 (comment)
|
|
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. |
fa89670 Add SHA512 tree hash to merge commits (Pieter Wuille) Tree-SHA512: 72321597336d3c4957719c8b907f258814b01499a82d2bc1e8c678b8825461d95f23b42ff6868a25725f4bfc3da24f7b12c058b45cbc7a7dfbf668888b68274e
fa89670 Add SHA512 tree hash to merge commits (Pieter Wuille) Tree-SHA512: 72321597336d3c4957719c8b907f258814b01499a82d2bc1e8c678b8825461d95f23b42ff6868a25725f4bfc3da24f7b12c058b45cbc7a7dfbf668888b68274e
11 participants
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 | sha512sumin the repository root.