View Issue Details

IDProjectCategoryView StatusLast Update
0017984mantisbtsecuritypublic2015-01-27 04:50
ReporternextgensAssigned Todregad 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version1.2.18 
Target Version1.2.19Fixed in Version1.2.19 
Summary0017984: CVE-2014-9624: CAPTCHA bypass is way easier than it should be
Description

One can get an unlimited amount of "samples" with different perturbations for the same challenge... which makes the whole captcha utterly useless and very easy to bypass.

Solutions involve either caching the captcha or changing the challenge

Steps To Reproduce

1) load the following in a browser tab
https://bugs.freenetproject.org/signup_page.php

2) load this in a new tab
https://bugs.freenetproject.org/make_captcha_img.php

3) refresh the second tab until you're 100% confident your poor OCR got it right

TagsNo tags attached.

Relationships

related to 0017811 closedvboctor CVE-2014-9117: CAPTCHA bypass 
related to 0017993 closeddregad User creation with captcha broken by fix for issue 0017811 

Activities

dregad

dregad

2014-12-29 17:33

developer   ~0042070

Spending time fixing this captcha kind of feels like beating on a dead horse, but anyway here it goes... please review the attached patch, let me know your feedback.

How would you like to be credited for this finding ?

dregad

dregad

2014-12-29 17:33

developer  

0001-Generate-a-unique-CAPTCHA-for-a-given-private-key.patch (3,442 bytes)
From f87632f56c16f80fec532f13e7624c315e562063 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Mon, 29 Dec 2014 23:05:05 +0100
Subject: [PATCH 1/3] Generate a unique CAPTCHA for a given private key
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Prior to this, the code would be the same but the displayed CAPTCHA
would vary slightly with each call to make_captcha_img.php, allowing an
attacker unlimited attempts to train their OCR software to decode the
image.

By seeding the random number generator, we ensure that the generated
CAPTCHA is always the same for a given private key.

The commit also removes several unnecessary calls to srand().

This issue was reported by Florent Daignière (nextgens).

Fixes #17984
---
 make_captcha_img.php | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/make_captcha_img.php b/make_captcha_img.php
index 2bf3734..28391d3 100644
--- a/make_captcha_img.php
+++ b/make_captcha_img.php
@@ -149,6 +149,8 @@
 
 			function make_captcha( $private_key )
 			{
+				srand( session_get( CAPTCHA_KEY ) );
+
 				if($this->debug) echo "\n<br />-Captcha-Debug: Generate private key: ($private_key)";
 
 				// create Image and set the apropriate function depending on GD-Version & websafecolor-value
@@ -182,17 +184,12 @@
 					if($this->debug) echo "\n<br />-Captcha-Debug: Fill background with noise: (".$this->nb_noise.")";
 					for($i=0; $i < $this->nb_noise; $i++)
 					{
-						srand((double)microtime()*1000000);
 						$size	= intval(rand((int)($this->minsize / 2.3), (int)($this->maxsize / 1.7)));
-						srand((double)microtime()*1000000);
 						$angle	= intval(rand(0, 360));
-						srand((double)microtime()*1000000);
 						$x		= intval(rand(0, $this->lx));
-						srand((double)microtime()*1000000);
 						$y		= intval(rand(0, (int)($this->ly - ($size / 5))));
 						$this->random_color(160, 224);
 						$color	= $func2($image, $this->r, $this->g, $this->b);
-						srand((double)microtime()*1000000);
 						$text	= chr(intval(rand(45,250)));
 						if(count ($this->TTF_RANGE)>0){
 							@ImageTTFText($image, $size, $angle, $x, $y, $color, $this->change_TTF(), $text);
@@ -225,11 +222,8 @@
 				for($i=0, $x = intval(rand($this->minsize,$this->maxsize)); $i < $this->chars; $i++)
 				{
 					$text	= utf8_strtoupper(substr($private_key, $i, 1));
-					srand((double)microtime()*1000000);
 					$angle	= intval(rand(($this->maxrotation * -1), $this->maxrotation));
-					srand((double)microtime()*1000000);
 					$size	= intval(rand($this->minsize, $this->maxsize));
-					srand((double)microtime()*1000000);
 					$y		= intval(rand((int)($size * 1.5), (int)($this->ly - ($size / 7))));
 					$this->random_color(0, 127);
 					$color	=  $func2($image, $this->r, $this->g, $this->b);
@@ -270,11 +264,8 @@
 
 			function random_color($min,$max)
 			{
-				srand((double)microtime() * 1000000);
 				$this->r = intval(rand($min,$max));
-				srand((double)microtime() * 1000000);
 				$this->g = intval(rand($min,$max));
-				srand((double)microtime() * 1000000);
 				$this->b = intval(rand($min,$max));
 			}
 
@@ -283,7 +274,6 @@
 				if(count($this->TTF_RANGE) > 0){
 					if(is_array($this->TTF_RANGE))
 					{
-						srand((float)microtime() * 10000000);
 						$key = array_rand($this->TTF_RANGE);
 						$this->TTF_file = $this->TTF_folder.$this->TTF_RANGE[$key];
 					}
-- 
1.9.1

nextgens

nextgens

2014-12-30 05:28

reporter   ~0042081

Hmm, the patch addresses the problem at hand but highlights another vulnerability...

unless I'm mistaken, the MT-based RNG is now seeded once, only with an int (in signup_page.php)... which means there's only "an int worth" (~4billions on 32bit) of possibilities (as opposed to the same keyspace but lots of different permutations before the patch). One can pre-compute the whole lot and instantly lookup the challenge...

The proposed cure looks worst than the disease in this case...

Please credit "Florent Daigniere from Matta Consulting"

dregad

dregad

2014-12-30 17:57

developer   ~0042082

Last edited: 2014-12-30 17:58

View 2 revisions

there's only "an int worth" (~4billions on 32bit) of possibilities

That's ~2m (2^31) different captcha keys actually, but that's not different from before (and already an improvement over Mantis < 1.2.18 where the range was only 0..9999).

One can pre-compute the whole lot and instantly lookup the challenge...

I didn't think about that.

Seeding the RNG sounded like a simple and effective approach, but I guess it was just too easy ;-)

The only alternative I can think of ATM (short of changing the whole captcha code, which is not worth the effort as we're switching to a new one in 1.3 anyway) would be to cache the generated image in the user's session. Thoughts ?

nextgens

nextgens

2014-12-31 05:12

reporter   ~0042085

Caching seems like the right solution to me

dregad

dregad

2015-01-03 17:24

developer  

0002-Cache-generated-captcha-to-ensure-uniqueness.patch (4,391 bytes)
From 8b23a8163273bcf9bb87d402d0691c81cc1bb7d8 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Fri, 2 Jan 2015 01:39:14 +0100
Subject: [PATCH] Cache generated captcha to ensure uniqueness

This is an improvement over the earlier fix which seeded the random
number generator with the captcha's key.

As Florent pointed out, the cure was worse than the disease as it
reduced the effective number of distinct captchas to a mere 2^31, making
it easy for an attacker to precompute the lot to bypass the challenge.

In addition, debug mode now works in context of make_captcha_img.php,
i.e. it doesn't set the image/jpeg content type header, and displays the
generated captcha as an image within an html page.

Fixes #17984
---
 core/constant_inc.php |  1 +
 make_captcha_img.php  | 40 +++++++++++++++++++++++++++++++++++-----
 signup.php            |  3 +++
 signup_page.php       |  1 +
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/core/constant_inc.php b/core/constant_inc.php
index 25715ca..7e04801 100644
--- a/core/constant_inc.php
+++ b/core/constant_inc.php
@@ -542,3 +542,4 @@ define( 'PASSWORD_MAX_SIZE_BEFORE_HASH', 1024 );
 define( 'SECONDS_PER_DAY', 86400 );
 
 define( 'CAPTCHA_KEY', 'captcha_key' );
+define( 'CAPTCHA_IMG', 'captcha_image' );
diff --git a/make_captcha_img.php b/make_captcha_img.php
index 28391d3..cda9b29 100644
--- a/make_captcha_img.php
+++ b/make_captcha_img.php
@@ -147,10 +147,8 @@
 				if($this->debug) echo "\n<br />-Captcha-Debug: Set image dimension to: (".$this->lx." x ".$this->ly.")";
 			}
 
-			function make_captcha( $private_key )
+			function generate_captcha( $private_key )
 			{
-				srand( session_get( CAPTCHA_KEY ) );
-
 				if($this->debug) echo "\n<br />-Captcha-Debug: Generate private key: ($private_key)";
 
 				// create Image and set the apropriate function depending on GD-Version & websafecolor-value
@@ -218,7 +216,7 @@
 				}
 
 				// generate Text
-				if($this->debug) echo "\n<br />-Captcha-Debug: Fill forground with chars and shadows: (".$this->chars.")";
+				if($this->debug) echo "\n<br />-Captcha-Debug: Fill foreground with chars and shadows: (".$this->chars.")";
 				for($i=0, $x = intval(rand($this->minsize,$this->maxsize)); $i < $this->chars; $i++)
 				{
 					$text	= utf8_strtoupper(substr($private_key, $i, 1));
@@ -239,10 +237,42 @@
 					}
 					$x += (int)($size + ($this->minsize / 5));
 				}
-				header('Content-type: image/jpeg');
+
+				# Generate the JPEG
+				ob_start();
 				@ImageJPEG($image, null, $this->jpegquality);
+				$jpg = ob_get_contents();
+				ob_end_clean();
+
 				@ImageDestroy($image);
 				if($this->debug) echo "\n<br />-Captcha-Debug: Destroy Imagestream.";
+
+				return $jpg;
+			}
+
+			function make_captcha( $private_key )
+			{
+				# Retrieve previously image generated from session cache
+				$t_image = session_get( CAPTCHA_IMG, null );
+
+				if( is_null( $t_image ) ) {
+					$t_image = $this->generate_captcha( $private_key );
+					if( $this->debug ) {
+						echo "\n<br />-Captcha-Debug: Caching generated image.";
+					}
+					session_set( CAPTCHA_IMG, $t_image );
+				} elseif( $this->debug ) {
+					echo "\n<br />-Captcha-Debug: Retrieved image from cache.";
+				}
+
+				# Output
+				if( $this->debug ) {
+					echo "\n<br />-Captcha-Debug: Generated image (" . strlen( $t_image ) . " bytes): "
+						. '<img src="data:image/jpeg;base64,' . base64_encode( $t_image ) . '">';
+				} else {
+					header('Content-type: image/jpeg');
+					echo $t_image;
+				}
 			}
 
 			/** @private **/
diff --git a/signup.php b/signup.php
index b63e772..8ee2449 100644
--- a/signup.php
+++ b/signup.php
@@ -63,6 +63,9 @@
 		if ( $t_key != $f_captcha ) {
 			trigger_error( ERROR_SIGNUP_NOT_MATCHING_CAPTCHA, ERROR );
 		}
+
+		# Clear captcha cache
+		session_delete( CAPTCHA_IMG );
 	}
 
 	email_ensure_not_disposable( $f_email );
diff --git a/signup_page.php b/signup_page.php
index 3a8c725..b7a302c 100644
--- a/signup_page.php
+++ b/signup_page.php
@@ -67,6 +67,7 @@
 	$t_allow_passwd = helper_call_custom_function( 'auth_can_change_password', array() );
 	if( ON == config_get( 'signup_use_captcha' ) && get_gd_version() > 0 && ( true == $t_allow_passwd ) ) {
 		session_set( CAPTCHA_KEY, mt_rand() );
+		session_delete( CAPTCHA_IMG );
 
 		# captcha image requires GD library and related option to ON
 ?>
-- 
1.9.1

dregad

dregad

2015-01-03 17:28

developer   ~0042090

Salut Florent,

Attached is a 2nd patch (to be applied on top of the previous one), which implements caching.

As I was testing this code, I noticed a regression bug in the 1.2.18 captcha validation, which was also reported by another user (see 0017993), you might want to check out the fix in the proposed pull request [1] too (feedback welcome).

[1] https://github.com/mantisbt/mantisbt/pull/566

nextgens

nextgens

2015-01-06 06:02

reporter   ~0042106

I would move the session_delete before the trigger_error() call in signup.php ... otherwise there's a race-condition the attacker is sure to win (he can read the response slowly and retry against the same challenge in parallel)

As for the session handling/pull request, I'm not sure I understand why the session is trashed in the first place. If what you're trying to avoid is session fixation attacks, changing the session identifier is the only thing that needs to happen... there's no need to void the content of the session itself (that's what session_regenerate_id() does)

dregad

dregad

2015-01-06 08:49

developer   ~0042112

Thanks for your feedback.

I would move the session_delete before the trigger_error() call in signup.php

Makes sense, will do.

I'm not sure I understand why the session is trashed in the first place.

I'm not sure either, tbh... Never touched this code before.

My understanding is that signing up implies logging out of Mantis, which is necessary because when signing up, someone may currently be logged in, either as anonymous or less likely as themselves (in which case signup_page.php needs to be accessed manually). I think it makes sense to clear the session's contents when logging out.

Does that make sense to you ?

nextgens

nextgens

2015-01-06 09:03

reporter   ~0042113

Destroying the session when logging out, sure...

For the rest not really. Traditionally applications allow concurrent sessions for users... anonymous/guest sessions are handled the same way... and that allows for "upgrading" an unauthenticated session to an authenticated one (sessions are independent so there's no problem there).

I suspect that Mantis sessions aren't handled that way... whether it makes sense to change it for this stable branch I'm not sure ^-^

dregad

dregad

2015-01-16 20:15

developer   ~0042178

CVE request sent http://article.gmane.org/gmane.comp.security.oss.general/15434

dregad

dregad

2015-01-18 18:23

developer   ~0042190

MITRE assigned CVE-2014-9624 to this issue [1].

[1] http://article.gmane.org/gmane.comp.security.oss.general/15452

Related Changesets

MantisBT: master-1.2.x 39a92726

2014-12-29 17:05:05

dregad

Details Diff
Cache the generated CAPTCHA to ensure uniqueness

Prior to this, the code would be the same but the displayed CAPTCHA
would vary slightly with each call to make_captcha_img.php, allowing an
attacker unlimited attempts to train their OCR software to decode the
image.

This issue was reported by Florent Daignière from Matta Consulting.

In addition, CAPTCHA debug mode now works in make_captcha_img.php's
context, i.e. it doesn't set the image/jpeg content type header, and
displays the generated captcha as an image within an html page.

Fixes 0017984
mod - core/constant_inc.php Diff File
mod - make_captcha_img.php Diff File
mod - signup.php Diff File
mod - signup_page.php Diff File

Issue History

Date Modified Username Field Change
2014-12-24 06:18 nextgens New Issue
2014-12-24 08:08 atrol Relationship added related to 0017811
2014-12-24 08:09 atrol Status new => confirmed
2014-12-29 17:33 dregad Assigned To => dregad
2014-12-29 17:33 dregad Status confirmed => assigned
2014-12-29 17:33 dregad Note Added: 0042070
2014-12-29 17:33 dregad File Added: 0001-Generate-a-unique-CAPTCHA-for-a-given-private-key.patch
2014-12-29 17:33 dregad Target Version => 1.2.19
2014-12-30 05:28 nextgens Note Added: 0042081
2014-12-30 17:57 dregad Note Added: 0042082
2014-12-30 17:58 dregad Status assigned => feedback
2014-12-30 17:58 dregad Note Edited: 0042082 View Revisions
2014-12-31 05:12 nextgens Note Added: 0042085
2014-12-31 05:12 nextgens Status feedback => assigned
2015-01-03 17:24 dregad File Added: 0002-Cache-generated-captcha-to-ensure-uniqueness.patch
2015-01-03 17:28 dregad Relationship added related to 0017993
2015-01-03 17:28 dregad Note Added: 0042090
2015-01-06 06:02 nextgens Note Added: 0042106
2015-01-06 08:49 dregad Note Added: 0042112
2015-01-06 09:03 nextgens Note Added: 0042113
2015-01-16 20:05 dregad Changeset attached => MantisBT master-1.2.x 39a92726
2015-01-16 20:05 dregad Status assigned => resolved
2015-01-16 20:05 dregad Resolution open => fixed
2015-01-16 20:05 dregad Fixed in Version => 1.2.19
2015-01-16 20:15 dregad Note Added: 0042178
2015-01-16 20:15 dregad View Status private => public
2015-01-18 18:23 dregad Summary CAPTCHA bypass is way easier than it should be => CVE-2014-9624: CAPTCHA bypass is way easier than it should be
2015-01-18 18:23 dregad Note Added: 0042190
2015-01-25 18:17 dregadmin Status resolved => closed
2015-01-27 04:50 dregad Issue cloned: 0019276