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

Fixes #19612 - CVE-2017-7505 don't expose admin to taxed users #4545

Merged
merged 1 commit into from Jun 1, 2017

Conversation

ares
Copy link
Member

@ares ares commented May 22, 2017

No description provided.

@tbrisker
Copy link
Member

@ares Didn't dive much into it but looks like failing tests are related.

@ares
Copy link
Member Author

ares commented May 23, 2017

Sadly, we relied on this behavior in some tests and code (notification recipients, authentication when login is disabled). All should be fixed now.

@ares ares changed the title Fixes #19612 - don't expose admin to taxed users Fixes #19612 - CVE-2017-7505 don't expose admin to taxed users May 23, 2017
subscribers = subscriber_ids
notification_recipients.build subscribers.map{|id| { :user_id => id}}
subscribers = User.unscoped.where(:id => subscriber_ids)
notification_recipients.build subscribers.map{|user| { :user => user}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reviewer (and myself): If we used :user_id, rails later tried to load the association when it validates presence of user but it respects the default scope defined by taxonomix, therefore it couldn't find the user and silently failed to save the built associated notification recipient. Searching for users like this should be more efficient, since it loads all of them in one query.

@tbrisker
Copy link
Member

[test foreman]
[test katello]
[test upgrade]
for some reason tests didn't fire...

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's see that tests pass now.

@tbrisker
Copy link
Member

I think https://github.com/Katello/katello/blob/master/test/katello_test_helper.rb#L91 needs to also be fixed for katello tests to run

@ares ares added the 1.15.1 label May 24, 2017
@dLobatog
Copy link
Member

dLobatog commented May 24, 2017

I was able to reproduce and test by:

  1. Having a couple of admin users NOT in any organization, and a test user (not admin) with a role which has the "edit_users" permission.
  2. The test user is in the organization "forklift-devel".
  3. Without the patch, the test user can see and change properties of the admin users.
  4. With the patch, the test user cannot do what I could on step 3.

If I remove the test user from all organizations, it still works fine and the test user cannot see the admin users.
If I add an admin user to the test user organization 'forklift-devel' - the admin user is now visible.

@ares 👍 from me, if you can rebase & make it green I'm happy to merge for inclusion on 1.15.1

@dLobatog
Copy link
Member

@iNecas Do katello tests run without minitest/minitest#696 ?

@iNecas
Copy link
Member

iNecas commented May 25, 2017

I don't see the relation of katello test failures to minitest/minitest#696

/usr/local/rvm/gems/ruby-2.2.5@test_develop_pr_katello-1/gems/activerecord-4.2.8/lib/active_record/relation/finder_methods.rb:324:in `raise_record_not_found_exception!': Couldn't find User with 'id'=135138680 [WHERE (1=0)] (ActiveRecord::RecordNotFound)

@ares
Copy link
Member Author

ares commented May 25, 2017

Thanks @dLobatog, katello tests should be fixed by Katello/katello#6800 I triggered tests of both at http://ci.theforeman.org/job/test_katello_pull_request/11873/

@tbrisker
Copy link
Member

@ares looks like something didn't work on the manual test run, care to take a look? I don't want to merge this only to discover we broke katello somewhere. Other plugins may also need fixes.

@ares
Copy link
Member Author

ares commented May 25, 2017

yeah, the jenkins params are not well documented, so I triggered it with wrong values, let's see if this will be better

@dLobatog
Copy link
Member

@ares I don't think that job got to run 😞 let's wait for Katello/katello#6800 to be merged, it's a tiny PR.

@ares
Copy link
Member Author

ares commented May 30, 2017

Katello PR was merged but this theforeman/foreman-tasks#252 should go in too before this is merged. Otherwise one katello task test will be broken. @iNecas confirmed they will merge and release new version soon (I expect it tomorrow)

@ares
Copy link
Member Author

ares commented May 31, 2017

Both required PRs are merged, foreman-tasks are released. I think everything would be green now but Foreman tests seems to be broken.

@dLobatog
Copy link
Member

[test katello]

@ares
Copy link
Member Author

ares commented Jun 1, 2017

I went through all Katello failures, all are instances of NoMethodError: undefined method split' for nil:NilClass` which other plugins hit recently, it was caused by using assert_equal while on of the value was nil. I don't think it's related, other PRs fail with the same.

@dLobatog
Copy link
Member

dLobatog commented Jun 1, 2017

@ares No worries, I'm keeping track of these. Foreman tests seem fine. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants