Bugzilla – Attachment 60326 Details for
Bug 18124
CSRF tokens must be limited to user's session
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18124: [Follow-up] Handle default parameters in a sub
Bug-18124-Follow-up-Handle-default-parameters-in-a.patch (text/plain), 5.91 KB, created by
Marcel de Rooy
on 2017-02-16 11:04:28 UTC
(
hide
)
Description:
Bug 18124: [Follow-up] Handle default parameters in a sub
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2017-02-16 11:04:28 UTC
Size:
5.91 KB
patch
obsolete
>From 50bf7b4c8971d7c591a2001cb7c4aa0ffd88384b Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >Date: Thu, 16 Feb 2017 11:59:12 +0100 >Subject: [PATCH] Bug 18124: [Follow-up] Handle default parameters in a sub >Content-Type: text/plain; charset=utf-8 > >Adds a internal routine to handle default values for the parameters >id and secret. >Also adds a parameter session_id for generate_csrf and check_csrf. This >session parameter is combined with the id parameter when generating or >checking a token. > >Test plan: >Run t/Token.t >--- > Koha/Token.pm | 36 ++++++++++++++++++++++++------------ > t/Token.t | 16 ++++++++-------- > 2 files changed, 32 insertions(+), 20 deletions(-) > >diff --git a/Koha/Token.pm b/Koha/Token.pm >index c560fce..002237e 100644 >--- a/Koha/Token.pm >+++ b/Koha/Token.pm >@@ -74,7 +74,7 @@ sub new { > > my $token = $tokenizer->generate({ length => 20 }); > my $csrf_token = $tokenizer->generate({ >- type => 'CSRF', id => $id, >+ type => 'CSRF', id => $id, secret => $secret, > }); > > Generate several types of tokens. Now includes CSRF. >@@ -85,10 +85,7 @@ sub new { > sub generate { > my ( $self, $params ) = @_; > if( $params->{type} && $params->{type} eq 'CSRF' ) { >- $self->{lasttoken} = _gen_csrf( { >- id => Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ), >- secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ), >- }); >+ $self->{lasttoken} = _gen_csrf( $params ); > } else { > $self->{lasttoken} = _gen_rand( $params ); > } >@@ -97,13 +94,15 @@ sub generate { > > =head2 generate_csrf > >- Shortcut for: generate({ type => 'CSRF', ... }) >+ Like: generate({ type => 'CSRF', ... }) >+ Note: id defaults to userid from context, secret to database password. >+ If session_id is provided, it is combined with id. > > =cut > > sub generate_csrf { > my ( $self, $params ) = @_; >- return unless $params->{id}; >+ $params = _add_default_csrf_params( $params ); > return $self->generate({ %$params, type => 'CSRF' }); > } > >@@ -128,17 +127,30 @@ sub check { > > =head2 check_csrf > >- Shortcut for: check({ type => 'CSRF', ... }) >+ Like: check({ type => 'CSRF', ... }) >+ Note: id defaults to userid from context, secret to database password. >+ If session_id is provided, it is combined with id. > > =cut > > sub check_csrf { > my ( $self, $params ) = @_; >+ $params = _add_default_csrf_params( $params ); > return $self->check({ %$params, type => 'CSRF' }); > } > > # --- Internal routines --- > >+sub _add_default_csrf_params { >+ my ( $params ) = @_; >+ $params->{session_id} //= ''; >+ $params->{id} //= Encode::encode( 'UTF-8', C4::Context->userenv->{id} ); >+ $params->{id} .= $params->{session_id}; >+ my $pw = C4::Context->config('pass'); >+ $params->{secret} //= md5_base64( Encode::encode( 'UTF-8', $pw ) ), >+ return $params; >+} >+ > sub _gen_csrf { > > # Since WWW::CSRF::generate_csrf_token does not use the NonBlocking >@@ -162,12 +174,12 @@ sub _gen_csrf { > > sub _chk_csrf { > my ( $params ) = @_; >- return if !$params->{id} || !$params->{token}; >+ return if !$params->{id} || !$params->{secret} || !$params->{token}; > >- my $id = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ); >- my $secret = md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ); > my $csrf_status = WWW::CSRF::check_csrf_token( >- $id, $secret, $params->{token}, >+ $params->{id}, >+ $params->{secret}, >+ $params->{token}, > { MaxAge => $params->{MaxAge} // ( CSRF_EXPIRY_HOURS * 3600 ) }, > ); > return $csrf_status == WWW::CSRF::CSRF_OK(); >diff --git a/t/Token.t b/t/Token.t >index 3a971a5..b0d8303 100644 >--- a/t/Token.t >+++ b/t/Token.t >@@ -33,7 +33,7 @@ is( length( $tokenizer->generate ), 1, "Generate without parameters" ); > my $token = $tokenizer->generate({ length => 20 }); > is( length($token), 20, "Token $token has 20 chars" ); > >-my $id = $tokenizer->generate({ lyyGength => 8 }); >+my $id = $tokenizer->generate({ length => 8 }); > my $csrftoken = $tokenizer->generate_csrf({ id => $id }); > isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" ); > >@@ -62,14 +62,14 @@ isnt( $result, 1, "CSRF token expired after one second" ); > > subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub { > plan tests => 2; >- $csrftoken = $tokenizer->generate_csrf({ id => $id }); >+ $csrftoken = $tokenizer->generate_csrf({ session_id => $id }); > $result = $tokenizer->check_csrf({ >- id => $id, token => $csrftoken, >+ session_id => $id, token => $csrftoken, > }); > is( $result, 1, "CSRF token verified" ); > C4::Context->set_userenv(0,43,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); > $result = $tokenizer->check_csrf({ >- id => $id, token => $csrftoken, >+ session_id => $id, token => $csrftoken, > }); > is( $result, '', "CSRF token is not verified if another logged in user is using the same id" ); > }; >@@ -77,15 +77,15 @@ subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub { > subtest 'Same logged in user with another session (cookie CGISESSID)' => sub { > plan tests => 2; > C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', '); >- $csrftoken = $tokenizer->generate_csrf({ id => $id }); >+ $csrftoken = $tokenizer->generate_csrf({ session_id => $id }); > $result = $tokenizer->check_csrf({ >- id => $id, token => $csrftoken, >+ session_id => $id, token => $csrftoken, > }); > is( $result, 1, "CSRF token verified" ); > # Get another session id >- $id = $tokenizer->generate({ lyyGength => 8 }); >+ $id = $tokenizer->generate({ length => 8 }); > $result = $tokenizer->check_csrf({ >- id => $id, token => $csrftoken, >+ session_id => $id, token => $csrftoken, > }); > is( $result, '', "CSRF token is not verified if another session is used" ); > }; >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18124
:
60266
|
60326
|
60332
|
60333
|
60334
|
61282
|
61283
|
61284
|
61688