Go: crypto: discourage new uses of insecure algorithms

[mdempsky]

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

[josharian]

Note that basic detection is a one-liner—pipe go list through grep.

[rsc]

"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.

[ianlancetaylor]

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?

[rsc]

I thought we were defending against a naive user who _does_ read the documentation, not that that's more likely. :-)

[mdempsky]

@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)?

[rsc]

Yes, I think it is fine to add comments pointing to specific authorities like that.

[minux]

By the same reasoning, we might as well just remove all of crypto/* packages
that provide crypto primitives because it's really hard for developers to
use
them correctly. And we should advise everyone to use high-level crypto APIs
like crypto/x509 or x/crypto/nacl instead.

No, this reasoning is fundamentally wrong. It's not the programming
language's
job to prevent bad use of libraries. Go is not a crypto toolkit, so it
should not
dictate what people should use.

I‘m against removing of any of the existing packages in crypto/*, nor should
we warn about use of insecure primitives, because, believe it or not, they
are still widely used.

The banking industry still uses 3DES a lot, probably due to extreme backward
compat requirements, and even Google Drive API only provides MD5 and
CRC32 for files.

And remember, system comprised only of secure crypto primitives is not
necessarily secure (and the sad reality is, most of these systems are indeed
not as secure as they should be.)

If you care about the security of Go programs, perhaps we should make
our various crypto primitive implementations resilient against side channel
attacks first.

[binarycrusader]

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."
https://golang.org/pkg/crypto/rc4/

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".

[tarndt]

There are legitimate non-security uses for some of these algorithms too. Take MD5 for example, its fine as a non-cryptographic hash.

[d1str0]

@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.

[gtank]

Also, perhaps cmd/vet could warn about imports of crypto/{des,md5,rc4,sha1}?

+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 crypto/hmac just doesn't convey the problem to a generalist developer. Even the new GCM example confusingly makes ASCII strings of various lengths seem like viable AES keys.

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.

[tarndt]

Also, perhaps cmd/vet could warn about imports of crypto/{des,md5,rc4,sha1}?

+1 from me. That seems like a good place to encode advice.

-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.

[mdempsky]

@tarndt Can those users configure their CI systems to disable the vet warnings they're not interested in?

[minux]

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.

[bradfitz]

I agree with Minux. New warning docs are okay, but not tool warnings.

[gopherbot]

CL https://golang.org/cl/42511 mentions this issue.