Project

General

Profile

Bug #10005

CVE-2015-1844 - Discovery hosts are not restricted to user taxonomies

Added by Marek Hulán almost 8 years ago. Updated almost 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Lukas Zapletal
Category:
Discovery plugin
Difficulty:
Triaged:
No
Bugzilla link:
Pull request:
https://github.com/theforeman/foreman_discovery/pull/177
Fixed in Releases:
Found in Releases:
Red Hat JIRA:

Description

I found security issue that's very similar to what is being fixed in Foreman and tracked as http://projects.theforeman.org/issues/9947 The Foreman fix doesn't fix it, because it's present in discovery codebase. See https://github.com/theforeman/foreman_discovery/blob/develop/app/models/host/discovered.rb#L30

This allows user to manipulate all discovered hosts (if they have global permission) even if they are assigned to specific org/loc. The fix is either to apply the same patch as we did in Foreman Host::Managed or (better) move the fix to Host::Base and remove this default scope from discovery.

Related issues

Related to Foreman - Refactor #10025: Move taxonomy related methods and scopes to Host::BaseClosed2015-04-06
Related to Foreman - Bug #9947: CVE-2015-1844 - GET /api/hosts doesn't respect organization/location membershipClosed2015-03-30

Associated revisions

Revision c2e61ec2 (diff)
Added by Lukas Zapletal almost 8 years ago

Fixes #10005 - removed unused default scope and added tax tests

History

#2 Updated by Marek Hulán almost 8 years ago

Yes, core bug is public. I'd wait with public PR until confirmed by others. I don't see a point why to duplicate this default_scope. Host::Base already has notion of taxonomies https://github.com/theforeman/foreman/blob/develop/app/models/host/base.rb#L202. Of course belongs_to $taxonomy should be moved to Base as well...

#3 Updated by Dominic Cleal almost 8 years ago

Small reminder - please don't mark notes as private within a private bug, all will be private automatically. It also means when the bug is made public, discussion will be visible.

#4 Updated by Lukas Zapletal almost 8 years ago

From comment 1: "I am against moving it to Host::Base that could be quite confusing. I will do the same patch in Host::Discovered."

Since Host::Base already contains that, I agree this would be good fit. Can you refactor this please? I will test this with Discovery plugin.

#5 Updated by Marek Hulán almost 8 years ago

Btw even if we move the taxonomy related stuff into Host::Base, you should probably prepare a patch that fixes the default scope in discovery for older versions. The move to Host::Base won't be backported. And after the move you still have to remove default_scope so it does not redefine the one from Base.

#6 Updated by Marek Hulán almost 8 years ago

  • Related to Refactor #10025: Move taxonomy related methods and scopes to Host::Base added

#8 Updated by Dominic Cleal almost 8 years ago

  • Related to Bug #9947: CVE-2015-1844 - GET /api/hosts doesn't respect organization/location membership added

#9 Updated by Dominic Cleal almost 8 years ago

  • Private changed from Yes to No

This is being handled as a continuation of CVE-2015-1844, specifically targeting Foreman Discovery.

#10025 has been merged to develop and 1.8-stable now, and I've marked it for backport to 1.7.5. I have just noticed comment 5 which says it won't be backported - do we need a different fix for Discovery 2.x? Any changes required for 3.x (for 1.8/1.9)?

#10 Updated by Dominic Cleal almost 8 years ago

  • Subject changed from Discovery hosts are not restricted to user taxonomies to CVE-2015-1844 - Discovery hosts are not restricted to user taxonomies

#11 Updated by Lukas Zapletal almost 8 years ago

The code there hasn't changed from 1.5 release, no need of backports there really.

#12 Updated by The Foreman Bot almost 8 years ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman_discovery/pull/177 added
  • Pull request deleted ()

#13 Updated by Anonymous almost 8 years ago

  • Status changed from Ready For Testing to Closed
  • % Done changed from 0 to 100

#14 Updated by Dominic Cleal almost 8 years ago

Lukas Zapletal wrote:

The code there hasn't changed from 1.5 release, no need of backports there really.

If it hasn't changed, then perhaps we do need to backport it to 2.x?

#15 Updated by Lukas Zapletal almost 8 years ago

Dominic Cleal wrote:

Lukas Zapletal wrote:

The code there hasn't changed from 1.5 release, no need of backports there really.

If it hasn't changed, then perhaps we do need to backport it to 2.x?

I have tested discovery 2.0 and it is not affected.

#16 Updated by Dominic Cleal almost 8 years ago

Lukas Zapletal wrote:

Dominic Cleal wrote:

Lukas Zapletal wrote:

The code there hasn't changed from 1.5 release, no need of backports there really.

If it hasn't changed, then perhaps we do need to backport it to 2.x?

I have tested discovery 2.0 and it is not affected.

Ok, was that with the #10025 patch on 1.7-stable or without?

I know 3.x has gained some taxonomy features, but did they introduce an issue here somehow?

#17 Updated by Lukas Zapletal almost 8 years ago

Okay let me explain this with a table. I covered the following:

Foreman version Discovery version Patch applied Vulnerable
1.7 2.0 No Yes
1.7 2.0 Yes Not tested
1.8 3.0 No Yes
1.8 3.0 Yes No

Since the bug #9947 is scheduled for 1.7 and since there were no changes in regard to taxonomy between 2.0 and 3.0 versions, I don't believe it's vulnerable. HAVEN'T TESTED THO. The patch does not apply cleanly.

#18 Updated by Lukas Zapletal almost 8 years ago

I've verified 1.7/2.0 with patch applied, NOT vulnerable. It works, we need the core patch in 1.7.

Also available in: Atom PDF