validateRequest() does not convert integers to zero padded strings before calling validate()

Created on 3 January 2024, over 1 year ago
Updated 24 January 2024, about 1 year ago

Problem/Motivation

Since πŸ“Œ Public Followup for 8.x-1.3 security release. Postponed we have seen random test failures in TfaTotpTest::testTotpCodeValidation.

We had an initial set of failures in that issue, however after making a change I tested with either 60,000 or 600,000 runs (command has been wiped from my history to validate count) running over multiple time windows.

Upon deeper testing it appears to likely be caused by the fact that integers converted to strings are not zero padded. If a code started with zero it would be lost when the code passed to validateRequest is converted to a string in validate().

Steps to reproduce

$plugin->validateRequest((int) '01234');

Proposed resolution

Inside validateRequest() convert the submitted integer to a left padded string.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

πŸ› Bug report
Status

Fixed

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States cmlara

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @cmlara
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Sample Log:

    Time: 00:55.922, Memory: 10.00 MB
    There was 1 failure:
    1) Drupal\Tests\tfa\Unit\Plugin\TfaValidation\TfaTotpTest::testTotpCodeValidation
    Code from previous window accepted
    Failed asserting that false is true.
    

    https://git.drupalcode.org/project/tfa/-/jobs/567976

    $ /var/www/html/vendor/phpunit/phpunit/phpunit --repeat=60000 --bootstrap /var/www/html/web/core/tests/bootstrap.php --configuration /var/www/html/phpunit.xml /var/www/html/web/modules/custom/tfa/tests/src/Unit/Plugin/TfaValidation/TfaTotpTest.php
    HTML output directory /var/www/html/simpletest/browser_output is not a writable directory.
    PHPUnit 9.6.15 by Sebastian Bergmann and contributors.
    
    Warning:       Your XML configuration validates against a deprecated schema.
    Suggestion:    Migrate your XML configuration using "--migrate-configuration"!
    
    Testing 
    ...........................................................    59 / 60000 (  0%)
    ...
    ........................................................    60000 / 60000 (100%)
    
    Time: 02:38.191, Memory: 124.01 MB
    
    OK (60000 tests, 660000 assertions)
    
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Another failure.
    https://git.drupalcode.org/project/tfa/-/jobs/583531

    There was 1 failure:
    1) Drupal\Tests\tfa\Unit\Plugin\TfaValidation\TfaTotpTest::testTotpCodeValidation
    Valid token accepted
    Failed asserting that false is true.

    I'm starting to suspect this perhaps is some weirdness around float to int conversion in the tests, or its a fault in string->int->string conversion.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Was able to capture this

    3300) Drupal\Tests\tfa\Unit\Plugin\TfaValidation\TfaTotpTest::testTotpCodeValidation
    Code from next window accepted 014656
    Failed asserting that false is true.

    In 2.x we have already converted this method to only accept a string instead of integer so the flaw will not exist in future versions.
    We can't really change the API in 1.x since its committed.

    I'm thinking inside of validateRequest() for the TOTP and HOTP plugin left padding the submitted int with zeros may be our solution.

    Re-titling issue now that a root cause has been found

  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara
  • Status changed to Fixed about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara
    • cmlara β†’ committed 4b9dc842 on 8.x-1.x
      Issue #3412200 by cmlara: validateRequest() does not convert integers to...
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024