-
Notifications
You must be signed in to change notification settings - Fork 583
Refuse to set signed or encrypted cookies with an insecure default secret #2252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,10 @@ sub encrypted_cookie { | |
my $app = $self->app; | ||
my $secret = $app->secrets->[0]; | ||
my $moniker = $app->moniker; | ||
|
||
Carp::croak 'Your secret passphrase must be changed to set encrypted cookies (see FAQ for more)' | ||
if !$ENV{MOJO_ALLOW_INSECURE_SECRET} and (!length $secret or $secret eq $moniker); | ||
|
||
return $self->cookie($name, Mojo::Util::encrypt_cookie($value, $secret, $moniker), $options); | ||
} | ||
|
||
|
@@ -242,12 +246,24 @@ sub session { | |
my $stash = $self->stash; | ||
$self->app->sessions->load($self) unless exists $stash->{'mojo.active_session'}; | ||
|
||
# Hash | ||
my $session = $stash->{'mojo.session'} //= {}; | ||
|
||
# Require changed secret if session data will be set | ||
my $set_operation = !!(@_ > 1 || ref $_[0]); | ||
if (!$ENV{MOJO_ALLOW_INSECURE_SECRET} and (keys %$session or $set_operation)) { | ||
my $app = $self->app; | ||
my $secret = $app->secrets->[0]; | ||
my $moniker = $app->moniker; | ||
|
||
Carp::croak 'Your secret passphrase must be changed to set session data (see FAQ for more)' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these secrets are "passphrase"s (and the FAQ doesn't call them that either) |
||
if !length $secret or $secret eq $moniker; | ||
} | ||
|
||
# Hash | ||
return $session unless @_; | ||
|
||
# Get | ||
return $session->{$_[0]} unless @_ > 1 || ref $_[0]; | ||
return $session->{$_[0]} unless $set_operation; | ||
|
||
# Set | ||
my $values = ref $_[0] ? $_[0] : {@_}; | ||
|
@@ -262,8 +278,15 @@ sub signed_cookie { | |
# Request cookie | ||
return $self->every_signed_cookie($name)->[-1] unless defined $value; | ||
|
||
my $app = $self->app; | ||
my $secret = $app->secrets->[0]; | ||
my $moniker = $app->moniker; | ||
|
||
Carp::croak 'Your secret passphrase must be changed to set signed cookies (see FAQ for more)' | ||
if !$ENV{MOJO_ALLOW_INSECURE_SECRET} and (!length $secret or $secret eq $moniker); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to move this check into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, sorry, github threw away my comment, and i crankly re-typed it wrong. I mean should it be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That attribute may be accessed by applications that don't actually do anything with it, such as Mojolicious::Plugin::Mount. But perhaps we can work around that more easily than the problems with this approach. |
||
|
||
# Response cookie | ||
my $sum = Digest::SHA::hmac_sha256_hex("$name=$value", $self->app->secrets->[0]); | ||
my $sum = Digest::SHA::hmac_sha256_hex("$name=$value", $secret); | ||
return $self->cookie($name, "$value--$sum", $options); | ||
} | ||
|
||
|
@@ -443,6 +466,9 @@ the same name, and you want to access more than just the last one, you can use L | |
are encrypted with ChaCha20-Poly1305, to prevent tampering, and the ones failing decryption will be automatically | ||
discarded. Note that this method is B<EXPERIMENTAL> and might change without warning! | ||
|
||
The L<application secrets|Mojolicious/secrets> B<MUST> be changed from the default to a secure value to set | ||
encrypted cookies. This requirement can be bypassed by setting the C<MOJO_ALLOW_INSECURE_SECRET> environment variable. | ||
|
||
=head2 every_cookie | ||
|
||
my $values = $c->every_cookie('foo'); | ||
|
@@ -745,6 +771,10 @@ Persistent data storage for the next few requests, all session data gets seriali | |
Base64 encoded in HMAC-SHA256 signed cookies, to prevent tampering. Note that cookies usually have a C<4096> byte | ||
(4KiB) limit, depending on browser. | ||
|
||
The L<application secrets|Mojolicious/secrets> B<MUST> be changed from the default to a secure value to set | ||
session data in a signed or encrypted cookie. This requirement can be bypassed by setting the | ||
C<MOJO_ALLOW_INSECURE_SECRET> environment variable. | ||
|
||
# Manipulate session | ||
$c->session->{foo} = 'bar'; | ||
my $foo = $c->session->{foo}; | ||
|
@@ -770,6 +800,9 @@ same name, and you want to access more than just the last one, you can use L</"e | |
cryptographically signed with HMAC-SHA256, to prevent tampering, and the ones failing signature verification will be | ||
automatically discarded. | ||
|
||
The L<application secrets|Mojolicious/secrets> B<MUST> be changed from the default to a secure value to set | ||
signed cookies. This requirement can be bypassed by setting the C<MOJO_ALLOW_INSECURE_SECRET> environment variable. | ||
|
||
=head2 stash | ||
|
||
my $hash = $c->stash; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -91,4 +91,21 @@ subtest 'Rotating secrets' => sub { | |||||
}; | ||||||
}; | ||||||
|
||||||
subtest 'Insecure secret' => sub { | ||||||
subtest 'Insecure secret (signed cookie)' => sub { | ||||||
$t->reset_session; | ||||||
$t->app->secrets([app->moniker]); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] For clarity and consistency with other parts of the code, consider using $t->app->moniker instead of app->moniker when setting insecure secrets in tests.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is actually good! |
||||||
$t->app->sessions->encrypted(0); | ||||||
$t->get_ok('/login')->status_is(500); | ||||||
}; | ||||||
|
||||||
subtest 'Insecure secret (encrypted cookie)' => sub { | ||||||
plan skip_all => 'CryptX required!' unless Mojo::Util->CRYPTX; | ||||||
$t->reset_session; | ||||||
$t->app->secrets([app->moniker]); | ||||||
$t->app->sessions->encrypted(1); | ||||||
$t->get_ok('/login')->status_is(500); | ||||||
}; | ||||||
}; | ||||||
|
||||||
done_testing(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The use of double negation (!!) can reduce readability; consider using an explicit boolean expression to check for a set operation.
Copilot uses AI. Check for mistakes.