Skip to content

Commit

Permalink
Mitigate CSRF predictability/vulnerability
Browse files Browse the repository at this point in the history
  • Loading branch information
@bcosca
bcosca committed May 7, 2018
1 parent 4690a7d commit 3446a34
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 4 deletions.
6 changes: 5 additions & 1 deletion db/jig/session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,11 @@ function __construct(\DB\Jig $db,$file='sessions',$onsuspect=NULL,$key=NULL) {
register_shutdown_function('session_commit');
$fw=\Base::instance();
$headers=$fw->HEADERS;
$this->_csrf=$fw->SEED.'.'.$fw->hash(mt_rand());
$this->_csrf=$fw->SEED.'.'.$fw->hash(
extension_loaded('openssl')?
array_pop(unpack('L',openssl_random_pseudo_bytes(4))):
mt_rand()
);
if ($key)
$fw->$key=$this->_csrf;
$this->_agent=isset($headers['User-Agent'])?$headers['User-Agent']:'';
Expand Down
6 changes: 5 additions & 1 deletion db/mongo/session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,11 @@ function __construct(\DB\Mongo $db,$table='sessions',$onsuspect=NULL,$key=NULL)
register_shutdown_function('session_commit');
$fw=\Base::instance();
$headers=$fw->HEADERS;
$this->_csrf=$fw->SEED.'.'.$fw->hash(mt_rand());
$this->_csrf=$fw->SEED.'.'.$fw->hash(
extension_loaded('openssl')?
array_pop(unpack('L',openssl_random_pseudo_bytes(4))):
mt_rand()
);
if ($key)
$fw->$key=$this->_csrf;
$this->_agent=isset($headers['User-Agent'])?$headers['User-Agent']:'';
Expand Down
6 changes: 5 additions & 1 deletion db/sql/session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,11 @@ function __construct(\DB\SQL $db,$table='sessions',$force=TRUE,$onsuspect=NULL,$
register_shutdown_function('session_commit');
$fw=\Base::instance();
$headers=$fw->HEADERS;
$this->_csrf=$fw->SEED.'.'.$fw->hash(mt_rand());
$this->_csrf=$fw->SEED.'.'.$fw->hash(

This comment has been minimized.

Sign in to view

Copy link
@Rayne

Rayne May 8, 2018

Member

I know that the system variable SEED is only used as prefix for cache entries and temporary files (according to New framework variable: SEED) but I don't like the fact that this secret is shared with the world as CSRF token.

  • A user shouldn't know If a common cache and temp file storage is shared across multiple domains

  • One could try to use the SEED as seed for a random function and the CSRF token would make the pseudo random values predictable

Please correct me if I'm wrong. @bcosca @ikkez

This comment has been minimized.

Sign in to view

Copy link
@KOTRET

KOTRET May 9, 2018

Contributor

@Rayne I'm unsure about the concrete consequences in this case:
in general i agree that the seed should not be dumped to the client (in form of a csrf-token), but related to your points:

A user shouldn't know If a common cache and temp file storage is shared across multiple domains

I dont see any problem here as for different domains the SEED in general is different, if you use same cache, you (manually) define the same seed for accessing the cache - the $fw->SEED generally remains untouched.

One could try to use the SEED as seed for a random function and the CSRF token would make the pseudo random values predictable

but the last part of the csrf still remains unpredictable.

can we get around this by passing the SEED into $fw->hash instead of prefixing the hash with seed or does that shorten the csrf too much?

		$this->_csrf=$fw->hash(
			$fw->SEED.'.'.
			(extension_loaded('openssl')?
				array_pop(unpack('L',openssl_random_pseudo_bytes(4))):
				mt_rand()
			)
		);

This comment has been minimized.

Sign in to view

Copy link
@Rayne

Rayne May 9, 2018

Member

Thank you for the comment. I meant that one could try to use the SEED system variable to produce pseudo random values in custom code, e.g. for a webbrowser game. If the system variable SEED gets "leaked"/known then we should document that developers should create custom seed variables for their RNGs instead of using SEED.

I don't know why multiple websites should share the seed for a RNG, but it would be a good idea to document how SEED is used and how it shouldn't be used. For instance I didn't expect that the user would be able to see the seed.

This comment has been minimized.

Sign in to view

Copy link
@ikkez

ikkez May 9, 2018

Member

I'm for keeping the SEED as private as can be, so the provided fix from @KOTRET would fix this issue here IMO.

This comment has been minimized.

Sign in to view

Copy link
@KOTRET

KOTRET May 9, 2018

Contributor

the "fix" was just a suggestion, but it adds a relatively stable vector as hashing base, which is not that good.

This comment has been minimized.

Sign in to view

Copy link
@Rayne

Rayne Oct 7, 2018

Member

(For the sake of completeness: Fixed with 8d41c1e)
extension_loaded('openssl')?
array_pop(unpack('L',openssl_random_pseudo_bytes(4))):
mt_rand()
);
if ($key)
$fw->$key=$this->_csrf;
$this->_agent=isset($headers['User-Agent'])?$headers['User-Agent']:'';
Expand Down
6 changes: 5 additions & 1 deletion session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,11 @@ function __construct($onsuspect=NULL,$key=NULL,$cache=null) {
register_shutdown_function('session_commit');
$fw=\Base::instance();
$headers=$fw->HEADERS;
$this->_csrf=$fw->SEED.'.'.$fw->hash(mt_rand());
$this->_csrf=$fw->SEED.'.'.$fw->hash(
extension_loaded('openssl')?
array_pop(unpack('L',openssl_random_pseudo_bytes(4))):
mt_rand()
);
if ($key)
$fw->$key=$this->_csrf;
$this->_agent=isset($headers['User-Agent'])?$headers['User-Agent']:'';
Expand Down

0 comments on commit 3446a34

Please sign in to comment.