Skip to content

/ salt

Permalink
Browse files

Change key generation seq

  • Loading branch information
@thatch45 @basepi
thatch45 authored and basepi committed May 8, 2013
1 parent 43d8c16 commit 5dd304276ba5745ec21fc1e6686a0b28da29e6fc
Unified Split
Showing with 1 addition and 1 deletion.
  1. +1 −1 salt/crypt.py
2 salt/crypt.py
View file
@@ -47,7 +47,7 @@ def gen_keys(keydir, keyname, keysize, user=None):
priv = '{0}.pem'.format(base)
pub = '{0}.pub'.format(base)

gen = RSA.gen_key(keysize, 1, callback=lambda x, y, z: None)
gen = RSA.gen_key(keysize, 65537, callback=lambda x, y, z: None)
cumask = os.umask(191)
gen.save_key(priv, None)
os.umask(cumask)

53 comments on commit 5dd3042

@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jun 28, 2013

Edit: This user does not represent the Salt project or Salt Stack

Just to clarify, this is about the public exponent, not keysize.

It's of course questionable whether 1 (not a prime) was a good choice for the exponent to begin with, but it's hardly necessary to lose faith over this.

As for the padding, that is properly done on encryption.

So, all in all: good this was changed, but the world wasn't doomed before either.
@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jun 28, 2013

Hehe, I was about to whack the Wikipedia article at you, but this isn't the issue.

Your explanation is sound, and I will just have to take your word, because quite frankly, I am not the professional cryptographer you want. I know crypto, but on a far higher level than mathematics. ;)

The commit should have fixed this once and for all, right?

Given that there were no reported compromises, there is no damage.

I am sure that Salt will at one point undergo an audit, that is if we don't change it all to SSH before (which we might well not). Would you want to volunteer?
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jun 28, 2013

Can I ask you to take a moment in Python or Ruby or something to actually try doing RSA with e=1?

RSA encryption is m^e%p. If e is 1, what is m^e%p?
@b

This comment has been minimized.

Sign in to view
Copy link

@b b replied Jun 28, 2013

Why was 65537 selected? Assuming correct padding and the 2048-bit RSA key size which I believe is your default, a smaller exponent like 3, would seem a better choice.
@withzombies

This comment has been minimized.

Sign in to view
Copy link

@withzombies withzombies replied Jun 28, 2013

You should probably add 1 just to be safe and use 4.

You don't want to get bit by that trailing null.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jun 28, 2013

e=3 is not a great choice; in the presence of other vulnerabilities, there are exploits that are easier to write on very-low-exponent RSA. 2^16+1 is the correct conservative choice.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jun 28, 2013

Also, isn't private_encrypt(x, 5) asking for ANSI X9.31 message format?
@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jun 28, 2013

@tqbf: surely, 65537 is better (and slower) than 3, but I fail to answer your earlier question because I don't know the precedence rules of ^ vs. % off the top of my head. ;)

Thanks for all the input.
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jun 28, 2013

A professional cryptographer did find this fault and evaluated the entire crypto sequence. This audit is what resulted in this change and the number selected was the recommended value to use.
With that said we are open to and continually seeking more analysis of the routines used.

Also, while I did not want to reveal it quite yet, I am working on an optional ssh transport system for Salt allowing Salt to run using just the ssh agent, although, of course, this makes Salt MUCH slower.

In the end, there are no known issues with the crypto in Salt today, and all security issues have been taken care of quickly, and publicly announced.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jun 28, 2013

Can I ask you which professional cryptographer told you it wasn't a big deal to use "1" as your public exponent?
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jun 28, 2013

No, the cryptographer never told us that 1 was ok, he told us that the best choice was 65537 and that we should change it from 1 ASAP, which we did.
@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jun 28, 2013

Oh, btw, I am not affiliated with Salt Inc.. I just use it and try to work towards sending push requests.

(There were people who thought I was an official representative, but it was just that my wife alerted me to the Twitter message.)
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jun 28, 2013

Tom Hatch: can you point me to the public announcement you guys did for the e=1 vulnerability? I'm interested in how you wrote it up. Thanks!
@codahale

This comment has been minimized.

Sign in to view
Copy link

@codahale codahale replied Jun 28, 2013

@tqbf: http://docs.saltstack.com/topics/releases/0.15.1.html#rsa-key-generation-fault
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jun 28, 2013

These represent the security issues found in the audit performed by Ronald Volgers

http://docs.saltstack.com/topics/releases/0.15.1.html

https://groups.google.com/d/msg/salt-users/JMXrTG6H9k0/vrbIeQ3u3lsJ
@tarcieri

This comment has been minimized.

Sign in to view
Copy link

@tarcieri tarcieri replied Jun 28, 2013

I think the larger question here is: why aren't you using TLS?

I will warn you in advance that "because we're using ZeroMQ" is a silly answer. This is at least the third vulnerability that has been found in your homebrew transport encryption, after the lack of a MAC and a timing attack. I hope you now realize that homebrewing your own transport encryption is a bad idea and you should seriously consider switching to TLS at this point to avoid future attacks.
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jun 28, 2013

I agree and am looking into working TLS into Salt, it is also noteworthy that we are actively working on an ssh transport system.

We are always looking for help and would appreciate help with getting TLS working in Salt and with ZeroMQ.
@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jun 29, 2013

Ansible has a "fireball" mode by which they start a 0mq listener through SSH, but that does not actually solve the problem of the security of the 0mq channel.

I am 100% for TLS. SSH is a nice idea too, but probably will be too slow. Good to have as an option for small changes and bootstrapping though.

It's a shame 0mq doesn't do TLS itself.
@mwielgoszewski

This comment has been minimized.

Sign in to view
Copy link

@mwielgoszewski mwielgoszewski replied Jun 30, 2013

@madduck: think of m^e%p as pow(m, e, p). m raised to an exponent of 1 will always be m, when m is less than p.

 >>> pow(int('ABC'.encode('hex'), 16), 1, p)
4276803L
>>> ("%x" % _).decode('hex')
'ABC'
@bigon

This comment has been minimized.

Sign in to view
Copy link

@bigon bigon replied Jun 30, 2013

Shouldn't you ask for a CVE number to track this in downstream distributions?
@peter-clemenko

This comment has been minimized.

Sign in to view
Copy link

@peter-clemenko peter-clemenko replied Jul 1, 2013

I think bigon is right, this is a CVE worthy bug.
@huzaifas

This comment has been minimized.

Sign in to view
Copy link

@huzaifas huzaifas replied Jul 1, 2013

CVE CVE CVE
@techhat

This comment has been minimized.

Sign in to view
Copy link
Member

@techhat techhat replied Jul 1, 2013

I recall hearing that a CVE was filed two months ago for this issue, when it was closed. I'm trying to track it down.
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jul 1, 2013

A CVE was issued, bt since ti was our first CVE it seems that there was a communication problem, thanks @kseifriedredhat for making sure this was filed, I will make sure these are filed in advance in the future
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 1, 2013

e=3 isn't a vulnerability, Kurt.
@techhat

This comment has been minimized.

Sign in to view
Copy link
Member

@techhat techhat replied Jul 1, 2013

@kseifriedredhat, I was not involved in any of this process, but I had heard that one was filed. Thanks for getting us set straight.
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jul 1, 2013

@kseifriedredhat, yes, I submitted this to mitre right away, they allocated a CVE for it, but I must have dropped the ball somewhere since the CVE number that was given was not used on their end. I am conversing with Mitre to make sure that this does not happen again and that future security issues get CVEs allocated correctly.
@thatch45

This comment has been minimized.

Sign in to view
Copy link
Member Author

@thatch45 thatch45 replied Jul 2, 2013

Thanks!
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 2, 2013

No, Kurt, GPUs do not threaten e=3 RSA.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 3, 2013

This paper does not say that GPUs threaten e=3 RSA, Kurt. It's about using GPUs to implement RSA efficiently, not about solving IFP or DLP with a GPU.

Exactly what made you think e=3 was a vulnerability?
@Arabus

This comment has been minimized.

Sign in to view
Copy link
Contributor

@Arabus Arabus replied Jul 5, 2013

Maybe because of this http://www.imc.org/ietf-openpgp/mail-archive/msg06063.html ? e.g. CVE-2006-4340 - AFAIK this was due to an implementation error of RSA and not a vulnerability of RSA itself.
@yanosz

This comment has been minimized.

Sign in to view
Copy link

@yanosz yanosz replied Jul 5, 2013

e=3 is vuln to chinese remaindering: If you have three keys having e=3, modulus N1, N2, N3 and one message is encrypted using each key, then chinese remaindering allows calculating mod N1 x N2 x N3 and using an ordinary (continuous) cubic root.

Please PM me if you have questions on this.
@lucian1900

This comment has been minimized.

Sign in to view
Copy link

@lucian1900 lucian1900 replied Jul 5, 2013

Is there a particular reason why DTLS can't be used on top of 0MQ?
@s0undt3ch

This comment has been minimized.

Sign in to view
Copy link
Member

@s0undt3ch s0undt3ch replied Jul 5, 2013

@lucian1900

This comment has been minimized.

Sign in to view
Copy link

@lucian1900 lucian1900 replied Jul 5, 2013

Yes, I saw that. I don't know much about 0MQ, but it looks to me like a good case for DTLS. Its usage is not as widespread as TLS, but it's still much better than a homegrown encrypted transport protocol.
@listrophy

This comment has been minimized.

Sign in to view
Copy link

@listrophy listrophy replied Jul 5, 2013

What @yanosz explained is basically Coppersmith's Attack. The model of the attack involves low e such that the number of parties participating in the common encryption of a substantially similar plaintext is greater than or equal to e. IIRC, in this case, decryption can be performed without d, p or q in polynomial time... which is a Big Deal.
@s0undt3ch

This comment has been minimized.

Sign in to view
Copy link
Member

@s0undt3ch s0undt3ch replied Jul 5, 2013

@lucian1900

This comment has been minimized.

Sign in to view
Copy link

@lucian1900 lucian1900 replied Jul 5, 2013

Known vulnerabilities having been addressed doesn't mean there aren't many more not yet found. Furthermore, why waste time maintaining an encrypted transport protocol at all (regardless of the wisdom of doing so), when it would (eventually) be less work to use an established protocol.
@mattkeenan

This comment has been minimized.

Sign in to view
Copy link

@mattkeenan mattkeenan replied Jul 5, 2013

@listrophy A corollary to that is if you have a "web scale" crypto system with more than 2^16+1 users and you use encrypted broadcasts your system may be vulnerable if enough of those individually encrypted broadcasts are hoovered up by an attacker (aka NSA).
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 5, 2013

You never encrypt the exact same message to more than one person in RSA, nor do you ever encrypt deterministically. This is an example of where e=3 makes a vulnerability that already exists easy to exploit, but not an example of an e=3 vulnerability. It's also the reason we have OAEP ("modern" RSA encryption). If you're passing plaintext directly to modexp to implement RSA encryption, you're boned no matter what public exponent you use.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 5, 2013

@mattkeenan --- that is a corollary only if you are using RSA unsafely to begin with. In a competent RSA design, you'd never be passing deterministic plaintext directly to RSA; every message would be different by dint of padding. You must not ever use RSA without a secure padding scheme. RSA padding is not like block cipher padding; it's not a nicety, but instead absolutely required for the security of the system.
@calyth

This comment has been minimized.

Sign in to view
Copy link

@calyth calyth replied Jul 5, 2013

For the e=3 vuln, a quick google search yield this

I've forgotten most of the crypto I learnt, but the solving the cube root rings a bell.
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 5, 2013

The cube root attack is another that only works if you're using unpadded RSA, which is unsafe with any public modulus.
@calyth

This comment has been minimized.

Sign in to view
Copy link

@calyth calyth replied Jul 5, 2013

Yeah. This stuff changes fast - I remember an older copy of applied crypto used to recommend e=3, and then the text that I was using for crypto course explained what's wrong with e=3.

Really should find the time to brush up on this stuff...
@tqbf

This comment has been minimized.

Sign in to view
Copy link

@tqbf tqbf replied Jul 5, 2013

Furgusen and Schneier, Cryptography Engineering, s 12.4.2, recommends e=3 RSA. But, don't use e=3, even though you can if you're careful.
@inthecloud247

This comment has been minimized.

Sign in to view
Copy link
Contributor

@inthecloud247 inthecloud247 replied Jul 5, 2013

How about implementing pyzmq ssh tunneling as an option?

http://zeromq.github.io/pyzmq/ssh.html
@inthecloud247

This comment has been minimized.

Sign in to view
Copy link
Contributor

@inthecloud247 inthecloud247 replied Jul 5, 2013

In response to this issue, @thatch45 has opened an issue to further improve salt's security model in the 0.17 release due mid-July:
#5913

"A few things to bring up here. First off, cryptography alone is not considered sufficient security, it is unwise to, for instance, expose ssh on the internet. Salt is a command and control system, it is unwise to expose ANY such system to the internet without aggressive firewall rules (any config management system, a system that contains code that manages a config management system or a system that contains keys allowing access to command and control systems)."
@inthecloud247

This comment has been minimized.

Sign in to view
Copy link
Contributor

@inthecloud247 inthecloud247 replied Jul 5, 2013

@kseifriedredhat Sounds like these are the approaches @thatch45 is considering in the next release ( 0.17, due mid-July 2013 ) to enhance the system security:

"The main goals in my mind with the present crypto in Salt are these:

  1. Change to use a lib where someone else maintains the crypto backend (libssh, paramiko, nacl, keyczar, libzmqCurve(very new, needs investigation), (pycrypto does a much better job of this now then when I first wrote Salt and we may be able to just lean entirely on pycrypto)). The question of us keeping up with the changes in the crypto world is of the greatest concern.
  2. Get rid of M2Crypto - this is unmaintained and not python3 compatible
  3. Keep the ssh like approach to keys (WAY easier to manage and less prone to user error - perhaps the most common cause for security breaches)
  4. Enable a pure ssh agent based system for Salt that does not require installing minions or masters at all (in the works - as in it kind of works in git right now - but can't use the awesome ZeroMQ power that makes Salt so amazing - read: slow, no peer system, no syndic, no mine etc.)"

Quoted from Saltstack Github Issue: #5913
@lucian1900

This comment has been minimized.

Sign in to view
Copy link

@lucian1900 lucian1900 replied Jul 5, 2013

Could 1. be "switch to OpenSSL-provided TLS"? TLS key handling can function like SSH's with a little massaging, so it's not fundamentally a worse user experience.
@madduck

This comment has been minimized.

Sign in to view
Copy link
Contributor

@madduck madduck replied Jul 5, 2013

@lucian1900: did you leave a comment in the issue?

In general, I suggest to move the discussion there…
@lucian1900

This comment has been minimized.

Sign in to view
Copy link

@lucian1900 lucian1900 replied Jul 5, 2013

Ah, I missed the last line in that comment somehow.
@iluminite

This comment has been minimized.

Sign in to view
Copy link
Contributor

@iluminite iluminite replied Jul 5, 2013

TLS ought to be avoided for it's inherent complexity and unresolved issues / limited (secure) crypto configuration.

aside from an ssh-based tunnel, the proper solution here is to see through enhancements to libzmq or outsourcing salt's crypto to a more common library like NaCl/pycrypto/etc/
@dlitz

This comment has been minimized.

Sign in to view
Copy link

@dlitz dlitz replied Jul 6, 2013

Those os.umask calls look odd. Are there multiple threads when this code runs?
Please sign in to comment.