Skip to content

Commit 8e10cda

Browse files
SpomkySpomky
authored
Merge pull request #667 from web-token/fix/ecc-point-cswap-readonly
Repair the conditional swap of the elliptic curve points
2 parents f766779 + 4c2232f commit 8e10cda

4 files changed

Lines changed: 199 additions & 111 deletions

File tree

‎.ci-tools/phpstan-baseline.neon‎

Lines changed: 0 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -6864,102 +6864,6 @@ parameters:
68646864
count: 1
68656865
path: ../src/Library/Core/Util/Ecc/Point.php
68666866

6867-
-
6868-
rawMessage: 'Method Jose\Component\Core\Util\Ecc\Point::cswapBigInteger() has parameter $sa that is passed by reference.'
6869-
identifier: ergebnis.noParameterPassedByReference
6870-
count: 1
6871-
path: ../src/Library/Core/Util/Ecc/Point.php
6872-
6873-
-
6874-
rawMessage: 'Method Jose\Component\Core\Util\Ecc\Point::cswapBigInteger() has parameter $sb that is passed by reference.'
6875-
identifier: ergebnis.noParameterPassedByReference
6876-
count: 1
6877-
path: ../src/Library/Core/Util/Ecc/Point.php
6878-
6879-
-
6880-
rawMessage: 'Method Jose\Component\Core\Util\Ecc\Point::cswapBoolean() has parameter $a that is passed by reference.'
6881-
identifier: ergebnis.noParameterPassedByReference
6882-
count: 1
6883-
path: ../src/Library/Core/Util/Ecc/Point.php
6884-
6885-
-
6886-
rawMessage: 'Method Jose\Component\Core\Util\Ecc\Point::cswapBoolean() has parameter $b that is passed by reference.'
6887-
identifier: ergebnis.noParameterPassedByReference
6888-
count: 1
6889-
path: ../src/Library/Core/Util/Ecc/Point.php
6890-
6891-
-
6892-
rawMessage: 'Parameter #1 $a is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$infinity.'
6893-
identifier: argument.byRef
6894-
count: 1
6895-
path: ../src/Library/Core/Util/Ecc/Point.php
6896-
6897-
-
6898-
rawMessage: 'Parameter #1 $sa is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$order.'
6899-
identifier: argument.byRef
6900-
count: 1
6901-
path: ../src/Library/Core/Util/Ecc/Point.php
6902-
6903-
-
6904-
rawMessage: 'Parameter #1 $sa is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$x.'
6905-
identifier: argument.byRef
6906-
count: 1
6907-
path: ../src/Library/Core/Util/Ecc/Point.php
6908-
6909-
-
6910-
rawMessage: 'Parameter #1 $sa is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$y.'
6911-
identifier: argument.byRef
6912-
count: 1
6913-
path: ../src/Library/Core/Util/Ecc/Point.php
6914-
6915-
-
6916-
rawMessage: 'Parameter #2 $b is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$infinity.'
6917-
identifier: argument.byRef
6918-
count: 1
6919-
path: ../src/Library/Core/Util/Ecc/Point.php
6920-
6921-
-
6922-
rawMessage: 'Parameter #2 $sb is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$order.'
6923-
identifier: argument.byRef
6924-
count: 1
6925-
path: ../src/Library/Core/Util/Ecc/Point.php
6926-
6927-
-
6928-
rawMessage: 'Parameter #2 $sb is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$x.'
6929-
identifier: argument.byRef
6930-
count: 1
6931-
path: ../src/Library/Core/Util/Ecc/Point.php
6932-
6933-
-
6934-
rawMessage: 'Parameter #2 $sb is passed by reference so it does not accept readonly property Jose\Component\Core\Util\Ecc\Point::$y.'
6935-
identifier: argument.byRef
6936-
count: 1
6937-
path: ../src/Library/Core/Util/Ecc/Point.php
6938-
6939-
-
6940-
rawMessage: Readonly property Jose\Component\Core\Util\Ecc\Point::$infinity is assigned outside of the constructor.
6941-
identifier: property.readOnlyAssignNotInConstructor
6942-
count: 2
6943-
path: ../src/Library/Core/Util/Ecc/Point.php
6944-
6945-
-
6946-
rawMessage: Readonly property Jose\Component\Core\Util\Ecc\Point::$order is assigned outside of the constructor.
6947-
identifier: property.readOnlyAssignNotInConstructor
6948-
count: 2
6949-
path: ../src/Library/Core/Util/Ecc/Point.php
6950-
6951-
-
6952-
rawMessage: Readonly property Jose\Component\Core\Util\Ecc\Point::$x is assigned outside of the constructor.
6953-
identifier: property.readOnlyAssignNotInConstructor
6954-
count: 2
6955-
path: ../src/Library/Core/Util/Ecc/Point.php
6956-
6957-
-
6958-
rawMessage: Readonly property Jose\Component\Core\Util\Ecc\Point::$y is assigned outside of the constructor.
6959-
identifier: property.readOnlyAssignNotInConstructor
6960-
count: 2
6961-
path: ../src/Library/Core/Util/Ecc/Point.php
6962-
69636867
-
69646868
rawMessage: Control structures using switch should not be used.
69656869
identifier: ergebnis.noSwitch

‎src/Library/Core/Util/Ecc/Curve.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,10 @@ public function mul(Point $one, BigInteger $n): Point
157157

158158
for ($i = 0; $i < $k; ++$i) {
159159
$j = $n1[$i];
160-
Point::cswap($r[0], $r[1], $j ^ 1);
160+
[$r[0], $r[1]] = Point::cswap($r[0], $r[1], $j ^ 1);
161161
$r[0] = $this->add($r[0], $r[1]);
162162
$r[1] = $this->getDouble($r[1]);
163-
Point::cswap($r[0], $r[1], $j ^ 1);
163+
[$r[0], $r[1]] = Point::cswap($r[0], $r[1], $j ^ 1);
164164
}
165165

166166
$this->validate($r[0]);

‎src/Library/Core/Util/Ecc/Point.php‎

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,26 +70,38 @@ public function getY(): BigInteger
7070
return $this->y;
7171
}
7272

73-
public static function cswap(self $a, self $b, int $cond): void
73+
/**
74+
* Returns both points, swapped when the condition is 1 and unchanged when it is 0.
75+
*
76+
* The points are rebuilt instead of being mutated in place: this class is readonly, so its properties cannot be
77+
* taken by reference.
78+
*
79+
* @return array{self, self}
80+
*/
81+
public static function cswap(self $a, self $b, int $cond): array
7482
{
75-
self::cswapBigInteger($a->x, $b->x, $cond);
76-
self::cswapBigInteger($a->y, $b->y, $cond);
77-
self::cswapBigInteger($a->order, $b->order, $cond);
78-
self::cswapBoolean($a->infinity, $b->infinity, $cond);
83+
[$xA, $xB] = self::cswapBigInteger($a->x, $b->x, $cond);
84+
[$yA, $yB] = self::cswapBigInteger($a->y, $b->y, $cond);
85+
[$orderA, $orderB] = self::cswapBigInteger($a->order, $b->order, $cond);
86+
[$infinityA, $infinityB] = self::cswapBoolean($a->infinity, $b->infinity, $cond);
87+
88+
return [new self($xA, $yA, $orderA, $infinityA), new self($xB, $yB, $orderB, $infinityB)];
7989
}
8090

81-
private static function cswapBoolean(bool &$a, bool &$b, int $cond): void
91+
/**
92+
* @return array{bool, bool}
93+
*/
94+
private static function cswapBoolean(bool $a, bool $b, int $cond): array
8295
{
83-
$sa = BigInteger::of((int) $a);
84-
$sb = BigInteger::of((int) $b);
85-
86-
self::cswapBigInteger($sa, $sb, $cond);
96+
[$sa, $sb] = self::cswapBigInteger(BigInteger::of((int) $a), BigInteger::of((int) $b), $cond);
8797

88-
$a = (bool) $sa->toBase(10);
89-
$b = (bool) $sb->toBase(10);
98+
return [(bool) $sa->toBase(10), (bool) $sb->toBase(10)];
9099
}
91100

92-
private static function cswapBigInteger(BigInteger &$sa, BigInteger &$sb, int $cond): void
101+
/**
102+
* @return array{BigInteger, BigInteger}
103+
*/
104+
private static function cswapBigInteger(BigInteger $sa, BigInteger $sb, int $cond): array
93105
{
94106
$size = max(strlen($sa->toBase(2)), strlen($sb->toBase(2)));
95107
$mask = (string) (1 - $cond);
@@ -103,5 +115,7 @@ private static function cswapBigInteger(BigInteger &$sa, BigInteger &$sb, int $c
103115
->xor($taA);
104116
$sa = $sa->xor($sb)
105117
->xor($taB);
118+
119+
return [$sa, $sb];
106120
}
107121
}

‎tests/Component/Core/EccPointTest.php‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Jose\Tests\Component\Core;
6+
7+
use Brick\Math\BigInteger;
8+
use Jose\Component\Core\Util\Ecc\Curve;
9+
use Jose\Component\Core\Util\Ecc\EcDH;
10+
use Jose\Component\Core\Util\Ecc\NistCurve;
11+
use Jose\Component\Core\Util\Ecc\Point;
12+
use Jose\Component\Core\Util\Ecc\PrivateKey;
13+
use PHPUnit\Framework\Attributes\DataProvider;
14+
use PHPUnit\Framework\Attributes\Test;
15+
use PHPUnit\Framework\TestCase;
16+
use function strlen;
17+
use const STR_PAD_LEFT;
18+
19+
/**
20+
* Point::cswap used to take the properties of a readonly class by reference, which made every scalar multiplication
21+
* throw. These tests cover the conditional swap itself and the operations built on top of it.
22+
*
23+
* @internal
24+
*/
25+
final class EccPointTest extends TestCase
26+
{
27+
#[Test]
28+
public function theConditionalSwapExchangesTheTwoPointsWhenTheConditionIsSet(): void
29+
{
30+
$a = Point::create(BigInteger::of(3), BigInteger::of(5), BigInteger::of(7));
31+
$b = Point::infinity();
32+
33+
[$swappedA, $swappedB] = Point::cswap($a, $b, 1);
34+
35+
static::assertTrue($swappedA->isInfinity());
36+
static::assertFalse($swappedB->isInfinity());
37+
static::assertTrue($swappedB->getX()->isEqualTo(BigInteger::of(3)));
38+
static::assertTrue($swappedB->getY()->isEqualTo(BigInteger::of(5)));
39+
static::assertTrue($swappedB->getOrder()->isEqualTo(BigInteger::of(7)));
40+
}
41+
42+
#[Test]
43+
public function theConditionalSwapKeepsTheTwoPointsWhenTheConditionIsNotSet(): void
44+
{
45+
$a = Point::create(BigInteger::of(3), BigInteger::of(5), BigInteger::of(7));
46+
$b = Point::infinity();
47+
48+
[$keptA, $keptB] = Point::cswap($a, $b, 0);
49+
50+
static::assertFalse($keptA->isInfinity());
51+
static::assertTrue($keptB->isInfinity());
52+
static::assertTrue($keptA->getX()->isEqualTo(BigInteger::of(3)));
53+
static::assertTrue($keptA->getY()->isEqualTo(BigInteger::of(5)));
54+
static::assertTrue($keptA->getOrder()->isEqualTo(BigInteger::of(7)));
55+
}
56+
57+
#[Test]
58+
public function theConditionalSwapLeavesTheGivenPointsUntouched(): void
59+
{
60+
$a = Point::create(BigInteger::of(3), BigInteger::of(5), BigInteger::of(7));
61+
$b = Point::infinity();
62+
63+
Point::cswap($a, $b, 1);
64+
65+
static::assertFalse($a->isInfinity());
66+
static::assertTrue($a->getX()->isEqualTo(BigInteger::of(3)));
67+
static::assertTrue($b->isInfinity());
68+
}
69+
70+
#[Test]
71+
#[DataProvider('curves')]
72+
public function theGeneratorMultipliedByItsOrderIsTheInfinity(Curve $curve): void
73+
{
74+
$generator = $curve->getGenerator();
75+
76+
static::assertTrue($curve->mul($generator, $generator->getOrder())->isInfinity());
77+
}
78+
79+
/**
80+
* The key pairs and the shared secrets were produced by OpenSSL.
81+
*/
82+
#[Test]
83+
#[DataProvider('keyPairs')]
84+
public function thePublicKeyComputedByOpensslIsFoundAgain(
85+
Curve $curve,
86+
string $privateKey,
87+
string $x,
88+
string $y,
89+
string $peerX,
90+
string $peerY,
91+
string $expectedSecret
92+
): void {
93+
$publicKey = $curve->createPublicKey(PrivateKey::create(BigInteger::fromBase($privateKey, 16)));
94+
95+
static::assertTrue($publicKey->getPoint()->getX()->isEqualTo(BigInteger::fromBase($x, 16)));
96+
static::assertTrue($publicKey->getPoint()->getY()->isEqualTo(BigInteger::fromBase($y, 16)));
97+
}
98+
99+
#[Test]
100+
#[DataProvider('keyPairs')]
101+
public function theSharedSecretIsTheOneComputedByOpenssl(
102+
Curve $curve,
103+
string $privateKey,
104+
string $x,
105+
string $y,
106+
string $peerX,
107+
string $peerY,
108+
string $expectedSecret
109+
): void {
110+
static::assertNotSame($x, $peerX);
111+
static::assertNotSame($y, $peerY);
112+
113+
$sharedSecret = EcDH::computeSharedKey(
114+
$curve,
115+
$curve->getPublicKeyFrom(BigInteger::fromBase($peerX, 16), BigInteger::fromBase($peerY, 16)),
116+
PrivateKey::create(BigInteger::fromBase($privateKey, 16))
117+
);
118+
119+
static::assertSame(
120+
$expectedSecret,
121+
str_pad($sharedSecret->toBase(16), strlen($expectedSecret), '0', STR_PAD_LEFT)
122+
);
123+
}
124+
125+
/**
126+
* @return iterable<string, array{Curve}>
127+
*/
128+
public static function curves(): iterable
129+
{
130+
yield 'P-256' => [NistCurve::curve256()];
131+
yield 'P-384' => [NistCurve::curve384()];
132+
yield 'P-521' => [NistCurve::curve521()];
133+
}
134+
135+
/**
136+
* @return iterable<string, array{Curve, string, string, string, string, string, string}>
137+
*/
138+
public static function keyPairs(): iterable
139+
{
140+
yield 'P-256' => [
141+
NistCurve::curve256(),
142+
'd24217e163d336415e5de5e6ca91b347975b9e724c5faf9baf6348e3cefcee32',
143+
'cf3697dd1416399bf340451880833f4a079f9947e723d4eadb7bfbdda0189b7d',
144+
'c3343bd6745a4674356f4496edc44912ff2a8b43123aa3418ac9c4a3676224ac',
145+
'8ad8fdb9c9cf93754117ade70489c9c72f499bdb762dc0b4b1ec1bde62d125aa',
146+
'0c8921842efbea856e4ff4691464f50d916b2f6036b37b713f81912c08a3c911',
147+
'6e7d837ea69b950234653748d6567d447d09865ce78920f24b98f89bb7754dc6',
148+
];
149+
150+
yield 'P-384' => [
151+
NistCurve::curve384(),
152+
'6ebdce3bf8de96541b7e03f5c784dbfbd7e3608b75256a2b318d0697bbe0e58a7ffed1b1b87794c30aaf53997a3f05da',
153+
'7dd63d67e3e9c90dd5c843495db4bd4ffab5eaa7522e5a2149990e0b8376caeb7686e9ad8585ac88f9c39218eb82f2d5',
154+
'4e45f17b05451b58390a6b6f720b5d2066f88aab155be190217baddd94ef23cf2972fa3551f553b14f9ec6e6cc47c61f',
155+
'aba57e60777227dcb258842da33a6da38e7ec0ea946a81fb6831d2bac37b1c6041bbb7b558931059a4ff40e11c60f749',
156+
'1013a12ff9480976dc641521aedfd5f6b416fb4848ff09379c0214fa52580d73a9071df6b55df4c1410c764fb196f40b',
157+
'fc8da7854eef964e2d40a5aa900d8cfda9fe8d1b85e6a5b530d2cba793ff3574cf92b1bce44f74cace8b5eeee4ff336b',
158+
];
159+
160+
yield 'P-521' => [
161+
NistCurve::curve521(),
162+
'00a9117ddf921446f08a60064b60d06e07687756c3a9bd8af331a3a1aa7d916c448da08dec839205455b812eac76a1e266cd118f2f207f8edcaba1cc750a8d9fcba5',
163+
'00f4fa0645be2fb177902720e1db522a780dd9e00048c185f235e01a6094e1612adaa655ea81314cc941cb5db736b456c77d7d46e41bf0502de8c6686515358be136',
164+
'00675cded5ac1eb20d9b27724911110afb482177da76d51e5428d3ad9abbb51e80bb3b15cf99fb88f502b19f716b2cf55a850dde5224e746252446357edffb51ee24',
165+
'012b39f908ed9c135cb7c5d6ad0757ffc18c63ca3e0fc26427a294fee6ae2d21132941a9a3f12c41f47d78104b88f7ec1bcb28a5c9e56f299645170bf9570194dd82',
166+
'00b650a42909de7554e03ba25b481292f9b216b1a1e9f7e7559950a380c5b1de7d06f113703e76b8d4137835430583a2426468cdccb567938a9eadbc028ab3defd6a',
167+
'018a83291a6ec33cc4065a89523a5eeccadcb94e58ea107b13a19f0a9d07eee981a3296aa4902607bd269c59c85ce1c88c1a1ba02ae70dc6d1cf6a8427bc5dc78cc6',
168+
];
169+
}
170+
}

0 commit comments

Comments
 (0)