Page MenuHomePhabricator

D579.id1847.diff
No OneTemporary

D579.id1847.diff

diff --git a/bin/controllers/auth.php b/bin/controllers/auth.php
--- a/bin/controllers/auth.php
+++ b/bin/controllers/auth.php
@@ -48,6 +48,9 @@
*/
public function oauth() {
+ $code_challenge = $_GET['code_challenge'];
+ $code_challenge_method = $_GET['code_challenge_method']?? 'plain';
+
/*
* We're going to need the session to provide it's ID to the code challenge
* model.
@@ -104,7 +107,22 @@
*/
$grant = false;
- #TODO: verify the challenge is not malformed
+ /*
+ * The oauth specification requires the server to provide a code challenge
+ * when using PKCE (which our server requires)
+ */
+ if (!is_string($code_challenge) || !is_string($code_challenge_method)) {
+ throw new PublicException('Malformed code_challenge, please review', 400);
+ }
+
+ /*
+ * Our implementation does not accept anything but S256, plain code_challenges
+ * are going to be rejected. These could be intercepted easily.
+ */
+ if ($code_challenge_method != 'S256') {
+ throw new PublicException('Invalid code_challenge_method', 400);
+ }
+
#TODO: Verify the scopes exist
/*
@@ -155,9 +173,9 @@
$challenge->client = $client;
$challenge->user = $this->user;
$challenge->state = $_GET['state'];
- $challenge->challenge = $_GET['challenge'];
+ $challenge->challenge = sprintf('%s:%s', $code_challenge_method, $code_challenge);
$challenge->scope = $_GET['scope'];
- $challenge->redirect = $_GET['redirect'];
+ $challenge->redirect = $redirect;
$challenge->created = time();
$challenge->expires = time() + 180;
$challenge->session = db()->table('session')->get('_id', $session->sessionId())->first();
diff --git a/bin/controllers/token.php b/bin/controllers/token.php
--- a/bin/controllers/token.php
+++ b/bin/controllers/token.php
@@ -36,6 +36,18 @@
$secret = $_POST['secret']?? null;
$expires = Environment::get('phpas.token.expiration')?: 14400;
+ /*
+ * We define the algorithms that the server understands. Since the oauth
+ * spec defines a different name for sha256, we will make our server translates
+ * the hashing algo back.
+ *
+ * We currently do not service anything but sha256.
+ */
+ $hash_algos = [
+ 's256' => 'sha256',
+ 'sha256' => 'sha256'
+ ];
+
/*
* Check if an app with the provided ID does indeed exist.
*/
@@ -88,10 +100,18 @@
}
/*
- * Check the code verifier
+ * Check the code verifier. We need to make sure that the algorhithm
+ * exists, so we don't pass invalid data into the hasing function.
*/
list($algo, $hash) = explode(':', $code->challenge);
-
+
+ if (isset($hash_algos[strtolower($algo)])) {
+ $algo = $hash_algos[strtolower($algo)];
+ }
+ else {
+ throw new PublicException('Invalid hashing algorhithm specified.', 400);
+ }
+
if (hash($algo, $_POST['verifier']) !== $hash) {
throw new PublicException('Hash failed', 403);
}

File Metadata

Mime Type
text/plain
Expires
Tue, Apr 13, 12:08 PM (4 w, 2 h ago)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
7059
Default Alt Text
D579.id1847.diff (2 KB)

Event Timeline