Project

General

Profile

Actions

Bug #10005

closed

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

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

Status:
Closed
Priority:
Normal
Category:
Discovery plugin
Difficulty:
Triaged:
Fixed in Releases:
Found in Releases:

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 2 (0 open2 closed)

Related to Foreman - Refactor #10025: Move taxonomy related methods and scopes to Host::BaseClosedMarek Hulán04/06/2015Actions
Related to Foreman - Bug #9947: CVE-2015-1844 - GET /api/hosts doesn't respect organization/location membershipClosedMarek Hulán03/30/2015Actions
Actions #2

Updated by Marek Hulán almost 9 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...

Actions #3

Updated by Dominic Cleal almost 9 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.

Actions #4

Updated by Lukas Zapletal almost 9 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.

Actions #5

Updated by Marek Hulán almost 9 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.

Actions #6

Updated by Marek Hulán almost 9 years ago

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

Updated by Dominic Cleal almost 9 years ago

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

Updated by Dominic Cleal almost 9 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)?

Actions #10

Updated by Dominic Cleal almost 9 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
Actions #11

Updated by Lukas Zapletal almost 9 years ago

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

Actions #12

Updated by The Foreman Bot almost 9 years ago

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

Updated by Anonymous almost 9 years ago

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

Updated by Dominic Cleal almost 9 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?

Actions #15

Updated by Lukas Zapletal almost 9 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.

Actions #16

Updated by Dominic Cleal almost 9 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?

Actions #17

Updated by Lukas Zapletal almost 9 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.

Actions #18

Updated by Lukas Zapletal almost 9 years ago

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

Actions

Also available in: Atom PDF