altcha-org / altcha-lib

A JavaScript library for creating and verifying ALTCHA challenges.
https://altcha.org
MIT License
18 stars 5 forks source link

Wrong server signature when using the Spam filter #5

Closed MrTimber closed 1 month ago

MrTimber commented 1 month ago

In the general case, the solution can be validated using algorithm, challenge, number,salt, and signature:
Solution validation

When we use the Spam filter, it's a bit different, we have to use algorithm, verificationData, and signature: Verifying the Signature (and then we can verify fields)

Although it's quite simple, I can't seem to validate the signature in PHP. So I looked at and tested the JavaScript code of altcha-lib.

We can pass either an ArrayBuffer or a string as the data parameter to the hmac function.
Then data is converted to an Uint8Array to pass it to the crypto.subtle.sign function,

image

But depending on whether we use the ArrayBuffer version or the string version, the result is not the same!

You can reproduce the issue using this code on tests/serversignature.test.ts, where hmacHex('SHA-256', hash256_ab, hmacKey) and hmacHex('SHA-256', hash256_str, hmacKey) give different results:

import { describe, expect, it } from 'vitest';
import { verifyServerSignature } from '../lib/index.js';
import { ab2hex, encoder, hash, hmacHex } from '../lib/helpers.js';

describe('server signature', () => {
  const hmacKey = 'test key';

  describe('verifyServerSignature()', () => {
    it('shoult return verified', async () => {
      const time = Math.floor(Date.now() / 1000);
      const verificationData = new URLSearchParams({
        email: 'čžýěžě@sfffd.net',
        expire: String(time + 10000),
        time: String(time),
        verified: String(true),
      }).toString();

      /** BEGIN - Using array buffer version of hash256 */
      console.log('# Using array buffer version of hash256\n');

      // Hash256 array buffer
      const hash256_ab= await hash('SHA-256', verificationData);
      console.log('hash256_ab:', hash256_ab);

      // Buffer source from Hash256 array buffer
      // (this is what is used internally by the 'hmac' function)
      const buffer_source_from_hash256_ab = new Uint8Array(hash256_ab);
      console.log('buffer_source_from_hash256_ab:', buffer_source_from_hash256_ab);

      // Signature (wrong)
      const signature_from_hash256_ab = await hmacHex(
        'SHA-256',
        hash256_ab,
        hmacKey
      );
      console.log('signature_from_hash256_ab:', signature_from_hash256_ab);

      const payload_from_hash256_ab = btoa(
        JSON.stringify({
          algorithm: 'SHA-256',
          signature_from_hash256_ab,
          verificationData,
          verified: true,
        })
      );
      console.log('payload_from_hash256_ab:', payload_from_hash256_ab);

      /** END - Using array buffer version of hash256 */

      /** BEGIN - Using string version of hash256 */
      console.log('\n# Using string version of hash256\n');

        // Hash256 string
      const hash256_str = ab2hex(hash256_ab);
      console.log('hash256_str:', hash256_str);

      // Buffer source from Hash256 string
      // (this is what is used internally by the 'hmac' function)
      const buffer_source_from_hash256_str = encoder.encode(hash256_str);
      console.log('buffer_source_from_hash256_str:', buffer_source_from_hash256_str);

      // Signature (correct)
      const signature_from_hash256_str = await hmacHex(
        'SHA-256',
        hash256_str,
        hmacKey
      );
      console.log('signature_from_hash256_str:', signature_from_hash256_str);

      const signature = signature_from_hash256_str;
      const payload_from_hash256_str = btoa(
        JSON.stringify({
          algorithm: 'SHA-256',
          signature,
          verificationData,
          verified: true,
        })
      );
      console.log('payload_from_hash256_str:', payload_from_hash256_str);

      /** END - Using string version of hash256 */

      console.log('\n# Verify\n');
      const result = await verifyServerSignature(payload_from_hash256_str, hmacKey);
      console.log('result:', result);
      expect(result.verified).toEqual(true);
    });
  });
});

And so for the test to pass you also have to modify lib/index.ts accordingly:
image

When passing the hash as a string, the calculated signature is correct and can actually be verified as indicated in the documentation

But this is not a solution because it would also have to work when passing the hash as an ArrayBuffer.

I don't know how to do it, any ideas?

MrTimber commented 1 month ago

EDITED: Compute the hash of verificationData using binary mode to make it working, as advised by @ovx below

If it can help someone, here is the PHP function I wrote to verify the ALTCHA response:

/**
 * Verify ALTCHA
 *
 * @param string $secret ALTCHA secret
 * @param string $method Form submission method
 *
 * @return bool|array
 * - in case of success
 *   - without using antispam: true
 *   - using antispam: array with 'classification' and 'score'
 * - else: false
 * @throws Exception
 */
public function verifyAltcha(string $secret, string $method = 'post')
{
    // Get form data, based on form submission method
    $method = strtolower($method);
    if ($method === 'post') {
        $data = $_POST;
    } elseif ($method === 'get') {
        $data = $_GET;
    } else {
        throw new \Exception("'$method' method isn't supported (please use 'post' or 'get')");
    }

    // Get the ALTCHA response
    if (empty($data['altcha'])) {
        // ALTCHA response is missing
        return false;
    }
    $altcha_response = json_decode(base64_decode($data['altcha']));

    // Check the format of the ALTCHA response
    if (!is_object($altcha_response)) {
        // Invalid ALTCHA response
        return false;
    }
    foreach(['algorithm', 'signature'] as $prop) {
        if (!property_exists($altcha_response, $prop)) {
            // Invalid ALTCHA response
            return false;
        }
    }

    // Check algorithm
    $algorithm = strtolower(str_replace('-', '', $altcha_response->algorithm));
    if (!in_array($algorithm, ['sha256', 'sha384', 'sha512'])) {
        // Unsupported algorithm
        return false;
    }

    // Verify the ALTCHA response
    if (property_exists($altcha_response, 'verified')
        && property_exists($altcha_response, 'verificationData')) {

        /** Using the spam filter */

        // Check signature
        $payload_hash = hash($algorithm, $altcha_response->verificationData, true);
        $signature = hash_hmac($algorithm, $payload_hash, $secret);
        if ($altcha_response->signature !== $signature) {
            // Invalid signature
            return false;
        }

        // Check 'verified' flag
        if (!$altcha_response->verified) {
            // ALTCHA isn't verified
            return false;
        }

        // Extracts verification data
        parse_str($altcha_response->verificationData, $verification_data);

        // Check fields hash
        if (!empty($verification_data['fields'])) {
            $fields = explode(',', $verification_data['fields']);
            $values = [];
            foreach($fields as $field) {
                if (!array_key_exists($field, $data)) {
                    // Field value is missing from data
                    return false;
                }
                $values[] = $data[$field];
            }
            $fields_hash = hash($algorithm, implode("\n", $values));

            if ($fields_hash !== $verification_data['fieldsHash']) {
                // Fields hash does not match
                return false;
            }
        }

        // Check time
        $current_time = time();
        if ($current_time < $verification_data['time'] || $current_time > $verification_data['expire']) {
            // Invalid time
            return false;
        }

        // Avoid reuse
        if (!isset($_SESSION['altcha_signatures']) || !is_array($_SESSION['altcha_signatures'])) {
            $_SESSION['altcha_signatures'] = [];
        }
        if (in_array($altcha_response->signature, $_SESSION['altcha_signatures'])) {
            // Already submitted
            return false;
        }
        $_SESSION['altcha_signatures'][] = $altcha_response->signature;

        return [
            'classification' => $verification_data['classification'],
            'score' => $verification_data['score'],
        ];
    } else if (property_exists($altcha_response, 'salt')
        && property_exists($altcha_response, 'number')
        && property_exists($altcha_response, 'challenge')) {

        /** Without using the spam filter */

        // Check challenge
        $challenge = hash($algorithm, $altcha_response->salt . $altcha_response->number);
        if ($altcha_response->challenge !== $challenge) {
            // Invalid challenge
            return false;
        }

        // Check signature
        $signature = hash_hmac($algorithm, $challenge, $secret);
        if ($altcha_response->signature !== $signature) {
            // Invalid signature
            return false;
        }

        return true;
    } else {
        // Invalid ALTCHA response
        return false;
    }
}
ovx commented 1 month ago

Hi, thanks for reporting. The problem isn't in the library itself, but it's with the PHP's hash function, which by default returns a hex-encoded string. You have set the third argument of the hash function to true to return binary data (docs).

It works just fine in the WordPress plugin, for reference, see the implementation https://github.com/altcha-org/wordpress-plugin/blob/main/includes/core.php#L271

The hmacHex functions works as expected with strings and buffers:

it('should return the same HMAC-512 for string and Uint8Array', async () => {
  const fromStr = await hmacHex('SHA-512', 'hello world', 'test');
  const fromAb = await hmacHex('SHA-512', new TextEncoder().encode('hello world'), 'test');
  expect(fromStr).toEqual(fromAb);
  expect(fromStr).toEqual(
    '2536d175df94a4638110701d8a0e2cbe56e35f2dcfd167819148cd0f2c8780cb3d3df52b4aea8f929004dd07235ae802f4b5d160a2b8b82e8c2f066289de85a3'
  );
});
MrTimber commented 1 month ago

Hi @ovx Thanks for your advice regarding PHP validation, it works.

But your test doesn't reproduce the TypeScript issue I'm talking about, because new TextEncoder().encode('hello world') isn't an ArrayBuffer, but an Uint8Array.

We can create an Uint8Array from an ArrayBuffer, but it isn't the same: image

So with new TextEncoder().encode('hello world') as data parameter of the hmacHex/hmac function, then when it calls the crypto.subtle.sign function, the passed data parameter is new Uint8Array(new TextEncoder().encode('hello world')), creating an Uint8Array from an Uint8Array (it's obviously the same thing).

The issue I'm talking about is when you use a string or an ArrayBuffer as the data parameter of the hmacHex/hmac function:

it('should return the same HMAC-256 from HASH-256 ArrayBuffer and from HASH-256 string', async () => {
  const data = 'hello world';
  const hmacKey = 'test key';

  const hash256_ab = await hash('SHA-256', data);
  console.log('hash256_ab:', hash256_ab);

  const hash256_str = ab2hex(hash256_ab);
  console.log('hash256_str:', hash256_str);

  const hmac_hex_from_ab = await hmacHex('SHA-256', hash256_ab, hmacKey);
  console.log('hmac_hex_from_ab:', hmac_hex_from_ab);

  const hmac_hex_from_str = await hmacHex('SHA-256', hash256_str, hmacKey);
  console.log('hmac_hex_from_str:', hmac_hex_from_str);

  expect(hmac_hex_from_ab).toEqual(hmac_hex_from_str);
});

Here, when it calls the crypto.subtle.sign function, the passed data parameter is:

And the result isn't the same: image

It seems abnormal to me, but if it's normal can you explain why please?

ovx commented 1 month ago

Hi, the ArrayBuffer and Uint8Array work with the same underlying data, doing this new TextEncoder().encode('hello world').buffer returns an ArrayBuffer which can be passed to the function and it works fine.

The main problem in your example is the conversion from binary data to hex-encoded string (ab2hex(hash256_ab)). The function does not accept hex-encoded strings, only raw binary data UTF8 strings.

The string type in the function is really mostly for tests, you're not expected to ever provide a string to the function when verifying the signatures as they will be always buffers. It should be also noted, that due to the use of the TextEncoder, the string provided will be encoded using UTF8 making this conversion fail for binary strings. If you really need to provide a string, you'll have to use different conversion, just like here on StackOverflow. As illustrated in the StackOverflow answer, converting binary data to strings and vice versa in JS in not trivial and prone to errors. Always use ArrayByffers/TypedArrays to be safe, that's how this library is supposed to be used.

MrTimber commented 1 month ago

Thanks for the explanation and for your work. Have a good evening.