-
Notifications
You must be signed in to change notification settings - Fork 18k
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
crypto: discourage new uses of insecure algorithms #14395
Comments
Note that basic detection is a one-liner—pipe |
"Strictly factual/unopinionated" sounds right to me. These algorithms exist, there are legitimate legacy reasons to run them. I would not want the mere fact of importing crypto/md5 to trigger commentary from Go, nor would I want us to remove them from the library. But it's perfectly fine for us to update our own higher-level code not to invoke them without confirmation. For example crypto/x509 today returns an InsecureAlgorithmError for MD2+RSA and MD5+RSA, and I have been expecting that in some future release it will do that for SHA1+X too by default, with a clearly-named insecure way to override the default. |
I know little about this stuff. But in general we have those packages in the tree because they are used by other protocols. For example, crypto/des is used by crypto/tls and crypto/x509. So a warning on import is a start, but it's not like it's going to prevent your program from using DES if that is the only option presented by the other side. In general the first step in preventing an attack is characterizing it. What kind of attack are we trying to defend against? A naive user who refuses to read the documentation? |
I thought we were defending against a naive user who does read the documentation, not that that's more likely. :-) |
@rsc Do you think it would be keeping with the strictly factual tone to, for example, point out in crypto/des's documentation that DES / FIPS 46-3 has been withdrawn by NIST in favor of AES / FIPS 197 (http://www.nist.gov/itl/fips/060205_des.cfm)? |
Yes, I think it is fine to add comments pointing to specific authorities like that. |
By the same reasoning, we might as well just remove all of crypto/* packages No, this reasoning is fundamentally wrong. It's not the programming I‘m against removing of any of the existing packages in crypto/*, nor should The banking industry still uses 3DES a lot, probably due to extreme backward And remember, system comprised only of secure crypto primitives is not If you care about the security of Go programs, perhaps we should make |
I just want to point out that the existing documentation is occasionally opinionated. For example: "RC4 is in common use but has design weaknesses that make it a poor choice for new protocols." I couldn't find any other packages that provided such advice, although they do sometimes have text advising developers to "get a good random salt". |
There are legitimate non-security uses for some of these algorithms too. Take MD5 for example, its fine as a non-cryptographic hash. |
@mdempsky I think adding documentation in a way that doesn't seem bloated would be a good idea. I think it should all be kept factual an unopinionated. I do not think that we should be adding too much unnecessary information to the documentation though as this could distract from the more important aspects. |
+1 from me. That seems like a good place to encode advice. I'm also strongly in favor of updating the docs with better guidance. In particular, I think the examples should reflect correct use of the strong/modern primitives. I've had to correct damage done by copy+pasting the unauthenticated CBC example several times in the past year because that comment about Sure, a legacy system might still be using DES-HMAC-MD4 for something. That's a fine reason to have the implementations. It's not a good argument for presenting that as an equally acceptable choice to developers who aren't crypto specialists. |
-1 Go vet failures are treated as blocking failures by my, and may other people's CI systems. Users who are using these algorithms for non-security uses (of which there are plenty), will be very sad if this happens. |
@tarndt Can those users configure their CI systems to disable the vet warnings they're not interested in? |
I'm OK with adding some warnings to the docs.
But any error/warning from compiler, go vet, or any other static
checking tool is no go for me.
If you don't know crypto, you shouldn't use packages under crypto/*
at all (perhaps with the sole exception of crypto/x509, but even that
is non-trivial to use correctly.) Much better to use higher-level
functionality from x/crypto/nacl in that case.
It's quite easy to design a system with crypto/aes that has fatal
flaws (for example, bad padding algorithm and bad mac/encrypt
ordering), and make the attacker's life much easier than bruteforcing
56-bit key of DES or constructing collisions for MD5.
Good primitive will not make bad programs secure. Unless the
go compiler gains the ability to statically prove that a program is
secure, it shouldn't tell the user it's using insecure primitives.
|
I agree with Minux. New warning docs are okay, but not tool warnings. |
CL https://golang.org/cl/42511 mentions this issue. |
Updates text from https://golang.org/cl/42511 Updates #14395 Change-Id: I711100525e074ab360e577520280c37645db1c95 Reviewed-on: https://go-review.googlesource.com/42614 Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com> Reviewed-by: Rob Pike <r@golang.org>
DES, MD5, RC4, and SHA1 are all considered deprecated and discouraged for new uses, but the current Go documentation is strictly factual/unopinionated. Perhaps they should warn against new usage and/or encourage users to consider newer algorithms (e.g., AES and SHA256/512)?
Also, perhaps cmd/vet could warn about imports of crypto/{des,md5,rc4,sha1}?
/cc @agl
The text was updated successfully, but these errors were encountered: