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

Running python with optimizations makes UsernamePasswordMako accept any password for any user #451

Open
obi1kenobi opened this Issue Sep 9, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@obi1kenobi

obi1kenobi commented Sep 9, 2017

On the current master branch, the UsernamePasswordMako class relies on an assert statement to check the user's password:
https://github.com/rohe/pysaml2/blob/9cbbd9bd9f6bfa5e9ceace064dd1af4e2ff2f68c/src/saml2/authn.py#L149

The assert is supposed to raise an exception if the password doesn't match. This is insecure: running python with optimizations enabled (either via the -O or -OO flags, or with the PYTHONOPTIMIZE environment variable) will remove all such assertions. This means that no exception will be raised on an incorrect password, and the UsernamePasswordMako will accept any password for any user.

It would be better to have an explicit check that raises an exception to avoid this problem.

rohe added a commit that referenced this issue Oct 11, 2017

Merge pull request #454 from jkakavas/fix_authn
Quick fix for the authentication bypass due to optimizations #451
@gmollett

This comment has been minimized.

Show comment
Hide comment
@gmollett

gmollett Nov 23, 2017

I think this fix introduces another issue. Classes that use UsernamePasswordMako as a base (such as LDAPAuthn) and provide their own _verify method that signals failure to the parent class with raise AssertionError() will no longer be caught.

gmollett commented Nov 23, 2017

I think this fix introduces another issue. Classes that use UsernamePasswordMako as a base (such as LDAPAuthn) and provide their own _verify method that signals failure to the parent class with raise AssertionError() will no longer be caught.

@c00kiemon5ter

This comment has been minimized.

Show comment
Hide comment
@c00kiemon5ter

c00kiemon5ter Nov 23, 2017

Member

Although, this seems to be only affecting LDAPAuthn this could affect other modules developed by other people. I suggest we change the exception to catch AssertionError too, for backwards compatibility.

         return resp
  
     def _verify(self, pwd, user):
- except (ValueError, KeyError):
+ except (ValueError, KeyError, AssertionError):

      def verify(self, request, **kwargs):
          """
Member

c00kiemon5ter commented Nov 23, 2017

Although, this seems to be only affecting LDAPAuthn this could affect other modules developed by other people. I suggest we change the exception to catch AssertionError too, for backwards compatibility.

         return resp
  
     def _verify(self, pwd, user):
- except (ValueError, KeyError):
+ except (ValueError, KeyError, AssertionError):

      def verify(self, request, **kwargs):
          """
@obi1kenobi

This comment has been minimized.

Show comment
Hide comment
@obi1kenobi

obi1kenobi Nov 25, 2017

In my opinion, it would be better to leave this as a breaking change and signal that through semver. Raising AssertionError to indicate authentication failure seems unintuitive at best, and we're better off without it IMHO.

obi1kenobi commented Nov 25, 2017

In my opinion, it would be better to leave this as a breaking change and signal that through semver. Raising AssertionError to indicate authentication failure seems unintuitive at best, and we're better off without it IMHO.

@gmollett

This comment has been minimized.

Show comment
Hide comment
@gmollett

gmollett Dec 5, 2017

The change is already in the 4.5.0 minor release (going by the git tags, apologies if this is not correct) from what I can see, which being a minor release shouldn't break break backwards compat?

I Agree re: semver and not using AssertionError to indicate auth fail though.

Maybe keep AssertionError in for now but give a deprecation warning and then remove it in 5.0.0?

gmollett commented Dec 5, 2017

The change is already in the 4.5.0 minor release (going by the git tags, apologies if this is not correct) from what I can see, which being a minor release shouldn't break break backwards compat?

I Agree re: semver and not using AssertionError to indicate auth fail though.

Maybe keep AssertionError in for now but give a deprecation warning and then remove it in 5.0.0?

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil Jan 5, 2018

This issue was assigned CVE-2017-1000433

carnil commented Jan 5, 2018

This issue was assigned CVE-2017-1000433

@rohe

This comment has been minimized.

Show comment
Hide comment
@rohe

rohe Jan 15, 2018

Collaborator

I'll go through the code and replace all assert statements with if statements.

Collaborator

rohe commented Jan 15, 2018

I'll go through the code and replace all assert statements with if statements.

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