{"action":"created","issue":{"url":"https://api.github.com/repos/manastech/crystal/issues/1313","labels_url":"https://api.github.com/repos/manastech/crystal/issues/1313/labels{/name}","comments_url":"https://api.github.com/repos/manastech/crystal/issues/1313/comments","events_url":"https://api.github.com/repos/manastech/crystal/issues/1313/events","html_url":"https://github.com/manastech/crystal/pull/1313","id":103738956,"number":1313,"title":"Fix Bcrypt Int32 overflow","user":{"login":"sferik","id":10308,"avatar_url":"https://avatars.githubusercontent.com/u/10308?v=3","gravatar_id":"","url":"https://api.github.com/users/sferik","html_url":"https://github.com/sferik","followers_url":"https://api.github.com/users/sferik/followers","following_url":"https://api.github.com/users/sferik/following{/other_user}","gists_url":"https://api.github.com/users/sferik/gists{/gist_id}","starred_url":"https://api.github.com/users/sferik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sferik/subscriptions","organizations_url":"https://api.github.com/users/sferik/orgs","repos_url":"https://api.github.com/users/sferik/repos","events_url":"https://api.github.com/users/sferik/events{/privacy}","received_events_url":"https://api.github.com/users/sferik/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-08-28T15:23:41Z","updated_at":"2015-08-28T15:41:17Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/manastech/crystal/pulls/1313","html_url":"https://github.com/manastech/crystal/pull/1313","diff_url":"https://github.com/manastech/crystal/pull/1313.diff","patch_url":"https://github.com/manastech/crystal/pull/1313.patch"},"body":"While reading through the `Bcrypt` implementation, I think I found a bug: [This line](https://github.com/manastech/crystal/blob/8c28ea85011a8d24aee30e8e196194149919e820/src/crypto/bcrypt.cr#L106) performs a left shift on `1` to exponentially increase the number of times the password and salt are expanded (according to the cost). However, when the cost is `31` (`1 << 31 == 2147483648`), this overflows `Int32`, resulting in `-2147483648`. This results in zero key expansions when there should be over 2 billion. Hashing the key this many times should take days, if not weeks; instead, it takes less than a second. Users who think they’re setting the maximum cost, and therefore the highest resilience to a brute-force attack, are actually getting the minimum.\r\n\r\nSince this patch changes the code to use an `Int64` instead of an `Int32`, I’ve increased the `MAX_COST` to `62`, allowing for even greater security and future-proofing this library while not overflowing `Int64`.\r\n\r\nI couldn’t think of a good way to add a regression test for this but I’m happy to add specs if you can suggest how they should be written. The `bcrypt-ruby` library implements a `calibrate` method, used to verify that `cost` behaves as expected, but even if Crystal had implemented [their specs](https://github.com/codahale/bcrypt-ruby/blob/1f9184a8df2b90fa02d01a32a364eefc9072e1b9/spec/bcrypt/engine_spec.rb#L4-L8), it wouldn’t have caught this overflow."},"comment":{"url":"https://api.github.com/repos/manastech/crystal/issues/comments/135809949","html_url":"https://github.com/manastech/crystal/pull/1313#issuecomment-135809949","issue_url":"https://api.github.com/repos/manastech/crystal/issues/1313","id":135809949,"user":{"login":"sferik","id":10308,"avatar_url":"https://avatars.githubusercontent.com/u/10308?v=3","gravatar_id":"","url":"https://api.github.com/users/sferik","html_url":"https://github.com/sferik","followers_url":"https://api.github.com/users/sferik/followers","following_url":"https://api.github.com/users/sferik/following{/other_user}","gists_url":"https://api.github.com/users/sferik/gists{/gist_id}","starred_url":"https://api.github.com/users/sferik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sferik/subscriptions","organizations_url":"https://api.github.com/users/sferik/orgs","repos_url":"https://api.github.com/users/sferik/repos","events_url":"https://api.github.com/users/sferik/events{/privacy}","received_events_url":"https://api.github.com/users/sferik/received_events","type":"User","site_admin":false},"created_at":"2015-08-28T15:41:17Z","updated_at":"2015-08-28T15:41:17Z","body":"I thought of a way this can be tested. Please don’t merge it just yet.\r\n\r\nThanks!"}}