Session secret documentation does not encourage good secret management strategies

[jacksingleton]

In the readme and intro documentation, we describe how to set a session secret manually with this code block:

set :session_secret, 'super secret'

We don't mention anywhere that the value should really be pulled in from an environment variable (or possibly config file not checked into the codebase, secret store, etc).

We also don't mention how to securely generate the value.

While ideally all developers would know that this secret shouldn't be checked into code, and should be generated with a CSPRNG, frameworks have an opportunity to promote good security practices. This will simultaneously educate developers and prevent vulnerabilities.

I'd suggest that we:

1. Show code examples sourcing secrets from environment variables
2. Add an example of generating a secure random secret

[aitbw]

A good example to add would be Ruby's SecureRandom as a way to generate a secure, random session secret, like this:

ruby -e "require 'securerandom'; puts SecureRandom.hex" and then add the output to your environment.

You could also use urlsafe_base64 or uuid as an option for SecureRandom, as stated here and here.

[jacksingleton]

+1

I'm not a crypto expert -- but if my reading of rfc2104 is correct, we should recommend a key length equal to the length of the HMAC hash (by default, SHA-1, 160 bits).

SecureRandom's length parameters are in bytes, so we'd say SecureRandom.hex(20)

[aitbw]

I'm no expert either, but I think that should be the correct length, especially since a bigger value won't make your app more secure.

The RFC2104 spec also mentions that these keys should be refreshed periodically. Maybe it would be nice to create a Sinatra recipe to address this issue in a more insightful way.

[zzak]

rack-protection has been a dependency of Sinatra since (I believe) 1.3.0: 1f1e58e

I would recommend anyone to use it for session management, instead of generating your own token since this gem also protects against a number of session-based attacks.

[aitbw]

Does the use of rack-protection avoids the following?

If so, I'm sold. But it'd still be nice to encourage better security practices, especially since Sinatra has such a low learning curve and it's preferred by Ruby newcomers.

[jkowens]

To @jacksingleton's point the README does recommend setting your own session token.

To improve security, the session data in the cookie is signed with a session secret. A random secret is generated for you by Sinatra. However, since this secret will change with every start of your application, you might want to set the secret yourself, so all your application instances share it:

set :session_secret, 'super secret'

Sinatra generates a random secret by default using set :session_secret, SecureRandom.hex(64).

See: https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L1797

This isn't something rack-protection handles as far as I can tell. I agree that some documentation to describe how to generate a secure secret would be helpful and a recommendation on how to keep out of source control would be 👍

[grempe]

I would also recommend setting the session secret in an environment variable. You can retrieve it within your code using the block form of ENV.fetch which will provide a safe fallback if the specified ENV var is unavailable. If you use ENV['I_DONT_EXIST'] it will return a nil value for a var which is not set which is probably not what you want to set your session keys to.

Storing config like this in env vars is also a key tenet of 12 Factor Apps

The twelve-factor app stores config in environment variables (often shortened to env vars or env). Env vars are easy to change between deploys without changing any code; unlike config files, there is little chance of them being checked into the code repo accidentally; and unlike custom config files, or other config mechanisms such as Java System Properties, they are a language- and OS-agnostic standard.

Option 1

Throws an exception if SESSION_SECRET does not
exist in the current ENV. Use this if you want to fail fast if this critical setting is missing.

set :session_secret, ENV.fetch('SESSION_SECRET')

Option 2

Generate a secure value for SESSION_SECRET if its not
set in the current env.

# fail safe to a secure hex value if `SESSION_SECRET` is unset
require 'securerandom'
set :session_secret, ENV.fetch('SESSION_SECRET') { SecureRandom.hex(64) }

Towards Stronger Keys

I would also recommend using the Sysrandom gem to generate all keys that need to be cryptographically secure.

https://github.com/cryptosphere/sysrandom

Example:

Its exactly the same, except for installing the gem and a different require.

# gem install sysrandom

# Replace the userspace Ruby (OpenSSL) RNG with `/dev/urandom`
require 'sysrandom/securerandom'

# works with the same API, calls `/dev/urandom`
# or an appropriate OS kernel alternative
set :session_secret, ENV.fetch('SESSION_SECRET') { SecureRandom.hex(64) }

Choosing a Key Length

I think it is not desired as @jacksingleton said to reduce the random key length to 20 bytes (40 hex chars). I think you should use a secure key that is the same block size as the underlying HMAC hash function (64 bytes for SHA1) which is what is used by Rack::Session::Cookie. You should likely utilize the full 64 bytes (512 bits, 128 hex chars) that HMAC-SHA1 wants.

https://github.com/rack/rack/blob/master/lib/rack/session/cookie.rb#L108
https://github.com/rack/rack/blob/master/lib/rack/session/cookie.rb#L185

I think Sinatra is also generating a 64 byte key if one isn't provided if this is the correct spot in the code where that is happening:

sinatra/lib/sinatra/base.rb

Line 1797 in 1b0edc0

set :session_secret, SecureRandom.hex(64)

Here is some good reading on this topic courtesy of @atoponce

https://pthree.org/2016/07/29/breaking-hmac/

tl;dr

So what does this mean? It means that when choosing your HMAC keys, you should stay within one block size of bytes- 64 bytes or less for MD5, RIPEMD-128/160, SHA-1, SHA-224, SHA-256, and 128-bytes or less for SHA-384 and SHA-512. If you do this, you'll be fine.

Bonus Points

Just for kicks, here's Ruby Demo showing the same thing Aaron showed in Python in that article:

key is 65 bytes, one byte larger than the block size of SHA1. If you pass in either the 65 byte key, or the SHA1 hash of that key, you get the same HMAC result.

[33] pry(main)> key = SecureRandom.random_bytes(65)
=> "..."
[34] pry(main)> msg = SecureRandom.random_bytes(256)
=> "..."
[35] pry(main)> OpenSSL::HMAC.hexdigest(OpenSSL::Digest::SHA1.new, key, msg)
=> "2875f708782f71e1bcf23cfb088cdd85155f1773"
[36] pry(main)> OpenSSL::HMAC.hexdigest(OpenSSL::Digest::SHA1.new, OpenSSL::Digest::SHA1.new(key).digest, msg)
=> "2875f708782f71e1bcf23cfb088cdd85155f1773"

[zzak]

Sure, I am open to whatever you suggest we should add to our documentation or improve secure defaults.

Could you submit us a patch?

[zzak]

/cc #1216

[grempe]

I'm working on a docs patch.

[grempe]

I have created a PR to improve the docs with what I consider to be better security practices and incorporating other suggestions in this issue.

#1218

This PR does not address the following docs:

http://www.sinatrarb.com/intro.html#Using%20Sessions

These are largely duplicative of the README content I modified and I am not even sure where that content is being generated from. Perhaps someone can open a new issue to transfer my changes there.

[jacksingleton]

@grempe nice, that documentation looks pretty clear 👍.

on key length, linking to the breaking-hmac article might be more confusing than anything else. the article itself states that the title is clickbait, hmac is not broken, and there are no known security implications to the property they point out.

even if a key longer than the block size is used, and it is reduced to 160 bits, that is more than enough entropy to sleep well at night for the next few decades (http://security.stackexchange.com/questions/6141/amount-of-simple-operations-that-is-safely-out-of-reach-for-all-humanity/6149#6149). anyone uncomfortable about with that amount of entropy should be switching away from HMAC-SHA1

of course, a longer key won't hurt anything so I don't care if we recommend 512 bits but on line 1387 we should make it clear that the important thing is using /more/ than a certain amount of entropy, not /less/.

[grempe]

Fair point about the link @jacksingleton

I just made some tweaks to the document patch and I force overwrote my previous PR commit. I think that it is prudent to continue to recommend a 64 byte secret. The practical difference for dev/ops is almost zero so going with the stronger value is recommended.

[jacksingleton]

looks good to me! thanks for putting that together @grempe

@zzak nice to see this in the plan for 2.0, thanks for taking it seriously

[zzak]

Thanks @grempe and @jacksingleton for your patch and feedback to get this done <3