Skip to content

Commit

Permalink
Fix wrong EdDSA key encoding (#437)
Browse files Browse the repository at this point in the history
  • Loading branch information
Spomky authored Jul 15, 2023
1 parent c53f585 commit 3db3259
Show file tree
Hide file tree
Showing 7 changed files with 203 additions and 173 deletions.
35 changes: 25 additions & 10 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -1501,11 +1501,6 @@ parameters:
count: 1
path: src/webauthn/src/AttestationStatement/AppleAttestationStatementSupport.php

-
message: "#^Cannot access offset 1 on array\\|false\\.$#"
count: 3
path: src/webauthn/src/AttestationStatement/AttestationObjectLoader.php

-
message: "#^Parameter \\#1 \\$data of static method Webauthn\\\\TrustPath\\\\TrustPathLoader\\:\\:loadTrustPath\\(\\) expects array, mixed given\\.$#"
count: 1
Expand Down Expand Up @@ -1836,6 +1831,31 @@ parameters:
count: 1
path: src/webauthn/src/AuthenticatorAttestationResponseValidator.php

-
message: "#^Cannot access offset 1 on array\\|false\\.$#"
count: 2
path: src/webauthn/src/AuthenticatorDataLoader.php

-
message: "#^Parameter \\#1 \\$search of function str_replace expects array\\|string, string\\|false given\\.$#"
count: 1
path: src/webauthn/src/AuthenticatorDataLoader.php

-
message: "#^Parameter \\#2 \\$callback of function array_reduce expects callable\\(string, mixed\\)\\: string, Closure\\(string, string\\)\\: non\\-empty\\-string given\\.$#"
count: 1
path: src/webauthn/src/AuthenticatorDataLoader.php

-
message: "#^Parameter \\#2 \\$needle of function mb_strpos expects string, string\\|false given\\.$#"
count: 1
path: src/webauthn/src/AuthenticatorDataLoader.php

-
message: "#^Parameter \\#2 \\$replace of function str_replace expects array\\|string, string\\|false given\\.$#"
count: 1
path: src/webauthn/src/AuthenticatorDataLoader.php

-
message: """
#^Access to deprecated property \\$requireResidentKey of class Webauthn\\\\AuthenticatorSelectionCriteria\\:
Expand Down Expand Up @@ -1890,11 +1910,6 @@ parameters:
count: 1
path: src/webauthn/src/PublicKeyCredentialDescriptorCollection.php

-
message: "#^Cannot access offset 1 on array\\|false\\.$#"
count: 2
path: src/webauthn/src/PublicKeyCredentialLoader.php

-
message: "#^Parameter \\#1 \\$json of method Webauthn\\\\PublicKeyCredentialLoader\\:\\:loadArray\\(\\) expects array, mixed given\\.$#"
count: 1
Expand Down
75 changes: 5 additions & 70 deletions src/webauthn/src/AttestationStatement/AttestationObjectLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,13 @@

use function array_key_exists;
use CBOR\Decoder;
use CBOR\MapObject;
use CBOR\Normalizable;
use function is_array;
use function ord;
use Psr\EventDispatcher\EventDispatcherInterface;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use Symfony\Component\Uid\Uuid;
use Throwable;
use function unpack;
use Webauthn\AttestedCredentialData;
use Webauthn\AuthenticationExtensions\AuthenticationExtensionsClientOutputsLoader;
use Webauthn\AuthenticatorData;
use Webauthn\AuthenticatorDataLoader;
use Webauthn\Event\AttestationObjectLoaded;
use Webauthn\Exception\InvalidDataException;
use Webauthn\MetadataService\CanLogData;
Expand All @@ -29,20 +23,13 @@

class AttestationObjectLoader implements CanDispatchEvents, CanLogData
{
private const FLAG_AT = 0b01000000;

private const FLAG_ED = 0b10000000;

private readonly Decoder $decoder;

private LoggerInterface $logger;

private EventDispatcherInterface $dispatcher;

public function __construct(
private readonly AttestationStatementSupportManager $attestationStatementSupportManager
) {
$this->decoder = Decoder::create();
$this->logger = new NullLogger();
$this->dispatcher = new NullEventDispatcher();
}
Expand All @@ -65,7 +52,7 @@ public function load(string $data): AttestationObject
]);
$decodedData = Base64::decode($data);
$stream = new StringStream($decodedData);
$parsed = $this->decoder->decode($stream);
$parsed = Decoder::create()->decode($stream);

$this->logger->info('Loading the Attestation Statement');
$parsed instanceof Normalizable || throw InvalidDataException::create(
Expand Down Expand Up @@ -94,69 +81,17 @@ public function load(string $data): AttestationObject
$attestationObject,
'Invalid attestation object'
);
$authData = $attestationObject['authData'];

$attestationStatementSupport = $this->attestationStatementSupportManager->get($attestationObject['fmt']);
$attestationStatement = $attestationStatementSupport->load($attestationObject);
$this->logger->info('Attestation Statement loaded');
$this->logger->debug('Attestation Statement loaded', [
'attestationStatement' => $attestationStatement,
]);
$authData = $attestationObject['authData'];
$authDataLoader = AuthenticatorDataLoader::create();
$authenticatorData = $authDataLoader->load($authData);

$authDataStream = new StringStream($authData);
$rp_id_hash = $authDataStream->read(32);
$flags = $authDataStream->read(1);
$signCount = $authDataStream->read(4);
$signCount = unpack('N', $signCount);
$this->logger->debug(sprintf('Signature counter: %d', $signCount[1]));

$attestedCredentialData = null;
if (0 !== (ord($flags) & self::FLAG_AT)) {
$this->logger->info('Attested Credential Data is present');
$aaguid = Uuid::fromBinary($authDataStream->read(16));
$credentialLength = $authDataStream->read(2);
$credentialLength = unpack('n', $credentialLength);
$credentialId = $authDataStream->read($credentialLength[1]);
$credentialPublicKey = $this->decoder->decode($authDataStream);
$credentialPublicKey instanceof MapObject || throw InvalidDataException::create(
$credentialPublicKey,
'The data does not contain a valid credential public key.'
);
$attestedCredentialData = new AttestedCredentialData(
$aaguid,
$credentialId,
(string) $credentialPublicKey
);
$this->logger->info('Attested Credential Data loaded');
$this->logger->debug('Attested Credential Data loaded', [
'at' => $attestedCredentialData,
]);
}

$extension = null;
if (0 !== (ord($flags) & self::FLAG_ED)) {
$this->logger->info('Extension Data loaded');
$extension = $this->decoder->decode($authDataStream);
$extension = AuthenticationExtensionsClientOutputsLoader::load($extension);
$this->logger->info('Extension Data loaded');
$this->logger->debug('Extension Data loaded', [
'ed' => $extension,
]);
}
$authDataStream->isEOF() || throw InvalidDataException::create(
null,
'Invalid authentication data. Presence of extra bytes.'
);
$authDataStream->close();

$authenticatorData = new AuthenticatorData(
$authData,
$rp_id_hash,
$flags,
$signCount[1],
$attestedCredentialData,
$extension
);
$attestationObject = new AttestationObject($data, $attestationStatement, $authenticatorData);
$this->logger->info('Attestation Object loaded');
$this->logger->debug('Attestation Object', [
Expand Down
12 changes: 6 additions & 6 deletions src/webauthn/src/AuthenticatorData.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,17 @@
*/
class AuthenticatorData
{
private const FLAG_UP = 0b00000001;
final public const FLAG_UP = 0b00000001;

private const FLAG_RFU1 = 0b00000010;
final public const FLAG_RFU1 = 0b00000010;

private const FLAG_UV = 0b00000100;
final public const FLAG_UV = 0b00000100;

private const FLAG_RFU2 = 0b00111000;
final public const FLAG_RFU2 = 0b00111000;

private const FLAG_AT = 0b01000000;
final public const FLAG_AT = 0b01000000;

private const FLAG_ED = 0b10000000;
final public const FLAG_ED = 0b10000000;

public function __construct(
protected string $authData,
Expand Down
117 changes: 117 additions & 0 deletions src/webauthn/src/AuthenticatorDataLoader.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
<?php

declare(strict_types=1);

namespace Webauthn;

use CBOR\ByteStringObject;
use CBOR\Decoder;
use CBOR\ListObject;
use CBOR\MapObject;
use CBOR\NegativeIntegerObject;
use CBOR\TextStringObject;
use CBOR\UnsignedIntegerObject;
use function chr;
use function ord;
use Symfony\Component\Uid\Uuid;
use Webauthn\AuthenticationExtensions\AuthenticationExtensionsClientOutputsLoader;
use Webauthn\Exception\InvalidDataException;

final class AuthenticatorDataLoader
{
private readonly Decoder $decoder;

private function __construct()
{
$this->decoder = Decoder::create();
}

public static function create(): self
{
return new self();
}

public function load(string $authData): AuthenticatorData
{
$authData = $this->fixIncorrectEdDSAKey($authData);
$authDataStream = new StringStream($authData);
$rp_id_hash = $authDataStream->read(32);
$flags = $authDataStream->read(1);
$signCount = $authDataStream->read(4);
$signCount = unpack('N', $signCount);

$attestedCredentialData = null;
if (0 !== (ord($flags) & AuthenticatorData::FLAG_AT)) {
$aaguid = Uuid::fromBinary($authDataStream->read(16));
$credentialLength = $authDataStream->read(2);
$credentialLength = unpack('n', $credentialLength);
$credentialId = $authDataStream->read($credentialLength[1]);
$credentialPublicKey = $this->decoder->decode($authDataStream);
$credentialPublicKey instanceof MapObject || throw InvalidDataException::create(
$authData,
'The data does not contain a valid credential public key.'
);
$attestedCredentialData = new AttestedCredentialData(
$aaguid,
$credentialId,
(string) $credentialPublicKey
);
}

$extension = null;
if (0 !== (ord($flags) & AuthenticatorData::FLAG_ED)) {
$extension = $this->decoder->decode($authDataStream);
$extension = AuthenticationExtensionsClientOutputsLoader::load($extension);
}
$authDataStream->isEOF() || throw InvalidDataException::create(
$authData,
'Invalid authentication data. Presence of extra bytes.'
);
$authDataStream->close();

return new AuthenticatorData(
$authData,
$rp_id_hash,
$flags,
$signCount[1],
$attestedCredentialData,
$extension
);
}

private function fixIncorrectEdDSAKey(string $data): string
{
$needle = hex2bin('a301634f4b500327206745643235353139');
$correct = hex2bin('a401634f4b500327206745643235353139');
$position = mb_strpos($data, $needle, 0, '8bit');
if ($position === false) {
return $data;
}

$begin = mb_substr($data, 0, $position, '8bit');
$end = mb_substr($data, $position, null, '8bit');
$end = str_replace($needle, $correct, $end);
$cbor = new StringStream($end);
$badKey = $this->decoder->decode($cbor);

($badKey instanceof MapObject && $cbor->isEOF()) || throw InvalidDataException::create(
$end,
'Invalid authentication data. Presence of extra bytes.'
);
$badX = $badKey->get(-2);
$badX instanceof ListObject || throw InvalidDataException::create($end, 'Invalid authentication data.');
$keyBytes = array_reduce(
$badX->normalize(),
static fn (string $carry, string $item): string => $carry . chr((int) $item),
''
);
$correctX = ByteStringObject::create($keyBytes);
$correctKey = MapObject::create()
->add(UnsignedIntegerObject::create(1), ByteStringObject::create('OKP'))
->add(UnsignedIntegerObject::create(3), NegativeIntegerObject::create(-8))
->add(NegativeIntegerObject::create(-1), TextStringObject::create('Ed25519'))
->add(NegativeIntegerObject::create(-2), $correctX);

return $begin . $correctKey;
}
}
Loading

0 comments on commit 3db3259

Please sign in to comment.