Skip to content
Permalink
Browse files Browse the repository at this point in the history
PrimeField: prevent infinite loop with composite primefields
  • Loading branch information
terrafrost committed Mar 2, 2023
1 parent 3b6030d commit 6298d1c
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
6 changes: 3 additions & 3 deletions phpseclib/Math/PrimeField.php
Expand Up @@ -44,9 +44,9 @@ class PrimeField extends FiniteField
*/
public function __construct(BigInteger $modulo)
{
//if (!$modulo->isPrime()) {
// throw new \UnexpectedValueException('PrimeField requires a prime number be passed to the constructor');
//}
if (!$modulo->isPrime()) {
throw new \UnexpectedValueException('PrimeField requires a prime number be passed to the constructor');
}

$this->instanceID = self::$instanceCounter++;
Integer::setModulo($this->instanceID, $modulo);
Expand Down
10 changes: 5 additions & 5 deletions phpseclib/Math/PrimeField/Integer.php
Expand Up @@ -263,13 +263,13 @@ public function squareRoot()
$r = $this->value->powMod($temp, static::$modulo[$this->instanceID]);

while (!$t->equals($one)) {
$i = clone $one;

while (!$t->powMod($two->pow($i), static::$modulo[$this->instanceID])->equals($one)) {
$i = $i->add($one);
for ($i == clone $one; $i->compare($m) < 0; $i = $i->add($one)) {
if ($t->powMod($two->pow($i), static::$modulo[$this->instanceID])->equals($one)) {
break;
}
}

if ($i->compare($m) >= 0) {
if ($i->compare($m) == 0) {
return false;
}
$b = $c->powMod($two->pow($m->subtract($i)->subtract($one)), static::$modulo[$this->instanceID]);
Expand Down

9 comments on commit 6298d1c

@terrafrost
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janedbal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any plans porting this to v2?

@terrafrost
Copy link
Member Author

@terrafrost terrafrost commented on 6298d1c Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janedbal - no. The changed files do not even exist in the 2.0 branch:

https://github.com/phpseclib/phpseclib/tree/2.0.41/phpseclib/Math https://github.com/phpseclib/phpseclib/tree/2.0/phpseclib/Math

2.0 does not implement PrimeFields nor does it implement any feature that would even make use of them (eg. elliptic curves, GCM, UHASH) and I am not going to be backporting any of those features to the 2.0 branch.

@janedbal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this report even valid? It says versions <= 2.0.41 are affected.

@terrafrost
Copy link
Member Author

@terrafrost terrafrost commented on 6298d1c Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying issue is valid but the affected version is not valid. The affected versions for that issue are >= 3.0.0 < 3.0.19.

According to that report, using phpseclib 3.0.0 is all fine and dandy whilst 2.0.41 isn't, which is inaccurate.

I am in contact with the people who created the CVE to get it updated. idk how to create CVEs or if it's even possible to update them after the fact but fingers crossed🤞

@janedbal
Copy link

@janedbal janedbal commented on 6298d1c Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I already created fix-proposal github/advisory-database#1752 (hopefully I understood the form correctly), ok thx

@terrafrost
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - I didn't know that their advisory-database had a git repo. Thanks for that!

@janedbal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a button for suggestions like this:
image

I used that.

@terrafrost
Copy link
Member Author

@terrafrost terrafrost commented on 6298d1c Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I hadn't seen that.

Thanks!

That said, I just did a new release of 2.0. Obviously it doesn't fix the non-issue but even if github.com merges the PR there's still Roave/SecurityAdvisories#108 to consider.

Please sign in to comment.