SHA1 collisions in key names was more exploitable than I thought
Yesterday's SHA1 collision attack could be used to generate eg: SHA256-sfoo--whatever.good SHA256-sfoo--whatever.bad Such that they collide. A repository with the good one could have the bad one swapped in and signed commits would still verify. I've already mitigated this.
This commit is contained in:
parent
27eca014be
commit
6b52fcbb7e
3 changed files with 10 additions and 13 deletions
|
@ -33,8 +33,9 @@ git-annex (6.20170215) UNRELEASED; urgency=medium
|
||||||
to wget, since curl is able to display only errors to stderr, unlike
|
to wget, since curl is able to display only errors to stderr, unlike
|
||||||
wget.
|
wget.
|
||||||
* status: Pass --ignore-submodules=when option on to git status.
|
* status: Pass --ignore-submodules=when option on to git status.
|
||||||
* Tighten key parser to mitigate against hypothetical SHA1 chosen-prefix
|
* Tighten key parser to prevent SHA1 collision attacks generating
|
||||||
attacks. This ensures that signed git commits of annexed files
|
two keys that have the same SHA1. (Only done for keys that contain
|
||||||
|
a hash). This ensures that signed git commits of annexed files
|
||||||
will remain secure, as long as git-annex is using a secure hashing
|
will remain secure, as long as git-annex is using a secure hashing
|
||||||
backend.
|
backend.
|
||||||
|
|
||||||
|
|
5
Key.hs
5
Key.hs
|
@ -127,12 +127,11 @@ file2key s
|
||||||
| otherwise = Nothing
|
| otherwise = Nothing
|
||||||
|
|
||||||
{- When a key HasExt, the length of the extension is limited in order to
|
{- When a key HasExt, the length of the extension is limited in order to
|
||||||
- mitigate against SHA1 collision attacks (specifically, chosen-prefix
|
- mitigate against SHA1 collision attacks.
|
||||||
- attacks).
|
|
||||||
-
|
-
|
||||||
- In such an attack, the extension of the key could be made to contain
|
- In such an attack, the extension of the key could be made to contain
|
||||||
- the collision generation data, with the result that a signed git commit
|
- the collision generation data, with the result that a signed git commit
|
||||||
- including such keys would not be secure.
|
- including such keys would not be secure.
|
||||||
-
|
-
|
||||||
- The maximum extension length ever generated for such a key was 8
|
- The maximum extension length ever generated for such a key was 8
|
||||||
- characters; 20 is used here to give a little future wiggle-room.
|
- characters; 20 is used here to give a little future wiggle-room.
|
||||||
|
|
|
@ -74,13 +74,10 @@ A few other potential problems:
|
||||||
* `*E` backends could embed sha1 collision data in a long filename
|
* `*E` backends could embed sha1 collision data in a long filename
|
||||||
extension in a key.
|
extension in a key.
|
||||||
|
|
||||||
Impact is limited, because even if an attacker does this, the key also
|
The recent SHA1 common-prefix attack could be used to exploit this;
|
||||||
contains the checksum (eg SHA2) of the annexed data. The current SHA1
|
the result would be two keys that have the same SHA1.
|
||||||
attack is only a common-prefix attack; it does not allow creating two
|
|
||||||
colliding keys that contain two different SHA2 checksums. That would
|
This can be fixed by limiting the length
|
||||||
need a chosen-prefix attack.
|
|
||||||
|
|
||||||
It might be worth limiting the length
|
|
||||||
of an extension allowed in such a key to the longest such extension
|
of an extension allowed in such a key to the longest such extension
|
||||||
git-annex has ever supported (probably < 20 bytes or so), which would
|
git-annex has ever supported (probably < 20 bytes or so), which would
|
||||||
be less than the size of the data needed for current SHA1 collision
|
be less than the size of the data needed for current SHA1 collision
|
||||||
|
@ -92,7 +89,7 @@ A few other potential problems:
|
||||||
Need to review the code and see if such extra fields are allowed.
|
Need to review the code and see if such extra fields are allowed.
|
||||||
|
|
||||||
Update: All fields are numeric, but could contain arbitrary data
|
Update: All fields are numeric, but could contain arbitrary data
|
||||||
after the number. Could have been used in a chosen-prefix attack.
|
after the number. Could have been used in a common-prefix attack.
|
||||||
This has been fixed; git-annex refuses to parse
|
This has been fixed; git-annex refuses to parse
|
||||||
such fields, so it won't work with files that try to exploit this.
|
such fields, so it won't work with files that try to exploit this.
|
||||||
|
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue