Add a custom SHA1 digest implementation to no longer depend on the digest gem before we know which version to activate #4989

casperisfine · 2021-10-15T10:30:53Z

Ref: https://bugs.ruby-lang.org/issues/17873

Ruby 3.1. is making digest a default gem, because of this if bundler
depends on it, it might cause already activated issues.

This currently happens if your Gemfile contains a git gem, because
the GitSource makes an SHA1 of the repository URL.

What was the end-user or developer problem that led to this PR?

Currently it is very complicated if not impossible to use a gem that has digest in its dependencies if you also have a gitst gem.

source "https://rubygems.org"

gem "net-imap"
gem "bootsnap", github: "Shopify/bootsnap"

$ ruby -v 
ruby 3.1.0dev (2021-10-06T06:42:37Z master d53493715c) [x86_64-darwin20]
$ bundle
Using msgpack 1.4.2
Using bootsnap 1.9.1 from https://github.com/Shopify/bootsnap.git
Using bundler 2.2.25
Using digest 3.0.0
Using io-wait 0.1.0
Using timeout 0.2.0
Using net-protocol 0.1.1
Using strscan 3.0.0
Using net-imap 0.2.2
Bundle complete! 2 Gemfile dependencies, 9 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
$ bundle exec ruby -r bundler/setup -r digest -e 'p :hello'
gems/bundler-2.2.25/lib/bundler/runtime.rb:309:in `check_for_activated_spec!': You have already activated digest 3.1.0.pre2, but your Gemfile requires digest 3.0.0. Prepending `bundle exec` to your command may solve this. (Gem::LoadError)
	from gems/bundler-2.2.25/lib/bundler/runtime.rb:25:in `block in setup'
	from gems/bundler-2.2.25/lib/bundler/spec_set.rb:136:in `each'
	from gems/bundler-2.2.25/lib/bundler/spec_set.rb:136:in `each'
	from gems/bundler-2.2.25/lib/bundler/runtime.rb:24:in `map'
	from gems/bundler-2.2.25/lib/bundler/runtime.rb:24:in `setup'
	from gems/bundler-2.2.25/lib/bundler.rb:149:in `setup'
	from gems/bundler-2.2.25/lib/bundler/setup.rb:10:in `block in <top (required)>'
	from gems/bundler-2.2.25/lib/bundler/ui/shell.rb:136:in `with_level'
	from gems/bundler-2.2.25/lib/bundler/ui/shell.rb:88:in `silence'
	from gems/bundler-2.2.25/lib/bundler/setup.rb:10:in `<top (required)>'
	from <internal:/opt/rubies/3.1.0-dev/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
	from <internal:/opt/rubies/3.1.0-dev/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'

What is your fix for the problem, implemented in this PR?

I found https://github.com/Solistra/ruby-digest which is under public domain and could be vendored. It conveniently implement the 3 hash algorithms used by bundler.

This is a rough first draft, it could certainly be cleaned a bit more.

@deivid-rodriguez how does this sound?

deivid-rodriguez · 2021-10-15T11:15:59Z

@casperisfine Yes, the explanation of the issue makes sense and the solution seems like the way to go.

The scary part is to start depending on a bunch of code from a random public repository. But as long as we properly audit the code, it should be fine. Did you do that?

As a side note, we could also workaround the issue by using OpenSSL::Digest instead of Digest directly, thanks to openssl not activating digest as a gem. But at ruby/digest@1617261#commitcomment-57698673 we agreed that was an openssl issue, which has already been fixed at ruby/openssl#463. So it would still be an issue if ruby 3.1 ships an openssl version with that patch. And in any case, it would just trick bundler into not detecting the problem, but you would still get redefinition warnings and your application would be using two different copies of the gem at the same time. Well, actually, you might not even get any warnings due to ruby/digest@1617261, but the problem will still be there and I believe ruby/digest@1617261 should be reverted anyways.

casperisfine · 2021-10-15T11:29:01Z

But as long as we properly audit the code, it should be fine. Did you do that?

Only very quickly, but if you think it's the way to go I can do a proper pass and even delete the parts we don't need. Overall this is all fairly well known and stable algorithms, and we can also vendor digest's test suite and run it against to make sure it works well.

As for the security risk, it's not hard to audit that it doesn't do anything funky, it's 99% bitwise operations on strings.

we could also workaround the issue by using OpenSSL::Digest i

Good point. Whatever you think is best, you have the most context here. I'll happily submit a PR for either solution. I mostly wanted to start a discussion as I really fear digest will cause a huge mess when 3.1.0 is released.

simi · 2021-10-15T11:29:19Z

@casperisfine There is one general rule - "don't roll your own crypto". Some info could be found at https://arxiv.org/abs/2107.04940 (or just google that term). I'm 👎 on this.

I tried to look around, but it is not clear to me why we can't vendor ruby/digest as we do with other gems (like fileutils). Can anyone briefly explain?

casperisfine · 2021-10-15T11:30:47Z

I tried to look around, but it is not clear to me why we can't vendor ruby/digest as we do with other gems (like fileutils).

Because it's a native gem (most of the code is in C)

deivid-rodriguez · 2021-10-15T11:31:36Z

@simi We can't really vendor C code, at least I don't know how.

casperisfine · 2021-10-15T11:32:36Z

There is one general rule - "don't roll your own crypto".

I don't disagree with the idea, but in this instance I don't think it qualifies as our own crypto. It's just a pure ruby implementation of MD5/SHA1/SHA256, it's very easy to unit test it.

It's not like we're implementing our own cryptographic algo.

simi · 2021-10-15T11:44:43Z

Just to ensure I do understand it well, is this is the problematic code doing MD5 too early?

rubygems/bundler/lib/bundler/source/rubygems/remote.rb

Line 29 in e4dcb33

uri_digest = SharedHelpers.digest(:MD5).hexdigest(uri_parts.compact.join("."))

deivid-rodriguez · 2021-10-15T11:45:43Z

Also, the particular use of hashing algorithms here (at least the one that's causing trouble), it's not really crypto-related. It's used to digest the url for git repositories into something that can be used as a file name on disk. So I think it should pose no issue if the digest could be reversed into the original uri. We could even stop using it and do something different like replacing slashes with other characters that can be used in filenames if it wasn't for backwards compatibility concerns.

simi · 2021-10-15T11:46:30Z

@deivid-rodriguez that's the way I'm currently looking as well. Remove the need of MD5 at all if not really needed.

deivid-rodriguez · 2021-10-15T11:46:41Z

We crossed messages. Yes. that's the line causing the issue.

casperisfine · 2021-10-15T12:05:51Z

if it wasn't for backwards compatibility concerns.

Yeah, that was my first though, but then I figured it wasn't possible to just change the algo.

Also re openssl, isn't OpenSSL a gem as well, wouldn't we run into the same problem exactly if we were to load it that soon?

casperisfine · 2021-10-15T12:07:12Z

As for the line causing the issue, in my case it's that one:

rubygems/bundler/lib/bundler/source/git.rb

Line 310 in c9f072a

SharedHelpers.digest(:SHA1).hexdigest(input)

deivid-rodriguez · 2021-10-15T12:11:47Z

Also re openssl, isn't OpenSSL a gem as well, wouldn't we run into the same problem exactly if we were to load it that soon?

Yes, that's what I was trying to explain earier. The only reason why bundler doesn't complain when digest is loaded through openssl is because openssl used to require digest from the C-extensions without using rubygems require, so when that happens digest is not activated as a gem and bundler is not able to detecte that the are two versions of digest loaded at the same time. But in any case, it's been fixed by ruby/openssl#463.

casperisfine · 2021-10-15T12:15:35Z

Ah, that makes sense now. Ok, so I suppose vendoring MD5/SHA1/SHA256 is the only way to go?

deivid-rodriguez · 2021-10-15T12:16:06Z

Yes, sorry, the problematic line is the one @casperisfine pointed out. The one pointed out by @simi doesn't seem to cause issues for the moment, but in any case, it's non crypto-related either.

I'm leaning towards going with the approach in this PR. We can cleanup all the unused code given the liberal license and the lack of updates, and limit this only to "url-digesting" usages (I think they might be our only usages of digest). The library can be unit tested easily as @casperisfine pointed out, and even if it had a bug, I don't see how it could become a security issue.

casperisfine · 2021-10-15T12:17:16Z

Ok, thanks. I'll cleanup this PR then, and ping back when it's in a decent state.

deivid-rodriguez · 2021-10-15T12:18:19Z

Alright, I'd like to have @simi on board here though. Hopefully our reasoning sounds good to him too.

casperisfine · 2021-10-15T12:19:41Z

What if the vendored code is in a namespace like Bundler::CryptoUnsafeDigest, would that solves the concerns of this ever be used in a crypto context?

simi · 2021-10-15T12:28:12Z

If that's related to git only, maybe we can use git to do the work for us.

echo "abc/cde" | git hash-object --stdin
52fdf764bebd1d866eba5402b89b9d6b4f1d9f53

🤔 Anyway that is still backwards incompatible. Also I wasn't able to find out how stable this hash is (if that depends on your preferred git hash function).

deivid-rodriguez · 2021-10-15T12:39:47Z

It's not really related to git specifically. For example, the usage you pointed out is to save a cache of gems for a specific rubygems (not git) source to disk. I think the general issue is that we need to save something related to some url to disk with a unique name. But it can't be the uri itself (I guess mainly because it includes slashes and other forbidden characters in filenames). So we use a hashing function. But we don't really care whether it's a hash function in the cryptographic sense (hard to reverse/decrypt), since we have nothing to hide in this case.

simi · 2021-10-15T15:27:57Z

What about to just call it path sanitizer/hasher and the fact it is MD5 algo leave hidden (mentioned in describing comment at the top of the class)?

casperisfine · 2021-10-15T15:50:51Z

Sure I can do that. Please note though that this callsite is the one currently causing problem, there might be other callsites causing problems in other situations that might be revealed later.

But OK. I'll likely resume work monday, and simply include a standalone MD5 function inside Source::Git with a comment explaining that MD5 is only used for backward compatibility, and than any simple mangling would work just as well.

casperisfine · 2021-10-21T13:07:30Z

Hum, good point. I modified the test to wrap it in a if RUBY_VERSION >= "2.7". Hopefully that's the right call and it's mergeable, otherwise let me know.

bundler/spec/runtime/setup_spec.rb

simi · 2021-10-21T14:43:13Z

It fails for 2.7 as well. Is that expected?

casperisfine · 2021-10-21T18:08:53Z

Hum, weird I thought it was a default gem in 2.7 already, but I might be wrong. I updated the version check.

simi · 2021-10-21T18:18:44Z

@casperisfine https://github.com/ruby/ruby/blob/ruby_3_0/NEWS.md#stdlib-compatibility-issues

casperisfine · 2021-10-22T08:25:49Z

Hum, Bundler under a ruby-core setup is failing too, but bot sure what it corresponds to.

deivid-rodriguez · 2021-10-22T09:07:04Z

That means the spec will fail when this PR is imported into ruby-core. Let me have a look at it.

deivid-rodriguez · 2021-10-22T12:02:08Z

@casperisfine I had a look. In the ruby-core test environment, there are no default gems installed, so dependency resolution fails because the default digest gem can't be used to provide bar's dependency on it. This is expected and the same reason why the spec fails on older rubies, so it's fine to skip the spec in ruby-core. Add the :ruby_repo tag to the spec to skip it and it should be fine 👍.

This allows `Source::Git` to no longer load the `digest` gem as it is causing issues on Ruby 3.1.

casperisfine · 2021-10-22T12:27:30Z

@deivid-rodriguez done. Thank you!

simi · 2021-10-22T14:41:39Z

Nice!

deivid-rodriguez

This is awesome, thank you so much @casperisfine. I'll try to get this released today!

casperisfine · 2021-10-25T11:10:32Z

Thanks for the merge!

deivid-rodriguez · 2021-10-25T11:11:41Z

Thanks for your work ❤️

nobu · 2021-10-25T14:44:23Z

bundler/lib/bundler/digest.rb

+        size   = string.bytesize * 8
+        buffer = string.bytes << 128
+        buffer << 0 while buffer.size % 64 != 56
+        [size].pack("Q").bytes.reverse_each {|b| buffer << b }


Why converting to native endian and reverse?
I suspect that this doesn't work on big endian platforms.
Isn't it buffer.concat([size].pack("Q>").bytes)?

Why converting to native endian and reverse?

It directly comes from the library i vendored: https://github.com/Solistra/ruby-digest/blob/d15f906caf09171f897efc74645c9e31373d7fd1/lib/ruby_digest.rb#L521

But you are right, something's off.

@nobu

As noticed by @nobu rubygems#4989 (comment) From wikipedia: https://en.wikipedia.org/wiki/SHA-1#SHA-1_pseudocode > append ml, the original message length in bits, as a 64-bit big-endian integer. `Q` is native endian, so little-endian on most modern hardware. The original code from RubyDigest reverses the bytes: https://github.com/Solistra/ruby-digest/blob/d15f906caf09171f897efc74645c9e31373d7fd1/lib/ruby_digest.rb#L521 But that makes the code non-portable, the correct way is to directly ask for a big-endian representation.

@nobu

As noticed by @nobu rubygems/rubygems#4989 (comment) From wikipedia: https://en.wikipedia.org/wiki/SHA-1#SHA-1_pseudocode > append ml, the original message length in bits, as a 64-bit big-endian integer. `Q` is native endian, so little-endian on most modern hardware. The original code from RubyDigest reverses the bytes: https://github.com/Solistra/ruby-digest/blob/d15f906caf09171f897efc74645c9e31373d7fd1/lib/ruby_digest.rb#L521 But that makes the code non-portable, the correct way is to directly ask for a big-endian representation. rubygems/rubygems@ba2be01ea4

Add a custom SHA1 digest implementation to no longer depend on the digest gem before we know which version to activate (cherry picked from commit e75f245)

bundler/lib/bundler/digest.rb

bundlerbot added the Bundler label Oct 15, 2021

casperisfine force-pushed the fix-digest-dependency branch 4 times, most recently from 7aa6a43 to e4dcb33 Compare October 15, 2021 10:52

casperisfine force-pushed the fix-digest-dependency branch 3 times, most recently from dfc2777 to 2220385 Compare October 15, 2021 15:26

casperisfine force-pushed the fix-digest-dependency branch from da7eb7e to c024b7e Compare October 21, 2021 13:06

simi reviewed Oct 21, 2021

View reviewed changes

bundler/spec/runtime/setup_spec.rb Outdated Show resolved Hide resolved

casperisfine force-pushed the fix-digest-dependency branch from c024b7e to bce2b7c Compare October 21, 2021 13:21

casperisfine force-pushed the fix-digest-dependency branch from bce2b7c to 5bcb1d7 Compare October 21, 2021 18:08

Vendor a pure ruby implementation of SHA1

c19a9f2

This allows `Source::Git` to no longer load the `digest` gem as it is causing issues on Ruby 3.1.

casperisfine force-pushed the fix-digest-dependency branch from 5bcb1d7 to c19a9f2 Compare October 22, 2021 12:27

simi requested a review from deivid-rodriguez October 23, 2021 20:13

deivid-rodriguez changed the title ~~Vendor RubyDigest::SHA1 to no longer depends on the digest gem before we know which version to activate~~ Add a custom SHA1 digest implementation to no longer depend on the digest gem before we know which version to activate Oct 25, 2021

deivid-rodriguez added the bundler: enhancement label Oct 25, 2021

deivid-rodriguez approved these changes Oct 25, 2021

View reviewed changes

deivid-rodriguez merged commit e75f245 into rubygems:master Oct 25, 2021

nobu reviewed Oct 25, 2021

View reviewed changes

casperisfine mentioned this pull request Oct 25, 2021

Fix Bundler::Digest#sha1 on big-endian systems #5011

Merged

nobu reviewed Jun 2, 2022

View reviewed changes

bundler/lib/bundler/digest.rb Show resolved Hide resolved

nobu mentioned this pull request Jun 2, 2022

As + is higher precedence than &, this equals SHA1_MASK & (SHA1_MASK & (rotate(a, 5) + f + e + k + w[I])). #5591

Closed

deivid-rodriguez mentioned this pull request Jun 9, 2022

Refine digest #5592

Closed

4 tasks

Add a custom SHA1 digest implementation to no longer depend on the digest gem before we know which version to activate #4989

Add a custom SHA1 digest implementation to no longer depend on the digest gem before we know which version to activate #4989

Conversation

casperisfine commented Oct 15, 2021 • edited Loading

What was the end-user or developer problem that led to this PR?

What is your fix for the problem, implemented in this PR?

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

simi commented Oct 15, 2021 • edited Loading

casperisfine commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

simi commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021 • edited Loading

simi commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

casperisfine commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

casperisfine commented Oct 15, 2021

simi commented Oct 15, 2021

deivid-rodriguez commented Oct 15, 2021

simi commented Oct 15, 2021 • edited Loading

casperisfine commented Oct 15, 2021

casperisfine commented Oct 21, 2021

simi commented Oct 21, 2021

casperisfine commented Oct 21, 2021

simi commented Oct 21, 2021

casperisfine commented Oct 22, 2021

deivid-rodriguez commented Oct 22, 2021

deivid-rodriguez commented Oct 22, 2021

casperisfine commented Oct 22, 2021

simi commented Oct 22, 2021

deivid-rodriguez left a comment

Choose a reason for hiding this comment

casperisfine commented Oct 25, 2021

deivid-rodriguez commented Oct 25, 2021

nobu Oct 25, 2021

Choose a reason for hiding this comment

casperisfine Oct 25, 2021

Choose a reason for hiding this comment

casperisfine commented Oct 15, 2021 •

edited

Loading

simi commented Oct 15, 2021 •

edited

Loading

deivid-rodriguez commented Oct 15, 2021 •

edited

Loading

simi commented Oct 15, 2021 •

edited

Loading