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

[obi1kenobi]

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.

[gmollett]

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]

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]

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]

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 issue was assigned CVE-2017-1000433

[rohe]

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

[peppelinux]

@obi1kenobi is there any update on this?
@rohe was then something done?

If any reply, this issue will be closed in weeks, thank you

[obi1kenobi]

Nothing from my end — #716 looks good to me so far.

It might be worth integrating a tool like bandit ( https://pypi.org/project/bandit/ ) into the CI flow, since it is able to automatically detect and warn about a variety of security-related problems. I believe it would have generated warnings for the assert statements here, for example. The GraphQL compiler project I maintain relies on bandit as well as a few other tools like it, and you can see an example of it here: https://github.com/kensho-technologies/graphql-compiler/blob/main/scripts/lint.sh#L107