From 6c551b08d863dd38635e768b650dd89731edd5ef Mon Sep 17 00:00:00 2001 From: Roman Date: Sun, 7 Apr 2024 18:29:33 +0200 Subject: [PATCH] few bugfixes, fido/u2f still WIP --- Core/API/Request.class.php | 5 +- Core/API/TfaAPI.class.php | 7 +- Core/API/UserAPI.class.php | 16 +++-- Core/API/VisitorsAPI.class.php | 2 +- Core/Objects/Context.class.php | 1 + .../TwoFactor/AttestationObject.class.php | 7 +- .../TwoFactor/AuthenticationData.class.php | 44 ++++++++---- Core/Objects/TwoFactor/CBORDecoder.trait.php | 5 +- .../KeyBasedTwoFactorToken.class.php | 2 +- Core/Objects/TwoFactor/PublicKey.class.php | 22 +++--- index.php | 2 +- js/account.js | 2 +- react/admin-panel/package.json | 2 +- react/admin-panel/src/App.jsx | 12 +++- .../admin-panel/src/views/profile/mfa-fido.js | 69 +++++++++++++++++-- .../admin-panel/src/views/profile/mfa-totp.js | 10 +-- .../admin-panel/src/views/profile/profile.js | 8 ++- react/shared/api.js | 12 ++-- react/shared/views/login.jsx | 3 +- 19 files changed, 164 insertions(+), 67 deletions(-) diff --git a/Core/API/Request.class.php b/Core/API/Request.class.php index ed90c21..48b3530 100644 --- a/Core/API/Request.class.php +++ b/Core/API/Request.class.php @@ -255,8 +255,11 @@ abstract class Request { $this->success = $req->execute(["method" => self::getEndpoint()]); $this->lastError = $req->getLastError(); if (!$this->success) { + $res = $req->getResult(); if (!$this->context->getUser()) { $this->result["loggedIn"] = false; + } else if (isset($res["twoFactorToken"])) { + $this->result["twoFactorToken"] = $res["twoFactorToken"]; } return false; } @@ -284,7 +287,7 @@ abstract class Request { // this should actually not occur, how to handle this case? $this->success = $success; } - } catch (\Error $err) { + } catch (\Throwable $err) { http_response_code(500); $this->createError($err->getMessage()); $this->logger->error($err->getMessage()); diff --git a/Core/API/TfaAPI.class.php b/Core/API/TfaAPI.class.php index 8e8366e..a746611 100644 --- a/Core/API/TfaAPI.class.php +++ b/Core/API/TfaAPI.class.php @@ -61,7 +61,6 @@ namespace Core\API\TFA { use Core\API\Parameter\StringType; use Core\API\TfaAPI; - use Core\Driver\SQL\Condition\Compare; use Core\Driver\SQL\Query\Insert; use Core\Objects\Context; use Core\Objects\TwoFactor\AttestationObject; @@ -265,10 +264,7 @@ namespace Core\API\TFA { $settings = $this->context->getSettings(); $relyingParty = $settings->getSiteName(); $sql = $this->context->getSQL(); - - // TODO: for react development, localhost / HTTP_HOST is required, otherwise a DOMException is thrown $domain = parse_url($settings->getBaseUrl(), PHP_URL_HOST); - // $domain = "localhost"; if (!$clientDataJSON || !$attestationObjectRaw) { $challenge = null; @@ -329,12 +325,13 @@ namespace Core\API\TFA { return $this->createError("Unsupported key type. Expected: -7"); } + $twoFactorToken->authenticate(); $this->success = $twoFactorToken->confirmKeyBased($sql, base64_encode($authData->getCredentialID()), $publicKey) !== false; $this->lastError = $sql->getLastError(); if ($this->success) { $this->result["twoFactorToken"] = $twoFactorToken->jsonSerialize(); - $this->context->invalidateSessions(); + $this->context->invalidateSessions(true); } } diff --git a/Core/API/UserAPI.class.php b/Core/API/UserAPI.class.php index 5b84257..f48d71b 100644 --- a/Core/API/UserAPI.class.php +++ b/Core/API/UserAPI.class.php @@ -146,6 +146,7 @@ namespace Core\API\User { use Core\Driver\SQL\Condition\Compare; use Core\Driver\SQL\Condition\CondIn; use Core\Driver\SQL\Expression\JsonArrayAgg; + use Core\Objects\TwoFactor\KeyBasedTwoFactorToken; use ImagickException; use Core\Objects\Context; use Core\Objects\DatabaseEntity\GpgKey; @@ -374,6 +375,12 @@ namespace Core\API\User { $this->result["loggedIn"] = false; $userGroups = []; } else { + + $twoFactorToken = $currentUser->getTwoFactorToken(); + if ($twoFactorToken instanceof KeyBasedTwoFactorToken && !$twoFactorToken->hasChallenge()) { + $twoFactorToken->generateChallenge(); + } + $this->result["loggedIn"] = true; $userGroups = array_keys($currentUser->getGroups()); $this->result["user"] = $currentUser->jsonSerialize(); @@ -629,7 +636,7 @@ namespace Core\API\User { $this->result["loggedIn"] = true; $this->result["user"] = $user->jsonSerialize(); - $this->result["session"] = $session->jsonSerialize(); + $this->result["session"] = $session->jsonSerialize(["expires", "csrfToken"]); $this->result["logoutIn"] = $session->getExpiresSeconds(); $this->check2FA($tfaToken); $this->success = true; @@ -1310,6 +1317,7 @@ namespace Core\API\User { } $settings = $this->context->getSettings(); + $siteName = htmlspecialchars($settings->getSiteName()); $baseUrl = htmlspecialchars($settings->getBaseUrl()); $token = htmlspecialchars(urlencode($token)); $url = "$baseUrl/confirmGPG?token=$token"; @@ -1317,14 +1325,12 @@ namespace Core\API\User { "you imported a GPG public key for end-to-end encrypted mail communication. " . "To confirm the key and verify, you own the corresponding private key, please click on the following link. " . "The link is active for one hour.

" . - "$url
- Best Regards
" . - $settings->getSiteName() . " Administration"; + "$url
Best Regards
$siteName Administration"; $sendMail = new \Core\API\Mail\Send($this->context); $this->success = $sendMail->execute(array( "to" => $currentUser->getEmail(), - "subject" => $settings->getSiteName() . " - Confirm GPG-Key", + "subject" => "[$siteName] Confirm GPG-Key", "body" => $mailBody, "gpgFingerprint" => $gpgKey->getFingerprint() )); diff --git a/Core/API/VisitorsAPI.class.php b/Core/API/VisitorsAPI.class.php index fd7b1f1..78a160b 100644 --- a/Core/API/VisitorsAPI.class.php +++ b/Core/API/VisitorsAPI.class.php @@ -29,7 +29,7 @@ namespace Core\API\Visitors { class ProcessVisit extends VisitorsAPI { public function __construct(Context $context, bool $externalCall = false) { parent::__construct($context, $externalCall, array( - "cookie" => new StringType("cookie") + "cookie" => new StringType("cookie", 26) )); $this->isPublic = false; } diff --git a/Core/Objects/Context.class.php b/Core/Objects/Context.class.php index 52f7287..aa32c50 100644 --- a/Core/Objects/Context.class.php +++ b/Core/Objects/Context.class.php @@ -91,6 +91,7 @@ class Context { } public function sendCookies(): void { + // TODO: what will we do, when there is a domain mismatch? forbid access or just send cookies for the current domain? or should we send a redirect? $domain = $this->getSettings()->getDomain(); $this->language->sendCookie($domain); $this->session?->sendCookie($domain); diff --git a/Core/Objects/TwoFactor/AttestationObject.class.php b/Core/Objects/TwoFactor/AttestationObject.class.php index 47d5ea1..b6d6cfd 100644 --- a/Core/Objects/TwoFactor/AttestationObject.class.php +++ b/Core/Objects/TwoFactor/AttestationObject.class.php @@ -2,6 +2,7 @@ namespace Core\Objects\TwoFactor; +use CBOR\MapObject; use Core\Objects\ApiObject; class AttestationObject extends ApiObject { @@ -9,14 +10,14 @@ class AttestationObject extends ApiObject { use CBORDecoder; private string $format; - private array $statement; + private MapObject $statement; private AuthenticationData $authData; public function __construct(string $buffer) { - $data = $this->decode($buffer)->getNormalizedData(); + $data = $this->decode($buffer); $this->format = $data["fmt"]; $this->statement = $data["attStmt"]; - $this->authData = new AuthenticationData($data["authData"]); + $this->authData = new AuthenticationData($data["authData"]->getValue()); } public function jsonSerialize(): array { diff --git a/Core/Objects/TwoFactor/AuthenticationData.class.php b/Core/Objects/TwoFactor/AuthenticationData.class.php index b84d383..a9af149 100644 --- a/Core/Objects/TwoFactor/AuthenticationData.class.php +++ b/Core/Objects/TwoFactor/AuthenticationData.class.php @@ -6,31 +6,47 @@ use Core\Objects\ApiObject; class AuthenticationData extends ApiObject { + use CBORDecoder; + + const FLAG_USER_PRESENT = 1; + const FLAG_USER_VERIFIED = 4; + const FLAG_ATTESTED_DATA_INCLUDED = 64; + const FLAG_EXTENSION_DATA_INCLUDED = 128; + private string $rpIDHash; private int $flags; - private int $counter; + private int $signCount; private string $aaguid; private string $credentialID; + private array $extensions; private PublicKey $publicKey; public function __construct(string $buffer) { - if (strlen($buffer) < 32 + 1 + 4) { + $bufferLength = strlen($buffer); + if ($bufferLength < 32 + 1 + 4) { throw new \Exception("Invalid authentication data buffer size"); } $offset = 0; $this->rpIDHash = substr($buffer, $offset, 32); $offset += 32; - $this->flags = ord($buffer[$offset]); $offset += 1; - $this->counter = unpack("N", $buffer, $offset)[1]; $offset += 4; + $this->flags = ord(substr($buffer, $offset, 1)); $offset += 1; + $this->signCount = unpack("N", substr($buffer, $offset, 4))[1]; $offset += 4; - if (strlen($buffer) >= $offset + 4 + 2) { + if ($this->attestedCredentialData()) { $this->aaguid = substr($buffer, $offset, 16); $offset += 16; - $credentialIdLength = unpack("n", $buffer, $offset)[1]; $offset += 2; + $credentialIdLength = unpack("n", substr($buffer, $offset, 4))[1]; $offset += 2; $this->credentialID = substr($buffer, $offset, $credentialIdLength); $offset += $credentialIdLength; - $credentialData = substr($buffer, $offset); - $this->publicKey = new PublicKey($credentialData); + if ($offset < $bufferLength) { + $publicKeyData = $this->decode(substr($buffer, $offset)); + $this->publicKey = new PublicKey($publicKeyData); + // TODO: we should add $publicKeyData->length to $offset, but it's not implemented yet?; + } + } + + if ($this->hasExtensionData()) { + // not supported yet } } @@ -38,7 +54,7 @@ class AuthenticationData extends ApiObject { return [ "rpIDHash" => base64_encode($this->rpIDHash), "flags" => $this->flags, - "counter" => $this->counter, + "signCount" => $this->signCount, "aaguid" => base64_encode($this->aaguid), "credentialID" => base64_encode($this->credentialID), "publicKey" => $this->publicKey->jsonSerialize() @@ -54,26 +70,26 @@ class AuthenticationData extends ApiObject { } public function isUserPresent(): bool { - return boolval($this->flags & (1 << 0)); + return boolval($this->flags & self::FLAG_USER_PRESENT); } public function isUserVerified(): bool { - return boolval($this->flags & (1 << 2)); + return boolval($this->flags & self::FLAG_USER_VERIFIED); } public function attestedCredentialData(): bool { - return boolval($this->flags & (1 << 6)); + return boolval($this->flags & self::FLAG_ATTESTED_DATA_INCLUDED); } public function hasExtensionData(): bool { - return boolval($this->flags & (1 << 7)); + return boolval($this->flags & self::FLAG_EXTENSION_DATA_INCLUDED); } public function getPublicKey(): PublicKey { return $this->publicKey; } - public function getCredentialID() { + public function getCredentialID(): string { return $this->credentialID; } } \ No newline at end of file diff --git a/Core/Objects/TwoFactor/CBORDecoder.trait.php b/Core/Objects/TwoFactor/CBORDecoder.trait.php index c42d3c3..4bd1b26 100644 --- a/Core/Objects/TwoFactor/CBORDecoder.trait.php +++ b/Core/Objects/TwoFactor/CBORDecoder.trait.php @@ -2,14 +2,13 @@ namespace Core\Objects\TwoFactor; +use CBOR\Decoder; use CBOR\StringStream; trait CBORDecoder { protected function decode(string $buffer): \CBOR\CBORObject { - $objectManager = new \CBOR\OtherObject\OtherObjectManager(); - $tagManager = new \CBOR\Tag\TagObjectManager(); - $decoder = new \CBOR\Decoder($tagManager, $objectManager); + $decoder = Decoder::create(); return $decoder->decode(new StringStream($buffer)); } diff --git a/Core/Objects/TwoFactor/KeyBasedTwoFactorToken.class.php b/Core/Objects/TwoFactor/KeyBasedTwoFactorToken.class.php index a2f7390..a5f5e4d 100644 --- a/Core/Objects/TwoFactor/KeyBasedTwoFactorToken.class.php +++ b/Core/Objects/TwoFactor/KeyBasedTwoFactorToken.class.php @@ -37,7 +37,7 @@ class KeyBasedTwoFactorToken extends TwoFactorToken { } public function hasChallenge(): bool { - return isset($this->challenge); + return isset($this->challenge) && !empty($this->challenge); } public function getChallenge(): string { diff --git a/Core/Objects/TwoFactor/PublicKey.class.php b/Core/Objects/TwoFactor/PublicKey.class.php index 81b2527..74d295b 100644 --- a/Core/Objects/TwoFactor/PublicKey.class.php +++ b/Core/Objects/TwoFactor/PublicKey.class.php @@ -2,26 +2,24 @@ namespace Core\Objects\TwoFactor; +use CBOR\MapObject; use Core\Objects\ApiObject; class PublicKey extends ApiObject { - use CBORDecoder; - private int $keyType; private int $usedAlgorithm; private int $curveType; private string $xCoordinate; private string $yCoordinate; - public function __construct(?string $cborData = null) { - if ($cborData) { - $data = $this->decode($cborData)->getNormalizedData(); - $this->keyType = $data["1"]; - $this->usedAlgorithm = $data["3"]; - $this->curveType = $data["-1"]; - $this->xCoordinate = $data["-2"]; - $this->yCoordinate = $data["-3"]; + public function __construct(?MapObject $publicKeyData = null) { + if ($publicKeyData) { + $this->keyType = $publicKeyData["1"]->getValue(); + $this->usedAlgorithm = $publicKeyData["3"]->getValue(); + $this->curveType = $publicKeyData["-1"]->getValue(); + $this->xCoordinate = $publicKeyData["-2"]->getValue(); + $this->yCoordinate = $publicKeyData["-3"]->getValue(); } } @@ -53,8 +51,8 @@ class PublicKey extends ApiObject { public function getNormalizedData(): array { return [ - "1" => $this->keyType, - "3" => $this->usedAlgorithm, + "1" => $this->keyType, + "3" => $this->usedAlgorithm, "-1" => $this->curveType, "-2" => $this->xCoordinate, "-3" => $this->yCoordinate, diff --git a/index.php b/index.php index a185703..301c9df 100644 --- a/index.php +++ b/index.php @@ -67,7 +67,7 @@ if ($installation) { } else { try { $response = $router->run($requestedUri); - } catch (\Error $e) { + } catch (\Throwable $e) { http_response_code(500); $router->getLogger()->error($e->getMessage()); $response = $router->returnStatusCode(500); diff --git a/js/account.js b/js/account.js index 4361e2f..1346c74 100644 --- a/js/account.js +++ b/js/account.js @@ -51,7 +51,7 @@ $(document).ready(function () { btn.prop("disabled", false); $("#password").val(""); createdDiv.hide(); - if (res.emailConfirmed === false) { + if (res.user.confirmed === false) { showAlert("danger", res.msg + ' Click here to resend the confirmation mail.', true); } else { showAlert("danger", res.msg); diff --git a/react/admin-panel/package.json b/react/admin-panel/package.json index 9b0940a..782a195 100644 --- a/react/admin-panel/package.json +++ b/react/admin-panel/package.json @@ -6,7 +6,7 @@ "react": "^18.2.0" }, "scripts": { - "dev": "react-app-rewired start" + "dev": "HTTPS=true react-app-rewired start" }, "author": "", "license": "ISC", diff --git a/react/admin-panel/src/App.jsx b/react/admin-panel/src/App.jsx index 86c1f22..10ef2f0 100644 --- a/react/admin-panel/src/App.jsx +++ b/react/admin-panel/src/App.jsx @@ -28,6 +28,14 @@ export default function App() { }); }, [api]); + const onLogout = useCallback(() => { + api.logout().then(data => { + if (!data.success) { + setError("Error logging out: " + data.msg); + } + }); + }, [api]); + const onInit = useCallback((force = false) => { if (loaded && !force) { return; @@ -97,8 +105,8 @@ export default function App() { } else { return {L("general.loading")}… } - } else if (!user || !api.loggedIn) { - return + } else if (!user || !api.loggedIn || (api.user.twoFactorToken?.confirmed && !api.user.twoFactorToken.authenticated)) { + return } else { return } diff --git a/react/admin-panel/src/views/profile/mfa-fido.js b/react/admin-panel/src/views/profile/mfa-fido.js index 8267dc6..9275d39 100644 --- a/react/admin-panel/src/views/profile/mfa-fido.js +++ b/react/admin-panel/src/views/profile/mfa-fido.js @@ -1,17 +1,76 @@ -import {Box, Paper} from "@mui/material"; +import {Box, CircularProgress, Paper} from "@mui/material"; import {LocaleContext} from "shared/locale"; import {useCallback, useContext} from "react"; +import {decodeText, encodeText} from "shared/util"; export default function MfaFido(props) { - const {api, showDialog, setDialogData, ...other} = props; + const {api, showDialog, setDialogData, set2FA, ...other} = props; const {translate: L} = useContext(LocaleContext); const openDialog = useCallback(() => { - if (api.hasPermission("tfa/registerKey")) { - + if (!api.hasPermission("tfa/registerKey")) { + return; } - }, [api, showDialog]); + + if (typeof navigator.credentials !== 'object' || typeof navigator.credentials.create !== 'function') { + showDialog(L("Key-based Two-Factor-Authentication (2FA) is not supported on this device."), L("Not supported")); + } + + api.register2FA().then(res => { + if (!res.success) { + showDialog(res.msg, L("Error registering 2FA-Device")); + return; + } + + setDialogData({ + show: true, + title: L("Register a 2FA-Device"), + message: L("You may need to interact with your Device, e.g. typing in your PIN or touching to confirm the registration."), + inputs: [ + { type: "custom", key: "progress", element: CircularProgress } + ], + options: [L("general.cancel")], + }) + + navigator.credentials.create({ + publicKey: { + challenge: encodeText(window.atob(res.data.challenge)), + rp: res.data.relyingParty, + user: { + id: encodeText(res.data.id), + name: api.user.name, + displayName: api.user.fullName + }, + userVerification: "discouraged", + attestation: "direct", + pubKeyCredParams: [{ + type: "public-key", + alg: -7, // "ES256" IANA COSE Algorithms registry + }] + } + }).then(res => { + if (res.response) { + let clientDataJSON = decodeText(res.response.clientDataJSON); + let attestationObject = window.btoa(String.fromCharCode.apply(null, new Uint8Array(res.response.attestationObject))); + api.register2FA(clientDataJSON, attestationObject).then((res) => { + setDialogData({show: false}); + if (res.success) { + showDialog(L("account.confirm_fido_success"), L("general.success")); + set2FA({ confirmed: true, type: "fido", authenticated: true }); + } else { + showDialog(res.msg, L("Error registering 2FA-Device")); + } + }); + } else { + showDialog(JSON.stringify(res), L("Error registering 2FA-Device")); + } + }).catch(ex => { + setDialogData({show: false}); + showDialog(ex.toString(), L("Error registering 2FA-Device")); + }); + }); + }, [api, showDialog, setDialogData, set2FA]); const disabledStyle = { background: "gray", diff --git a/react/admin-panel/src/views/profile/mfa-totp.js b/react/admin-panel/src/views/profile/mfa-totp.js index e8f6848..c7296df 100644 --- a/react/admin-panel/src/views/profile/mfa-totp.js +++ b/react/admin-panel/src/views/profile/mfa-totp.js @@ -4,7 +4,7 @@ import {LocaleContext} from "shared/locale"; export default function MfaTotp(props) { - const {setDialogData, api, showDialog, ...other} = props; + const {setDialogData, api, showDialog, set2FA, ...other} = props; const {translate: L} = useContext(LocaleContext); const onConfirmTOTP = useCallback((code) => { @@ -14,10 +14,11 @@ export default function MfaTotp(props) { } else { setDialogData({show: false}); showDialog(L("account.confirm_totp_success"), L("general.success")); + set2FA({ confirmed: true, type: "totp", authenticated: true }); } }); return false; - }, [api, showDialog]); + }, [api, showDialog, set2FA, setDialogData]); const openDialog = useCallback(() => { if (api.hasPermission("tfa/generateQR")) { @@ -28,8 +29,8 @@ export default function MfaTotp(props) { "On Android, you can use the Google Authenticator."), inputs: [ { - type: "custom", element: Box, textAlign: "center", children: - {"[QR-Code]"}/ + type: "custom", element: Box, textAlign: "center", key: "qr-code", + children: {"[QR-Code]"} }, { type: "number", placeholder: L("account.6_digit_code"), @@ -37,6 +38,7 @@ export default function MfaTotp(props) { sx: { "& input": { textAlign: "center", fontFamily: "monospace" } }, } ], + options: [L("general.ok"), L("general.cancel")], onOption: (option, data) => option === 0 ? onConfirmTOTP(data.code) : true }) } diff --git a/react/admin-panel/src/views/profile/profile.js b/react/admin-panel/src/views/profile/profile.js index 15b588d..1910c4d 100644 --- a/react/admin-panel/src/views/profile/profile.js +++ b/react/admin-panel/src/views/profile/profile.js @@ -388,8 +388,10 @@ export default function ProfileView(props) { : - - + setProfile({...profile, twoFactorToken: token })} /> + setProfile({...profile, twoFactorToken: token })} /> } @@ -409,7 +411,7 @@ export default function ProfileView(props) { message={dialogData.message} inputs={dialogData.inputs} onClose={() => setDialogData({show: false})} - options={[L("general.ok"), L("general.cancel")]} + options={dialogData.options} onOption={dialogData.onOption} /> } \ No newline at end of file diff --git a/react/shared/api.js b/react/shared/api.js index eb051b0..bb32c43 100644 --- a/react/shared/api.js +++ b/react/shared/api.js @@ -47,10 +47,14 @@ export default class API { } let res = await response.json(); - if (!res.success && res.loggedIn === false) { - this.loggedIn = false; - this.user = null; - this.session = null; + if (!res.success) { + if (res.loggedIn === false) { + this.loggedIn = false; + this.user = null; + this.session = null; + } else if (res.twoFactorToken === true) { + this.user.twoFactorToken = res.twoFactorToken; + } } return res; diff --git a/react/shared/views/login.jsx b/react/shared/views/login.jsx index 4a3b985..015c39c 100644 --- a/react/shared/views/login.jsx +++ b/react/shared/views/login.jsx @@ -138,6 +138,7 @@ export default function LoginForm(props) { type: "public-key", }], userVerification: "discouraged", + attestation: "direct", }, signal: abortSignal }).then((res) => { @@ -207,7 +208,7 @@ export default function LoginForm(props) { {tfaToken.step !== 2 ? - : + :
{L("general.something_went_wrong")}:
{tfaToken.error}