New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

This plugin may be vulnerable to timing attacks. #3

Merged
merged 3 commits into from Nov 23, 2011

Conversation

Projects
None yet
2 participants
@arturosevilla
Contributor

arturosevilla commented Nov 13, 2011

For example, if I'm using bcrypt as my hash generator, an attacker may notice that he may discover usernames when the login fails more slowly than for non-existing usernames. This is due to the following part:

if user:
    validator = getattr(user, self.translations['validate_password'])
    if validator(identity['password']):
        return identity['login']

In file repoze/who/plugins/sa.py line 167

If the user does not exist then validator never gets executed.

In the attached commit I've included my proposed solution (with unit tests). Notice that because I've introduced a new "translation" called dummy_validate. While I would have preferred that "validate_password" should have been changed to be a function instead of a method (so that we can call it when user is None), this would a break a lot of existing software.

@arturosevilla

This comment has been minimized.

Show comment
Hide comment
@arturosevilla

arturosevilla Nov 13, 2011

Contributor

I also modified the documentation to reflect the proposed solution.

Contributor

arturosevilla commented Nov 13, 2011

I also modified the documentation to reflect the proposed solution.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Nov 13, 2011

I'd call it "dummy_validate_password" to be consistent with the actual validator.

I'd call it "dummy_validate_password" to be consistent with the actual validator.

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Nov 13, 2011

Member

Hello, Arturo.

Thank you very much for the patch. I only have one suggestion, as you'll see in the commit.

I need to run the test suite with SQLAlchemy and Elixir before I can release it to PYPI, but unfortunately my laptop is being repaired at the moment and the one I'm currently using isn't set up or suitable for development. I should be able to release it on Tuesday or Wednesday.

I assume it's not critical to release it ASAP given that this can only work under a brute force attack, right? These attacks should be prevented at a higher level (e.g., the OS) anyway.

Cheers,

  • Gustavo.
Member

gnarea commented Nov 13, 2011

Hello, Arturo.

Thank you very much for the patch. I only have one suggestion, as you'll see in the commit.

I need to run the test suite with SQLAlchemy and Elixir before I can release it to PYPI, but unfortunately my laptop is being repaired at the moment and the one I'm currently using isn't set up or suitable for development. I should be able to release it on Tuesday or Wednesday.

I assume it's not critical to release it ASAP given that this can only work under a brute force attack, right? These attacks should be prevented at a higher level (e.g., the OS) anyway.

Cheers,

  • Gustavo.
@arturosevilla

This comment has been minimized.

Show comment
Hide comment
@arturosevilla

arturosevilla Nov 14, 2011

Contributor

That's right, they should be prevented at another level or by generating a CAPTCHA challenge if many requests are done. However, I think that repoze.who shouldn't be (or generate) a weak point in case it does not occur. After all, this would be optional.
I will change the name of the translation and update this pull request.

Contributor

arturosevilla commented Nov 14, 2011

That's right, they should be prevented at another level or by generating a CAPTCHA challenge if many requests are done. However, I think that repoze.who shouldn't be (or generate) a weak point in case it does not occur. After all, this would be optional.
I will change the name of the translation and update this pull request.

Rename dummy_validate translation to dummy_validate_password.
This refactoring also includes changes in the unit tests. The documentation
was modified to make it more clear what does dummy_validate_password receive.
@arturosevilla

This comment has been minimized.

Show comment
Hide comment
@arturosevilla

arturosevilla Nov 14, 2011

Contributor

I ran the unit tests and all 30 pass, but I have no problem waiting for your approval and eventual upload to PYPI. I will be hanging in #repoze for a while, if you want to discuss any other thing for this pull request.

Contributor

arturosevilla commented Nov 14, 2011

I ran the unit tests and all 30 pass, but I have no problem waiting for your approval and eventual upload to PYPI. I will be hanging in #repoze for a while, if you want to discuss any other thing for this pull request.

Removed a redundant return
This redundant return was making the unit test coverage less than 100%.
@arturosevilla

This comment has been minimized.

Show comment
Hide comment
@arturosevilla

arturosevilla Nov 14, 2011

Contributor

I'm also working in repoze.what-quickstart so that it may use the dummy_validate_password translation. But I will upload that commit after this plugin has been updated in PYPI.

Contributor

arturosevilla commented Nov 14, 2011

I'm also working in repoze.what-quickstart so that it may use the dummy_validate_password translation. But I will upload that commit after this plugin has been updated in PYPI.

gnarea added a commit that referenced this pull request Nov 23, 2011

Merge pull request #3 from arturosevilla/timing-attack-prevention
Added ability to protect from timing attacks

@gnarea gnarea merged commit 6ce3b26 into repoze:master Nov 23, 2011

@gnarea

This comment has been minimized.

Show comment
Hide comment
@gnarea

gnarea Nov 23, 2011

Member

Thanks, Arturo! And sorry for the tardiness.

I'll test the code with different dependencies and then I'll release it to PYPI.

And also thanks for working on the support of this functionality in repoze.what-quickstart.

Cheers.

Member

gnarea commented Nov 23, 2011

Thanks, Arturo! And sorry for the tardiness.

I'll test the code with different dependencies and then I'll release it to PYPI.

And also thanks for working on the support of this functionality in repoze.what-quickstart.

Cheers.

@gnarea

This comment has been minimized.

Show comment
Hide comment
Member

gnarea commented Nov 23, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment