Skip to content
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

SECURITY: Fix misused _ensure_auth calls #109

Merged
merged 3 commits into from May 6, 2018
Merged

Conversation

bigpresh
Copy link
Owner

@bigpresh bigpresh commented May 5, 2018

Oh, man.

Some places called _ensure_auth() in void context, presumably thinking it would modify the handler coderef given to inject authentication checking - but it doesn't, _ensure_auth() returns a new coderef which does the required authentication checks then calls the original handler coderef which was given to _ensure_auth(). So, calling it in void context then passing the original handler coderef when setting up the route means there's no actual checking done on that route. Oops.

This is a pretty embarassing fuckup - a security problem on one of my projects. I hold my hands up and apologise to anyone affected by this, for this is a stupid mistake. A better test suite would have caught this.

I will see if a CVE ID is warranted for this, and apply for one if so.

Big thanks to @joshrabinowitz for finding and reporting this one, and for adding tests which demonstrated the issue.

joshr and others added 2 commits May 5, 2018 21:09
At some points in the codebase, _ensure_auth() was being used in void context,
presumably under the belief that it would modify the handler coderef given to
it; it doesn't do that, it uses DPAE's require_login / require_role etc methods,
which return a coderef which does the required checks (is logged in, has the
appropriate role, etc) then executes the original coderef if all is OK.

So, calling that in void context does nothing - the coderef with the user/role
checking is just thrown away, and the original, non-authenticated coderef used
as the route handler.

This is pretty embarassing stuff, a security cockup in one of my projects is
awful, and I hold my hands up and apologise.

It also highlights that the test suite wasn't robust enough, or tests would have
been failing.

Thanks, @joshrabinowitz for finding and reporting it, and for adding tests which
demonstrate the problem.

/me sheepishly prepares a release wit this fix, and considers whether requesting
a CVE ID would be warranted.
@bigpresh bigpresh mentioned this pull request May 5, 2018
@bigpresh
Copy link
Owner Author

bigpresh commented May 5, 2018

This is issue #104 - further description on that ticket.

Missed removing this one when replacing it with a proper call.
@joshrabinowitz
Copy link
Contributor

thumbs up - can we merge and make a new release?

@bigpresh
Copy link
Owner Author

bigpresh commented May 6, 2018

Yes! Will try to get it out tonight.

@bigpresh bigpresh merged commit 806dceb into master May 6, 2018
@joshrabinowitz
Copy link
Contributor

That would be very nice. Thanks @bigpresh

joshrabinowitz pushed a commit to joshrabinowitz/Dancer-Plugin-SimpleCRUD that referenced this pull request May 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants