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

Pillar matching on mine.get can be used to discover pillar info #7197

Closed
Mrten opened this issue Sep 12, 2013 · 18 comments · Fixed by #7770
Closed

Pillar matching on mine.get can be used to discover pillar info #7197

Mrten opened this issue Sep 12, 2013 · 18 comments · Fixed by #7770
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around

Comments

@Mrten
Copy link
Contributor

Mrten commented Sep 12, 2013

EDITED by @basepi: Repurposing this bug report for the potential pillar security implications of being able to match on pillar, specifically with globbing.

I'm thinking these two calls should've reported the same number (well not really because the mine.get call has two extra lines of output but 34 is too different)

root@salt-master:/home/salt# salt -I location:1 test.ping | wc -l
     66
root@salt-master:/home/salt# salt monitoring mine.get 'location:1' 'test.ping' expr_form='pillar' | wc -l
    100

The latter returns all the minions, where it should only return the minions where the location pillar is 1, right?

This works for grains, and the manual says it should work for pillars as well.

Targets can be matched based on any standard matching system that can be matched on the master via these keywords:

glob
pcre
grain
grain_pcre
pillar

Mine config:

mine_functions:
  network.interfaces: []
  test.ping: []

#auto-update twee keer per dag
mine_interval: 720
@basepi
Copy link
Contributor

basepi commented Sep 12, 2013

I think it should work for pillar. Thanks for the report, we'll look into it.

@terminalmage
Copy link
Contributor

@Mrten I'm not able to replicate this against git, can you test against an up-to-date checkout?

@Mrten
Copy link
Contributor Author

Mrten commented Sep 13, 2013

Not easily, no. I'm on the ubuntu PPA, I tried the salt-daily PPA but the version number there is 0.16.0 and apt-get won't let me downgrade.

@Mrten
Copy link
Contributor Author

Mrten commented Sep 13, 2013

OK, this helped:

apt-get install salt-master=0.16.0-8965~ubuntu12.04.1 salt-common=0.16.0-8965~ubuntu12.04.1 salt-minion=0.16.0-8965~ubuntu12.04.1

Then I do indeed get the right results.

@Mrten
Copy link
Contributor Author

Mrten commented Sep 13, 2013

Erm, isn't it a security problem? Because this works as well:

root@monitoring:/home/mrten# salt-call mine.get 'location:2' 'test.ping' expr_form='pillar'

So you can infer pillar data from minions this way. Pillars are supposed to be secret, right? This leaks pillar-data to other minions.

@terminalmage
Copy link
Contributor

Yeah, you can't exactly ask for the data and get it back, but you can keep asking and start to get a picture of other hosts' pillar data.

We'll discuss this internally, and try to come up with a solution.

@basepi
Copy link
Contributor

basepi commented Sep 16, 2013

Generally, pillar things that need to remain secret are password hashes, keys, and the like. You're never going to be able to guess those values to infer pillar on other minions.

That said, it would probably be fairly simple to make it configurable whether mine allowed pillar targeting or not. Easy fix, should please everyone. =)

@basepi
Copy link
Contributor

basepi commented Sep 16, 2013

Did we open a new issue for the pillar targeting? Looks like this issue can be closed, but we should open a new issue for the pillar targeting before we forget.

@terminalmage
Copy link
Contributor

I do not think we have opened a new issue, no.

@Mrten
Copy link
Contributor Author

Mrten commented Sep 16, 2013

Can you do regexp or glob pillar matching? Because if you can, you can iterate over all characters à la BEAST (or was it CRIME) and recover ssl keys.

(for the uninitiated: try a_, b_, c_..., z_, then if d is a hit: da_, db_, dc_, dd_, etc)

@basepi
Copy link
Contributor

basepi commented Sep 16, 2013

Good point, I don't know if it supports globs or not. I want to say "no" but I'm not sure. Could you test that? I don't have the mine set up on my test machine.

@Mrten
Copy link
Contributor Author

Mrten commented Sep 16, 2013

They do, sir, they do.

root@monitoring:/# salt-call mine.get 'mysql_exim_password:a*' 'test.ping' 'pillar'
local:
    ----------
    mx-1:
        True
[more hosts]
root@monitoring:/# salt-call mine.get 'mysql_exim_password:ab*' 'test.ping' 'pillar'
local:
    ----------
root@monitoring:/# salt-call mine.get 'mysql_exim_password:ac*' 'test.ping' 'pillar'
local:
    ----------
    mx-1:
        True
[more hosts]

I don't know the syntax for deeper nested pillar matching, so I did not get that to work.

@basepi
Copy link
Contributor

basepi commented Sep 16, 2013

Yep, in this case we need to definitely remove globbing support for the mine, and then additionally make pillar matching configurable in the first place.

Thanks for your good detective work here.

@basepi
Copy link
Contributor

basepi commented Sep 16, 2013

I'm going to repurpose this bug for the pillar matching problems, rather than create a new one. Renaming and re-labeling.

@Mrten
Copy link
Contributor Author

Mrten commented Oct 9, 2013

So, since 0.17 is out, is this now CVE-worthy? This kinda prevents me from putting secret data in the pillar.

@thatch45
Copy link
Contributor

thatch45 commented Oct 9, 2013

This was fixed right? I thought @basepi got it....

@basepi
Copy link
Contributor

basepi commented Oct 9, 2013

Nope, it's one of my many open tabs reminding me "hey, this needs to be fixed!"

@basepi
Copy link
Contributor

basepi commented Oct 11, 2013

Sorry this one took so long for such a simple solution. Fix will be in 0.17.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants