Major issue - Githubissues

yedderson commented 11 years ago

I think there is a design issue here. the randomly generated user/password hash cannot be verified against the hash already in the store, it will never match. the verification process is very wrong, if only the username is valid (eg. admin) then ANY password will grant you access.

FedericoCeratto commented 11 years ago

Hello, can you please describe how to reproduce the issue exactly? When testing multiple logins with different passwords make sure that a logout has been done and the cookie has been removed.

yedderson commented 11 years ago

Yes, here is a sample application, when you login, the password is simply ignored.

from bottle import request, route, template, default_app, run
from beaker.middleware import SessionMiddleware
from cork import Cork

app = default_app()
aaa = Cork('example_conf')
#    I used this code in the 'recreate_example_conf.py' to generated the hashes
#    ...
#    tstamp = str(datetime.utcnow())
#        username = 'root'
#        password = 'pwd'
#        cork._store.users[username] = {
#            'role': 'admin',
#            'hash': cork._hash(username, password),
#            'email_addr': username + '@localhost.local',
#            'desc': username + ' test user',
#            'creation_date': tstamp
#        }
#    ...
app = SessionMiddleware(app, {
                        'beaker.session.auto': True,
                        'beaker.session.type': 'cookie',
                        'beaker.session.validate_key': True,
                        'beaker.session.cookie_expires': True,
                        'beaker.session.timeout': 3600 * 24})

@route('/login', method=['GET', 'POST'])
def login():
    if request.method == 'GET':
        return template("""
        <html>
        <form method="post" action="/login">
            <input name="user" required="required" type="text" placeholder="user">
            <input name="password" type="password" required="required" placeholder="password">
            <input type="submit" name="login" value="login">
        </form>
        </html>
        """)
    if request.method == 'POST':
        username = request.POST.get('user','')
        password = request.POST.get('password','')
        aaa.login(username=username, password=password, success_redirect='/', fail_redirect='/login')

@route('/')
def home():
    aaa.require(role='admin',  fail_redirect='/login')
    return 'welcome'

run(app=app,  host='0.0.0.0', port=80)

FedericoCeratto commented 11 years ago

I'm not having any unexpected behavior from the code you pasted. Incorrect passwords are making the login fail. However, you have no code to remove the cookie in that snippet. Try inspecting the cookie on your browser before and during the login.

yedderson commented 11 years ago

I'm cleaning the cookies manually, I tested the code above on Opera, Chrome and Firefox and using root/pwd, root/abc, root/anything they all get me in.

I'm using the latest releases for beaker, cork and bottle, are you with this same config ?

Beaker==1.6.4
bottle==0.11.2
bottle-cork==0.2

and here is the details for the cookies I'm getting:

name                value                                               domain      size    path    expire
beaker.session.id   78ef4b6864da48d3c62c5ca...3RpbWVxBUdB1CNZUYyLRHUu   127.0.0.1   197 B   /       Session

what can I do to help debug this code?

FedericoCeratto commented 11 years ago

The library versions are very close. Can you join the #bottlepy IRC channel on Freenode? In the meantime I'll keep investigating.

Beaker 1.6.3 Bottle 0.11.3 Cork 0.2

Thanks!

FedericoCeratto commented 11 years ago

Also, did you tried running the unit and functional tests? Are they all successful?

23min commented 11 years ago

Which version of pycrypto?

yedderson commented 11 years ago

bottle-cork uses cypto from beaker.

FedericoCeratto commented 11 years ago

I'm using pycrypto v. 2.6

FedericoCeratto commented 11 years ago

I've updated the unit and functional tests to run them on windows as well.

yedderson commented 11 years ago

Beaker depends on one of Pycryptopp, NSScrypto, JCEcrypto or Crypto and he's silently failing when no one is installed. this make the whole password verification process for cork useless.

I've successfully tested with Crypto and cryptopp and because there is no dependency check when pip install beaker the solution is to enforce dependency on Crypto for cork and add some safety checks.

On windows there is no pip install for any of them, you can get Crypto binaries from here or pycryptopp eggs from here

thanks goes to FedericoCeratto and defnull.

23min commented 11 years ago

I had added pycrypto dependency in my heroku-fix pull request. Really the solution is probably for beaker to either not silently fail, or to have a hard default dependency. But for cork to be useful, it may be good to just add the default pycrypto as a dependency.

yedderson commented 11 years ago

@23min I've just looked into your pull request, yes you're right.

FedericoCeratto commented 11 years ago

I've added a safety check in this commit: https://github.com/FedericoCeratto/bottle-cork/commit/6f0755ea2ccf53f13858c34f9a320ba75d6d3bb3

FedericoCeratto / bottle-cork

Major issue #17