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

crypto: discourage new uses of insecure algorithms #14395

Closed
mdempsky opened this issue Feb 19, 2016 · 16 comments
Closed

crypto: discourage new uses of insecure algorithms #14395

mdempsky opened this issue Feb 19, 2016 · 16 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 19, 2016

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

@bradfitz bradfitz added this to the Unplanned milestone Feb 19, 2016
@josharian
Copy link
Contributor

@josharian josharian commented Feb 19, 2016

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

@rsc
Copy link
Contributor

@rsc rsc commented Feb 19, 2016

"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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 19, 2016

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
Copy link
Contributor

@rsc rsc commented Feb 19, 2016

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

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Feb 19, 2016

@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
Copy link
Contributor

@rsc rsc commented Feb 19, 2016

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

@minux
Copy link
Member

@minux minux commented Feb 19, 2016

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
Copy link
Contributor

@binarycrusader binarycrusader commented Feb 19, 2016

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
Copy link

@tarndt tarndt commented Feb 19, 2016

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

@d1str0
Copy link
Contributor

@d1str0 d1str0 commented Feb 20, 2016

@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
Copy link

@gtank gtank commented Feb 25, 2016

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
Copy link

@tarndt tarndt commented Feb 26, 2016

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
Copy link
Member Author

@mdempsky mdempsky commented Feb 26, 2016

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

@minux
Copy link
Member

@minux minux commented Feb 26, 2016

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Feb 26, 2016

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

@gopherbot
Copy link

@gopherbot gopherbot commented May 2, 2017

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

@gopherbot gopherbot closed this in 25db5d1 May 3, 2017
gopherbot pushed a commit that referenced this issue May 4, 2017
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>
@golang golang locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.