GoogleAuthenticator icon indicating copy to clipboard operation
GoogleAuthenticator copied to clipboard

verifyCode() method could be slightly improved

Open kikosoft opened this issue 3 years ago • 3 comments

I've noticed that you use a timing safe equals comparison method in verifyCode(), yet this method itself is not time safe. That seems to be a bit of a contradiction. It has to do with the return inside the for loop:

        for ($i = -$discrepancy; $i <= $discrepancy; ++$i) {
            $calculatedCode = $this->getCode($secret, $currentTimeSlice + $i);
            if ($this->timingSafeEquals($calculatedCode, $code)) {
                return true;
            }
        }

Depending on the time slice in which a match is found the verifyCode() method will take longer or shorter to execute. To be honest, I don't see how this can be exploited in a timing attack, but still, why not simply get rid of this problem? Something like:

        $success = false;
        for ($i = -$discrepancy; $i <= $discrepancy; ++$i) {
            $calculatedCode = $this->getCode($secret, $currentTimeSlice + $i);
            if ($this->timingSafeEquals($calculatedCode, $code)) {
                $success = true;
            }
        }
        return $success;

Now the time this piece of code takes to execute does no longer depend on when a match is found.

kikosoft avatar Jul 04 '21 11:07 kikosoft

Is there a reason you didn't open a pull request for this? Just wondering.

EAWF avatar Jul 05 '21 05:07 EAWF

I have no idea how that works. I tried but can't get very far with that. All Github allows me is to read the documentation, nothing more.

kikosoft avatar Jul 05 '21 08:07 kikosoft

Here's a link to how to propose/request a change to a github repository, such as what you are proposing:

https://www.earthdatascience.org/courses/intro-to-earth-data-science/git-github/github-collaboration/how-to-submit-pull-requests-on-github/

Hope that helps, and looking forward to see your code implemented into the project.

EAWF avatar Jul 15 '21 15:07 EAWF