Secure by default sessions #2200
Conversation
|
|
||
| $self->log->trace("Your secret passphrase has been set to strong random value and stored in $file"); | ||
|
|
||
| return [$secret]; |
There was a problem hiding this comment.
While I think this is generally a good and necessary step, this will invalidate session cookies in currently insecure apps so that will need to at minimum be noted in the changelog (which is not the responsibility of this PR).
kraih
requested review from
a team,
marcusramberg,
kraih,
christopherraa and
Grinnz
|
Make sure to squash your comits when you are ready. |
|
Thanks! I'm addressing all feedback, and will update the PR tomorrow or so. |
lib/Mojo/Util.pm
Outdated
| require Win32::API; | ||
|
|
||
| my $RtlGenRandom = Win32::API->new('advapi32', 'INT SystemFunction036 (PVOID RandomBuffer, ULONG RandomBufferLength)') | ||
| or croak "SystemFunction036 import failed: $!"; |
There was a problem hiding this comment.
Is this actually used in our Windows CI tests? This seems like code nobody on the team can maintain long term and we will end up removing it because of that the first it causes problems.
There was a problem hiding this comment.
Tests are running SystemFunction036 (RtlGenRandom) on Windows to get random bytes. It's used in the same way as for example Crypt::URandom. However, It is a legacy API, so we can consider using ProcessPrng instead.
Here are some discussions in rust and golang:
There was a problem hiding this comment.
This part of the code will probably end up being a blocker, since i don't see anyone on the team agreeing to take responsibility for maintaining it in the future.
There was a problem hiding this comment.
I'd not expect finding such code in web framework. In the same time, AFAIK the alternatives are:
- using some small old library with single maintainer and with no recent releases, that does exactly this call, but conveniently wrapped,
- overkill of making Net-SSLeay a dependency to get it through openssl binding.
There was a problem hiding this comment.
Net::SSLeay is already a soft dependency, maybe there's a clean way to make this feature optional? A hard dependency is pretty much out of the question. It's unfortunate Perl doesn't ship with some kind of OpenSSL binding, for mojo.js we've chosen a different solution and just encrypt all sessions because Node has crypto support.
There was a problem hiding this comment.
Crypt::URandom has a pile of different window support calls. Adding that as an optional dependency for windows enjoyers might unblock this, while outsourcing the windows support
There was a problem hiding this comment.
Crypt::URandom appears to sets up some context for CryptGenRandom for Win2k, and uses RtlGenRandom for later Windows versions.
According to a discussion in golang crypto/rand, it seems fine to use RtlGenRandom (SystemFunction036) golang/go#33542 (comment)
We could use Crypt::URandom as a soft dependency for Win and Linux/UNIX, and fail over to /dev/urandom if that module is not installed? (And fatal on Windows maybe)
There was a problem hiding this comment.
What about something like this?
use constant CRYPT_URANDOM => !!eval { require Crypt::URandom };
...
sub urandom_bytes {
my $num = shift || 32;
return Crypt::URandom::urandom($num) if CRYPT_URANDOM;
croak 'Cannot find /dev/urandom. On Windows? Try installing "Crypt::URandom" from CPAN'
unless -f '/dev/urandom';
open(my $urandom, '<', '/dev/urandom') or croak "Cannot open /dev/urandom: $!";
sysread($urandom, my $bytes, $num) == $num or die "sysread() from /dev/urandom didn't return $num bytes";
close($urandom);
return $bytes;
}
There was a problem hiding this comment.
That's exactly the kind of shim(affectionate) I was suggesting!
You might not need to mention windows directly in the message, you know, just to be inclusive of that one person who runs mojo apps on VAX or AIX
There was a problem hiding this comment.
I've updated the PR with logic similar to the above, avoiding any Win32 specific code.
Also added Crypt::URandom to the Windows GH actions workflow, should the be in Makefile.PL instead?
stigtsp
force-pushed
the
secure-by-default-sessions
branch
3 times, most recently
from
c33c0eb to
6bd013a
Compare
|
I've (hopefully) addressed all comments and pushed a single commit to this PR. One problem that remains is that a |
| # Read secrets and filter out those who are less than 22 characters long | ||
| # (~128 bits), as they are not likely to be sufficiently strong. | ||
| my @secrets = grep { length $_ >= 22 } split /\n/, path($file)->slurp; |
There was a problem hiding this comment.
Should there be a warning message if a candidate secret is skipped? Right now, they're discarded silently.
There was a problem hiding this comment.
Yes - or maybe even a fatal error?
Btw, I'm not sure how useful more than one key in app->secrets is. With a strong secret, wouldn't rotation only makes sense if it is compromised/leaked/etc? And then you wouldn't want to continue trusting the old key imho.
There was a problem hiding this comment.
It is useful so that instances using the session key can be upgraded to use the new secret over even a short period of time without immediately invalidating everything.
There was a problem hiding this comment.
Also, regular secret rotation is good practice even when not found to be compromised.
There was a problem hiding this comment.
I think only the first secret (the one used to set new session cookies) should be subject to the length restriction, since you might still want to restore sessions cookies that were set with crappy secrets so you can re-issue them with new stronger key...
stigtsp
force-pushed
the
secure-by-default-sessions
branch
4 times, most recently
from
0cbcf0d to
53b7107
Compare
* Add `urandom_bytes` and `urandom_urlsafe` to `Mojo::Util` for
generating secure random bits from either Crypt::Random or
/dev/urandom
* Don't use the hard coded moniker as the default secret
* Generate and store a strong secret if not exists in
`$ENV{MOJO_HOME}/mojo.secrets`, overridable with
`$ENV{MOJO_SECRETS_FILE}` when app->secrets is called
* Only load secrets from `mojo.secrets` that are over 22 chars
* Use `urandom_urlsafe` when generating CSRF tokens
* Use `urandom_urlsafe` when in `mojo generate app`
* Add `mojo generate secret`
* Tests:
- Add misc tests for generating and loading mojo.secrets in
`t/mojolicious/secret/` and for `mojo generate secret`.
- Add a default secret in `t/mojolicious/mojo.secrets` so other
session checks work
* Install Crypt::URandom in GH Windows workflow so urandom_bytes works
on that platform
stigtsp
force-pushed
the
secure-by-default-sessions
branch
from
53b7107 to
fb4ae3f
Compare
|
Heads up, early November is SUSE hack week, and i might work on a port of the mojo.js encrypted session implementation to Mojolicious. That's probably the best solution for security. |
|
[like] Lars Madsen reacted to your message:
…________________________________
From: Sebastian Riedel ***@***.***>
Sent: Thursday, October 10, 2024 5:02:56 PM
To: mojolicious/mojo ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [mojolicious/mojo] Secure by default sessions (PR #2200)
Heads up, early November is SUSE hack week, and i might work on a port of the mojo.js encrypted session implementation to Mojolicious. That's probably the best solution for security.
—
Reply to this email directly, view it on GitHub<#2200 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKKC6COO4SPGOUGYJVIUTWTZ22XMBAVCNFSM6AAAAABO7QOICOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBVGYZDOMJZGY>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
|
Assuming that we're talking about https://github.com/mojolicious/mojo.js/blob/436b3263690efe65b0a8b4ecbbb09e4a3c99af5e/src/session.ts#L77 , please consider if it's not vulnerable to IV nonce reuse (https://frereit.de/aes_gcm/) and is using scrypt key derivation algorithm correctly (https://nodejs.org/api/crypto.html#cryptoscryptpassword-salt-keylen-options-callback , see notes about choosing proper salt). If considering change of session cookies format, what about just using Crypt::JWT library instead? |
Good idea. Worth investigating. |
|
I'm not sure if JWT gives us much more than an extra header section with:
|
Even if the session cookie format is changed, a secure method for generating secrets is needed, as provided by this PR. Having strong keys so that the cryptographic methods can uphold their security guarantees may be more important than deciding whether to offer only Integrity (via the current use of a HMAC), or Integrity+Confidentiality (by switching to an authenticated encryption method like AES-GCM). If porting the JavaScript code, it could be made significantly more efficient: static async encrypt(secret: string, value: string): Promise<string> {
const key = await scrypt(secret, 'salt', 32);
const iv = await randomBytes(12);
const cipher = crypto.createCipheriv('aes-256-gcm', key as crypto.CipherKey, iv);
Mojo.js doesn't have benchmarks involving session cookies, but if it had some then caching (or removing) the scrypt() call might save on the order of 100ms per request.
+1 on sticking close to standard formats like JWT, as this neatly sidesteps many pitfalls from doing low-level crypto manually. (Aside from the weakness that a JWT decoder might fail to limit which algorithms are accepted.) That particular library has a hard dependency on But pulling in a large and security-critical XS dependency just for that might not be worth it? The current (and much simpler) approach to session cookies can already be secure (if strong keys are used). |
Is it really? Why can't we outsource that problem to plugins on CPAN? Any core mojo APIs missing? |
@kraih Yes. Strong keys are required for HMAC signed cookies. I have no strong opinion on what CPAN modules or syscalls are used for randomness, as long as bits are from a csprng. For example, NIST requires HMAC keys to be a minimum of 128 bits from a cryptographic random number generator: https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-224.ipd.pdf Problems:
This PR aims to start fixing these problems, so let me know if there is anything more I can do to help. |
Perhaps there was a miscommunication then. We make no claim to be secure by default, that's why we ask users to change their secrets and suggest to rotate regularly. The secrets we generate by default are strictly for development purposes, just like the TLS certificate we ship. It is the users responsibility to change both for production. The default secrets being better with an optional CryptX dependency because of an alternative session implementation would merely be a positive side effect, not an end goal. |
|
Out of curiosity, I checked how some top starred projects on Github with direct Mojolicious dependency are dealing with this problem:
|
|
This seems like a really neat solution and appears very Mojo. The PR looks really well made and followed all the contributing guidelines. I'd love to see this merged. |
|
I'm more and more leaning against this proposal. Especially the secret files don't feel very Mojolicious. We'll see after encrypted sessions have been implemented. |
|
Personally I am 👎 on file based solutions like the one proposed. Any writing to disk also introduce a potential additional attack-vector and on top of that deviates from the move we've been seeing in recent years towards more and more env-based approach to configuration of services, especially for containerized workloads. When doing application hardening and insulation we actively try to limit the applications access to and use of the file system, except where strictly necessary. Those were my $.02 |
|
@christopherraa If the secret isn't persisted, all cookies are invalidated every time the application restarts. The only other choice is making the secret mandatory and aborting startup if it's not set... which gets awful CrashLoopBackoff'y |
|
Not to mention that there are numerous applications out there which have never touched the session cookie or at least have not used it for anything sensitive. I personally think it is very important that it be secure by default, but I also have reservations about writing to disk for permissions/deployment reasons; but it would go along with the default behavior of hypnotoad to write a pidfile to the project directory if not configured otherwise. (The default behavior of the application log to log to the project directory has been removed, as it simply logs to STDERR by default now) |
|
I have to agree with @christopherraa that ephemeral parts of the config, such as secrets, need to be easily updatable in container environments. |
|
What about using a machine-generated unique ID, and then fall back on the moniker if the file can be read? /var/lib/dbus/machine-id |
|
#2212 is what i had in mind for encrypted sessions. |
|
|
||
| return Crypt::URandom::urandom($num) if CRYPT_URANDOM; | ||
|
|
||
| croak 'Cannot find /dev/urandom, install Crypt::URandom from CPAN' unless -e '/dev/urandom'; |
There was a problem hiding this comment.
Is it worth having a fallback to read /dev/urandom vs simply requiring the user to have this module installed if they are using this feature?
|
This pull request is now in conflicts. Could you fix it @stigtsp? 🙏 |
Summary
To make session secrets secure by default, generate and persist a 256 bit session secret:
Add
urandom_bytesandurandom_urlsafetoMojo::Utilfor generating secure random bits from eitherCrypt::Randomor/dev/urandomDon't use the hard coded moniker as the default secret
Generate and store a strong secret if not exists in
$ENV{MOJO_HOME}/mojo.secrets, overridable with$ENV{MOJO_SECRETS_FILE}when app->secrets is calledOnly load secrets from
mojo.secretsthat are over 22 charsUse
urandom_urlsafewhen generating CSRF tokensUse
urandom_urlsafeinmojo generate appAdd
mojo generate secretTests:
t/mojolicious/secret/and formojo generate secret.t/mojolicious/mojo.secretsso other session checks workInstall
Crypt::URandomin GH Windows workflow sourandom_bytesworks on that platformAlso consider
app->secrets()Motivation
Making HMAC protected sessions secure by default after discussing with @jberger at PTS24.
It has been demonstrated by Baking Mojolicious cookies and the recent discussion in #1791 that this is not the case.