From fb4ae3fec2ce3ba86784b2d5dc0d68554a0ca7b6 Mon Sep 17 00:00:00 2001 From: Stig Palmquist Date: Sun, 29 Sep 2024 12:26:17 +0200 Subject: [PATCH] Generate and persist a 256 bit session secret by default * 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 --- .github/workflows/windows.yml | 2 +- lib/Mojo/Util.pm | 41 +++++++++- lib/Mojolicious.pm | 40 ++++++--- .../Command/Author/generate/app.pm | 4 +- .../Command/Author/generate/secret.pm | 82 +++++++++++++++++++ lib/Mojolicious/Plugin/DefaultHelpers.pm | 4 +- t/mojo/util.t | 16 +++- t/mojolicious/app.t | 6 +- t/mojolicious/commands.t | 35 ++++++++ t/mojolicious/lite_app.t | 8 +- t/mojolicious/mojo.secrets | 1 + t/mojolicious/secret/custom.secrets | 3 + t/mojolicious/secret/lite-create.t | 16 ++++ t/mojolicious/secret/lite-env-file.t | 17 ++++ t/mojolicious/secret/lite-home.t | 13 +++ t/mojolicious/secret/lite-weak.t | 14 ++++ t/mojolicious/secret/weak.secrets | 2 + 17 files changed, 278 insertions(+), 26 deletions(-) create mode 100644 lib/Mojolicious/Command/Author/generate/secret.pm create mode 100644 t/mojolicious/mojo.secrets create mode 100644 t/mojolicious/secret/custom.secrets create mode 100644 t/mojolicious/secret/lite-create.t create mode 100644 t/mojolicious/secret/lite-env-file.t create mode 100644 t/mojolicious/secret/lite-home.t create mode 100644 t/mojolicious/secret/lite-weak.t create mode 100644 t/mojolicious/secret/weak.secrets diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index abd0e283df..85e4fa8bfb 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -24,6 +24,6 @@ jobs: - name: Install Dependencies run: | cpanm --installdeps . - cpanm -n TAP::Formatter::GitHubActions + cpanm -n TAP::Formatter::GitHubActions Crypt::URandom - name: Run Tests run: prove --merge --formatter TAP::Formatter::GitHubActions -l t t/mojo t/mojolicious diff --git a/lib/Mojo/Util.pm b/lib/Mojo/Util.pm index 069cf9cf9c..fc97981e3b 100644 --- a/lib/Mojo/Util.pm +++ b/lib/Mojo/Util.pm @@ -13,7 +13,7 @@ use IO::Compress::Gzip; use IO::Poll qw(POLLIN POLLPRI); use IO::Uncompress::Gunzip; use List::Util qw(min); -use MIME::Base64 qw(decode_base64 encode_base64); +use MIME::Base64 qw(decode_base64 encode_base64 encode_base64url); use Mojo::BaseUtil qw(class_to_path monkey_patch); use Pod::Usage qw(pod2usage); use Socket qw(inet_pton AF_INET6 AF_INET); @@ -24,6 +24,8 @@ use Unicode::Normalize (); # Check for monotonic clock support use constant MONOTONIC => !!eval { Time::HiRes::clock_gettime(Time::HiRes::CLOCK_MONOTONIC()) }; +use constant CRYPT_URANDOM => !!eval { require Crypt::URandom }; + # Punycode bootstring parameters use constant { PC_BASE => 36, @@ -72,7 +74,7 @@ our @EXPORT_OK = ( qw(extract_usage getopt gunzip gzip header_params hmac_sha1_sum html_attr_unescape html_unescape humanize_bytes), qw(md5_bytes md5_sum monkey_patch network_contains punycode_decode punycode_encode quote scope_guard secure_compare), qw(sha1_bytes sha1_sum slugify split_cookie_header split_header steady_time tablify term_escape trim unindent), - qw(unquote url_escape url_unescape xml_escape xor_encode) + qw(unquote urandom_bytes urandom_urlsafe url_escape url_unescape xml_escape xor_encode) ); # Aliases @@ -379,6 +381,25 @@ sub unquote { return $str; } +sub urandom_bytes { + my $num = shift || 32; + + return Crypt::URandom::urandom($num) if CRYPT_URANDOM; + + croak 'Cannot find /dev/urandom, install Crypt::URandom from CPAN' unless -e '/dev/urandom'; + + open(my $urandom, '<', '/dev/urandom') or croak "Cannot open /dev/urandom: $!"; + sysread($urandom, my $bytes, $num) == $num or croak "sysread() from /dev/urandom didn't return $num bytes"; + close($urandom); + + return $bytes; +} + +sub urandom_urlsafe { + my $num = shift; + return encode_base64url(urandom_bytes($num)); +} + sub url_escape { my ($str, $pattern) = @_; @@ -958,6 +979,22 @@ Unindent multi-line string. Unquote string. +=head2 urandom_bytes + + my $bytes = urandom_bytes; + my $bytes = urandom_bytes 32; + +Returns strong random bytes. Uses L if it is installed, with fallback to /dev/urandom. The default +number of random bytes returned is 32. + +=head2 urandom_urlsafe + + my $token = urandom_urlsafe; + my $token = urandom_urlsafe 32; + +Generates a base64url encoded string of random bytes suitable for session tokens and similar. The default number of random +bytes encoded is 32. + =head2 url_escape my $escaped = url_escape $str; diff --git a/lib/Mojolicious.pm b/lib/Mojolicious.pm index 2eb879c74b..3006665360 100644 --- a/lib/Mojolicious.pm +++ b/lib/Mojolicious.pm @@ -9,7 +9,8 @@ use Mojo::Home; use Mojo::Loader; use Mojo::Log; use Mojo::Server; -use Mojo::Util; +use Mojo::Util qw(urandom_urlsafe); +use Mojo::File qw(path); use Mojo::UserAgent; use Mojolicious::Commands; use Mojolicious::Controller; @@ -41,14 +42,31 @@ has plugins => sub { Mojolicious::Plugins->new }; has preload_namespaces => sub { [] }; has renderer => sub { Mojolicious::Renderer->new }; has routes => sub { Mojolicious::Routes->new }; +has secrets_file => sub { $ENV{MOJO_SECRETS_FILE} || shift->home->rel_file('mojo.secrets') }; has secrets => sub { my $self = shift; + my $file = $self->secrets_file; - # Warn developers about insecure default - $self->log->trace('Your secret passphrase needs to be changed (see FAQ for more)'); + if (-f $file) { - # Default to moniker - return [$self->moniker]; + # 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; + + die qq{"Your secrets_file "$file" does not contain any acceptable secret (of 22 chars or more)} unless @secrets; + + return [@secrets]; + } + + # If no secrets file exists, generate one and attempt to write it back to + # secrets_file, taking care that the file is only readable by the current + # user. + my $secret = urandom_urlsafe; + path($file)->touch->chmod(0600)->spew($secret); + + $self->log->trace(qq{Your secret passphrase has been set to strong random value and stored in "$file"}); + + return [$secret]; }; has sessions => sub { Mojolicious::Sessions->new }; has static => sub { Mojolicious::Static->new }; @@ -496,11 +514,13 @@ endpoints for your application. my $secrets = $app->secrets; $app = $app->secrets([$bytes]); -Secret passphrases used for signed cookies and the like, defaults to the L of this application, which is -not very secure, so you should change it!!! As long as you are using the insecure default there will be debug messages -in the log file reminding you to change your passphrase. Only the first passphrase is used to create new signatures, -but all of them for verification. So you can increase security without invalidating all your existing signed cookies by -rotating passphrases, just add new ones to the front and remove old ones from the back. +Secret passphrases used for signed cookies and the like, defaults to 256 bits of data from your systems secure +random number generator and is stored in the file mojo.secrets in your MOJO_HOME directory. You can override +the location of this file by setting MOJO_SECRETS_FILE in your environment. + +Only the first passphrase is used to create new signatures, but all of them for verification. So you can +increase security without invalidating all your existing signed cookies by rotating passphrases, just add new +ones to the front and remove old ones from the back. # Rotate passphrases $app->secrets(['new_passw0rd', 'old_passw0rd', 'very_old_passw0rd']); diff --git a/lib/Mojolicious/Command/Author/generate/app.pm b/lib/Mojolicious/Command/Author/generate/app.pm index 7e42ccfb03..166c979ce8 100644 --- a/lib/Mojolicious/Command/Author/generate/app.pm +++ b/lib/Mojolicious/Command/Author/generate/app.pm @@ -196,7 +196,7 @@ done_testing();

@@ config -% use Mojo::Util qw(sha1_sum steady_time); +% use Mojo::Util qw(urandom_urlsafe); --- secrets: - - <%= sha1_sum $$ . steady_time . rand %> + - <%= urandom_urlsafe %> diff --git a/lib/Mojolicious/Command/Author/generate/secret.pm b/lib/Mojolicious/Command/Author/generate/secret.pm new file mode 100644 index 0000000000..ecfb8f23a3 --- /dev/null +++ b/lib/Mojolicious/Command/Author/generate/secret.pm @@ -0,0 +1,82 @@ +package Mojolicious::Command::Author::generate::secret; +use Mojo::Base 'Mojolicious::Command'; +use Mojo::File qw(path); +use Mojo::Util qw(urandom_urlsafe); + +has description => 'Generate secret'; +has usage => sub { shift->extract_usage }; + +sub run { + my ($self, $secret_file) = (shift, shift); + + $secret_file //= $self->app->secrets_file; + + my $token = urandom_urlsafe(); + + print "Writing secret to $secret_file\n"; + + path($secret_file)->touch->chmod(0600)->spew($token); +} + +1; + +=encoding utf8 + +=head1 NAME + +Mojolicious::Command::Author::generate::secret - Secret generator command + +=head1 SYNOPSIS + + Usage: APPLICATION generate secret [PATH] + + mojo generate secret + mojo generate secret /path/to/secret + + Options: + -h, --help Show this summary of available options + +=head1 DESCRIPTION + +L generates a secret token for protecting session cookies + +This is a core command, that means it is always enabled and its code a good example for learning to build new commands, +you're welcome to fork it. + +See L for a list of commands that are available by default. + +=head1 ATTRIBUTES + +L inherits all attributes from L and implements +the following new ones. + +=head2 description + + my $description = $app->description; + $app = $app->description('Foo'); + +Short description of this command, used for the command list. + +=head2 usage + + my $usage = $app->usage; + $app = $app->usage('Foo'); + +Usage information for this command, used for the help screen. + +=head1 METHODS + +L inherits all methods from L and implements +the following new ones. + +=head2 run + + $app->run(@ARGV); + +Run this command. + +=head1 SEE ALSO + +L, L, L. + +=cut diff --git a/lib/Mojolicious/Plugin/DefaultHelpers.pm b/lib/Mojolicious/Plugin/DefaultHelpers.pm index 163a8cef12..e68cd3a6e7 100644 --- a/lib/Mojolicious/Plugin/DefaultHelpers.pm +++ b/lib/Mojolicious/Plugin/DefaultHelpers.pm @@ -8,7 +8,7 @@ use Mojo::Collection; use Mojo::Exception; use Mojo::IOLoop; use Mojo::Promise; -use Mojo::Util qw(dumper hmac_sha1_sum steady_time); +use Mojo::Util qw(dumper urandom_urlsafe); use Time::HiRes qw(gettimeofday tv_interval); use Scalar::Util qw(blessed weaken); @@ -95,7 +95,7 @@ sub _convert_to_exception { return (blessed $e && $e->isa('Mojo::Exception')) ? $e : Mojo::Exception->new($e); } -sub _csrf_token { $_[0]->session->{csrf_token} ||= hmac_sha1_sum($$ . steady_time . rand, $_[0]->app->secrets->[0]) } +sub _csrf_token { $_[0]->session->{csrf_token} ||= urandom_urlsafe; } sub _current_route { return '' unless my $route = shift->match->endpoint; diff --git a/t/mojo/util.t b/t/mojo/util.t index e7905c97af..5094ac88c0 100644 --- a/t/mojo/util.t +++ b/t/mojo/util.t @@ -12,7 +12,7 @@ use Mojo::Util qw(b64_decode b64_encode camelize class_to_file class_to_path dec qw(extract_usage getopt gunzip gzip header_params hmac_sha1_sum html_unescape html_attr_unescape humanize_bytes), qw(md5_bytes md5_sum monkey_patch network_contains punycode_decode punycode_encode quote scope_guard secure_compare), qw(sha1_bytes sha1_sum slugify split_cookie_header split_header steady_time tablify term_escape trim unindent), - qw(unquote url_escape url_unescape xml_escape xor_encode); + qw(unquote urandom_bytes urandom_urlsafe url_escape url_unescape xml_escape xor_encode); subtest 'camelize' => sub { is camelize('foo_bar_baz'), 'FooBarBaz', 'right camelized result'; @@ -661,4 +661,18 @@ subtest 'Hide DATA usage from error messages' => sub { unlike $@, qr/DATA/, 'DATA has been hidden'; }; +subtest 'urandom' => sub { + isnt urandom_bytes, urandom_bytes, "two urandom_bytes invocations are not the same"; + is length(urandom_bytes), 32, "urandom_bytes returns 32 bytes by default"; + is length(urandom_bytes(16)), 16, "urandom_bytes(16) returns 16 bytes"; + + isnt urandom_urlsafe, urandom_urlsafe, "two urandom_urlsafe invocations are not the same"; + like urandom_urlsafe, qr/^[-A-Za-z0-9_]{43}$/, "urandom_urlsafe returns 43 chars of urlsafe encoded base64"; + like urandom_urlsafe(128), qr/^[-A-Za-z0-9_]{171}$/, + "urandom_urlsafe(128) returns 171 chars of urlsafe encoded base64"; + like urandom_urlsafe(2048), qr/^[-A-Za-z0-9_]{2731}$/, + "urandom_urlsafe(2048) returns 2731 chars of urlsafe encoded base64"; +}; + + done_testing(); diff --git a/t/mojolicious/app.t b/t/mojolicious/app.t index 506981c4a9..e508c4cd82 100644 --- a/t/mojolicious/app.t +++ b/t/mojolicious/app.t @@ -90,9 +90,9 @@ is_deeply $t->app->commands->namespaces, ['Mojolicious::Command::Author', 'Mojolicious::Command', 'MojoliciousTest::Command'], 'right namespaces'; is $t->app, $t->app->commands->app, 'applications are equal'; is $t->app->static->file('hello.txt')->slurp, "Hello Mojo from a development static file!\n", 'right content'; -is $t->app->static->file('does_not_exist.html'), undef, 'no file'; -is $t->app->moniker, 'mojolicious_test', 'right moniker'; -is $t->app->secrets->[0], $t->app->moniker, 'secret defaults to moniker'; +is $t->app->static->file('does_not_exist.html'), undef, 'no file'; +is $t->app->moniker, 'mojolicious_test', 'right moniker'; +is $t->app->secrets->[0], 'NeverGonnaGiveYouUpNeverGonnaLetYouDown', 'secret defaults to content of mojo.secrets'; is $t->app->renderer->template_handler({template => 'foo/bar/index', format => 'html'}), 'epl', 'right handler'; is $t->app->build_controller->req->url, '', 'no URL'; is $t->app->build_controller->render_to_string('does_not_exist'), undef, 'no result'; diff --git a/t/mojolicious/commands.t b/t/mojolicious/commands.t index 6e5b80ee00..c62c5a6da0 100644 --- a/t/mojolicious/commands.t +++ b/t/mojolicious/commands.t @@ -155,6 +155,13 @@ $buffer = ''; } like $buffer, qr/Usage: APPLICATION generate lite-app \[OPTIONS\] \[NAME\]/, 'right output'; $buffer = ''; +{ + open my $handle, '>', \$buffer; + local *STDOUT = $handle; + $commands->run('generate', 'secret', '--help'); +} +like $buffer, qr/Usage: APPLICATION generate secret \[PATH\]/, 'right output'; +$buffer = ''; { open my $handle, '>', \$buffer; local *STDOUT = $handle; @@ -474,6 +481,34 @@ $buffer = ''; like $buffer, qr/Unknown option: unknown/, 'right output'; chdir $cwd; +# generate lite_app +require Mojolicious::Command::Author::generate::secret; +$app = Mojolicious::Command::Author::generate::secret->new; +ok $app->description, 'has a description'; +like $app->usage, qr/secret/, 'has usage information'; +$dir = tempdir CLEANUP => 1; +chdir $dir; +{ + my $check = sub { + my $f = path($dir)->child(shift); + ok -f $f, "$f exists"; + like $f->slurp, qr/^[-A-Za-z0-9_]{43}$/, "$f contains a urandom_urlsafe generated secret"; + }; + + local $ENV{MOJO_HOME} = $dir; + $app->run; + $check->("mojo.secrets"); + + local $ENV{MOJO_SECRETS_FILE} = path($dir)->child("from-env-var.secrets"); + $app = Mojolicious::Command::Author::generate::secret->new; + $app->run; + $check->("from-env-var.secrets"); + + $app->run("from-args.secrets"); + $check->("from-args.secrets"); +} +chdir $cwd; + # inflate require Mojolicious::Command::Author::inflate; my $inflate = Mojolicious::Command::Author::inflate->new; diff --git a/t/mojolicious/lite_app.t b/t/mojolicious/lite_app.t index 15159b5a62..8e22df96d8 100644 --- a/t/mojolicious/lite_app.t +++ b/t/mojolicious/lite_app.t @@ -22,10 +22,8 @@ app->defaults(default => 23); # Secret app->log->level('trace')->unsubscribe('message'); -my $logs = app->log->capture('trace'); -is app->secrets->[0], app->moniker, 'secret defaults to moniker'; -like $logs, qr/Your secret passphrase needs to be changed/, 'right message'; -undef $logs; + +is app->secrets->[0], 'NeverGonnaGiveYouUpNeverGonnaLetYouDown', 'secret defaults to content of mojo.secrets'; # Test helpers helper test_helper => sub { shift->param(@_) }; @@ -751,7 +749,7 @@ $t->get_ok('/to_string')->status_is(200)->content_is('beforeafter'); $t->get_ok('/source')->status_is(200)->content_type_is('application/octet-stream')->content_like(qr!get_ok\('/source!); # File does not exist -$logs = app->log->capture('trace'); +my $logs = app->log->capture('trace'); $t->get_ok('/source?fail=1')->status_is(500)->content_like(qr/Static file "does_not_exist.txt" not found/); like $logs, qr/Static file "does_not_exist.txt" not found/, 'right message'; undef $logs; diff --git a/t/mojolicious/mojo.secrets b/t/mojolicious/mojo.secrets new file mode 100644 index 0000000000..2fcb949352 --- /dev/null +++ b/t/mojolicious/mojo.secrets @@ -0,0 +1 @@ +NeverGonnaGiveYouUpNeverGonnaLetYouDown diff --git a/t/mojolicious/secret/custom.secrets b/t/mojolicious/secret/custom.secrets new file mode 100644 index 0000000000..938500f796 --- /dev/null +++ b/t/mojolicious/secret/custom.secrets @@ -0,0 +1,3 @@ +NeverGonnaMakeYouCryNeverGonnaSayGoodbye +skip-me +NeverGonnaTellALieAndHurtYou diff --git a/t/mojolicious/secret/lite-create.t b/t/mojolicious/secret/lite-create.t new file mode 100644 index 0000000000..13fbbb3b7a --- /dev/null +++ b/t/mojolicious/secret/lite-create.t @@ -0,0 +1,16 @@ +use Mojo::Base -strict; + +use Mojo::File qw(tempdir path); +use Test::Mojo; +use Test::More; +use Mojolicious::Lite; + + +my $tmpdir = tempdir; +my $file = $tmpdir->child("mojo.secrets"); +$ENV{MOJO_SECRETS_FILE} = $file; + +like app->secrets->[0], qr/^[-A-Za-z0-9_]{43}$/, 'secret was generated, and matches expected urandom_urlsafe format'; +is app->secrets->[0], $file->slurp, 'secret stored at $ENV{MOJO_SECRETS_FILE} is the same as app->secrets->[0]'; + +done_testing(); diff --git a/t/mojolicious/secret/lite-env-file.t b/t/mojolicious/secret/lite-env-file.t new file mode 100644 index 0000000000..b5e3f38351 --- /dev/null +++ b/t/mojolicious/secret/lite-env-file.t @@ -0,0 +1,17 @@ +use Mojo::Base -strict; + +BEGIN { + $ENV{MOJO_SECRETS_FILE} = "t/mojolicious/secret/custom.secrets"; +} + +use Test::Mojo; +use Test::More; +use Mojolicious::Lite; + +is_deeply( + app->secrets, + ['NeverGonnaMakeYouCryNeverGonnaSayGoodbye', 'NeverGonnaTellALieAndHurtYou'], + 'only valid secrets are loaded from $ENV{MOJO_SECRETS_FILE}' +); + +done_testing(); diff --git a/t/mojolicious/secret/lite-home.t b/t/mojolicious/secret/lite-home.t new file mode 100644 index 0000000000..a6924967ed --- /dev/null +++ b/t/mojolicious/secret/lite-home.t @@ -0,0 +1,13 @@ +use Mojo::Base -strict; + +BEGIN { + $ENV{MOJO_HOME} = "t/mojolicious/"; +} + +use Test::Mojo; +use Test::More; +use Mojolicious::Lite; + +is app->secrets->[0], 'NeverGonnaGiveYouUpNeverGonnaLetYouDown', 'secret is loaded from home'; + +done_testing(); diff --git a/t/mojolicious/secret/lite-weak.t b/t/mojolicious/secret/lite-weak.t new file mode 100644 index 0000000000..a562199402 --- /dev/null +++ b/t/mojolicious/secret/lite-weak.t @@ -0,0 +1,14 @@ +use Mojo::Base -strict; + +BEGIN { + $ENV{MOJO_SECRETS_FILE} = "t/mojolicious/secret/weak.secrets"; +} + +use Test::Mojo; +use Test::More; +use Mojolicious::Lite; + +eval { app->secrets; }; +like $@, qr/does not contain any acceptable secret/; + +done_testing(); diff --git a/t/mojolicious/secret/weak.secrets b/t/mojolicious/secret/weak.secrets new file mode 100644 index 0000000000..b3607ec5a2 --- /dev/null +++ b/t/mojolicious/secret/weak.secrets @@ -0,0 +1,2 @@ +hunter2 +password