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

name_generator uses a fixed, insecure hash algorithm (SHA1) #26

Closed
jeking3 opened this Issue Sep 5, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@jeking3
Collaborator

jeking3 commented Sep 5, 2017

Earlier this year Google announced that they had produced the first SHA1 collision:

https://security.googleblog.com/2017/02/announcing-first-sha1-collision.html

The boost::uuid library maintains a SHA1 implementation today for purposes of the name_generator, which takes any arbitrary sized set of data and generates a UUID from it. The library is hardcoded to use the SHA1 hash algorithm today.

The proposed improvement here is to make the hash algorithm configurable. A new HashingProvider concept would be introduced (or at least discussed) and/or a pure virtual base specifying the required methods an algorithm would need to implement (process_byte, process_bytes, get_digest); modify name_generator to be a template class where the hash algorithm is supplied as a template parameter, and checked to be a base of this virtual class at compile time such that a usable error message would occur.

To maintain backwards compatibility a default template parameter leveraging the existing SHA1 hash algorithm would be introduced, however it would be deprecated, and eventually the default would be removed thus forcing all use of name_generator to specify the hash algorithm as a mechanism to gently move them away from the less secure SHA1 to something else.

It may also be useful to move hashing to its own library where the HashingProvider concept could live, similar to how the UniformRandomNumberGenerator concept lives in boost::random.

@jeking3 jeking3 self-assigned this Sep 6, 2017

@Lastique

This comment has been minimized.

Show comment
Hide comment
@Lastique

Lastique Sep 7, 2017

Member
  1. The UUID generation from name is described in RFC4122, which specifies MD5 or SHA1. I'm not sure the current implementation fully follows the spec, but I think it should be used as the reference and the default.
  2. If you still want to make the hash algorithm configurable, I would prefer an implementation without virtual functions. They are not required if you follow the templated approach - just define the concept of the hashing function and use the template argument accordingly.
  3. In order to not break backward compatibility I would suggest implementing the configurable version of name_generator under a different name, like basic_name_generator or something. name_generator can be made a typedef to this new implementation.
Member

Lastique commented Sep 7, 2017

  1. The UUID generation from name is described in RFC4122, which specifies MD5 or SHA1. I'm not sure the current implementation fully follows the spec, but I think it should be used as the reference and the default.
  2. If you still want to make the hash algorithm configurable, I would prefer an implementation without virtual functions. They are not required if you follow the templated approach - just define the concept of the hashing function and use the template argument accordingly.
  3. In order to not break backward compatibility I would suggest implementing the configurable version of name_generator under a different name, like basic_name_generator or something. name_generator can be made a typedef to this new implementation.
@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Sep 7, 2017

Collaborator

@Lastique Thank you for your comments.

On item 1, hashes, given SHA1 has recently been compromised, it would make sense to start planning for a replacement. This actually got me thinking as to whether a new IETF document with all the published errata and the addition of another hashing algorithm (or two?) would be warranted.

On item 2, I think it would be an interesting exercise to do what you suggest, and add MD5 as an option. Obviously there will be little to no use of it, but it will complete the implementation with respect to the RFC. I like your strategy in item 3 as well.

Collaborator

jeking3 commented Sep 7, 2017

@Lastique Thank you for your comments.

On item 1, hashes, given SHA1 has recently been compromised, it would make sense to start planning for a replacement. This actually got me thinking as to whether a new IETF document with all the published errata and the addition of another hashing algorithm (or two?) would be warranted.

On item 2, I think it would be an interesting exercise to do what you suggest, and add MD5 as an option. Obviously there will be little to no use of it, but it will complete the implementation with respect to the RFC. I like your strategy in item 3 as well.

@mclow

This comment has been minimized.

Show comment
Hide comment
@mclow

mclow Sep 7, 2017

Contributor

I can ask an IETF co-worker of mine about future work on RFC4122 (or a followup RFC).

As a design comment, I agree with @Lastique that having a "hash function" as a template argument rather than using inheritance is a better way to go. [ See the unordered containers for examples. ]

Contributor

mclow commented Sep 7, 2017

I can ask an IETF co-worker of mine about future work on RFC4122 (or a followup RFC).

As a design comment, I agree with @Lastique that having a "hash function" as a template argument rather than using inheritance is a better way to go. [ See the unordered containers for examples. ]

@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Sep 7, 2017

Collaborator

@mclow much appreciated - thank you for taking the time do check on that.

Collaborator

jeking3 commented Sep 7, 2017

@mclow much appreciated - thank you for taking the time do check on that.

@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Sep 22, 2017

Collaborator

@mclow were you able to make any progress on a RFC4122 follow-up discussion?

Collaborator

jeking3 commented Sep 22, 2017

@mclow were you able to make any progress on a RFC4122 follow-up discussion?

@mclow

This comment has been minimized.

Show comment
Hide comment
@mclow

mclow Sep 22, 2017

Contributor

I believe that the IETF people are discussing it - but no resolution AFAIK yet.

Contributor

mclow commented Sep 22, 2017

I believe that the IETF people are discussing it - but no resolution AFAIK yet.

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 23, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 24, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 24, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

This fixes boostorg#26
@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Sep 25, 2017

Collaborator

@mclow I reached out by email to the original authors in RFC 4122 however paulle and rsalz emails bounced.

Collaborator

jeking3 commented Sep 25, 2017

@mclow I reached out by email to the original authors in RFC 4122 however paulle and rsalz emails bounced.

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 25, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 25, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 26, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule to isolate each header as an include and make sure it
has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 26, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule to isolate each header as an include and make sure it
has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 26, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 26, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 26, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Sep 28, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

jeking3 added a commit to jeking3/uuid that referenced this issue Oct 9, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes boostorg#26

@jeking3 jeking3 closed this in #36 Oct 10, 2017

jeking3 added a commit that referenced this issue Oct 10, 2017

refactor name_generator to have a configurable hash algorithm
and provide backwards compatibility for sha1, and also added
md5 to complete the RFC 4122 spec implementation

as such, boost::uuids::name_generator is deprecated, however
still defined to use the same implementation in previous boost
releases, and name_generator_sha1 as well as name_generator_md5
now exist, in preparation for whatever will replace sha1.

to properly test the new header structure, I took the bjam
rule from winapi to isolate each header as an include and make sure
it has no dependencies on other headers to be included first,
and was able to remove a few test files that became unnecessary.

This fixes #26

@jeking3 jeking3 added the enhancement label Oct 18, 2017

@jeking3 jeking3 added this to the v1.66.0 milestone Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment